216638 – Add some pointer sanity checks to speculationFromCell().

Bug 216638 - Add some pointer sanity checks to speculationFromCell().

Summary: Add some pointer sanity checks to speculationFromCell().

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	JavaScriptCore (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Mark Lam

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2020-09-16 23:01 PDT by Mark Lam
Modified:	2020-09-17 19:07 PDT (History)
CC List:	7 users (show)

See Also:

Attachments
proposed patch. (2.78 KB, patch) 2020-09-16 23:27 PDT, Mark Lam	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
proposed patch. (2.78 KB, patch) 2020-09-16 23:33 PDT, Mark Lam	ysuzuki: review+	Details \| Formatted Diff \| Diff
patch for landing. (2.85 KB, patch) 2020-09-17 00:08 PDT, Mark Lam	no flags	Details \| Formatted Diff \| Diff
proposed patch. (2.85 KB, patch) 2020-09-17 02:01 PDT, Mark Lam	no flags	Details \| Formatted Diff \| Diff
proposed patch. (5.12 KB, patch) 2020-09-17 02:05 PDT, Mark Lam	ysuzuki: review+ ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
proposed patch. (5.52 KB, patch) 2020-09-17 02:11 PDT, Mark Lam	no flags	Details \| Formatted Diff \| Diff
addressing Saam's feedback. (1.63 KB, patch) 2020-09-17 13:33 PDT, Mark Lam	saam: review+	Details \| Formatted Diff \| Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Mark Lam 2020-09-16 23:01:58 PDT

rdar://23226333

Comment 1 Mark Lam 2020-09-16 23:27:01 PDT

Created attachment 408993 [details]
proposed patch.

Comment 2 Yusuke Suzuki 2020-09-16 23:32:25 PDT

Comment on attachment 408993 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=408993&action=review

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:559
> +#if USE(JSVALUE64)
> +    uintptr_t pointerAsInt = bitwise_cast<uintptr_t>(pointer);
> +    uintptr_t canonicalPointerBits = pointerAsInt << 16;
> +    uintptr_t nonCanonicalPointerBits = pointerAsInt >> 48;
> +    return !nonCanonicalPointerBits && canonicalPointerBits;
> +#else

I think this is not correct for ARM64_32 since pointer is 32bit.
Let's use CPU(ADDRESS64).

Comment 3 Mark Lam 2020-09-16 23:32:35 PDT

Comment on attachment 408993 [details]
proposed patch.

New patch coming soon to address watch build failure.

Comment 4 Mark Lam 2020-09-16 23:33:36 PDT

Created attachment 408995 [details]
proposed patch.

Comment 5 Yusuke Suzuki 2020-09-16 23:36:09 PDT

Comment on attachment 408995 [details]
proposed patch.

r=me

Comment 6 Yusuke Suzuki 2020-09-16 23:58:40 PDT

Comment on attachment 408995 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=408995&action=review

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:568
> +        return SpecNone;

Let's crash if it is debug build.

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:573
> +                return SpecNone;

Ditto.

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:581
> +        return SpecNone;

Ditto.

Comment 7 Mark Lam 2020-09-17 00:08:23 PDT

(In reply to Yusuke Suzuki from comment #6)
> Comment on attachment 408995 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408995&action=review
> 
> > Source/JavaScriptCore/bytecode/SpeculatedType.cpp:568
> > +        return SpecNone;
> 
> Let's crash if it is debug build.

I'm going to add the ASSERT in the isSanePointer() function instead.

Comment 8 Mark Lam 2020-09-17 00:08:56 PDT

Created attachment 408998 [details]
patch for landing.

Comment 9 Mark Lam 2020-09-17 02:01:04 PDT

Created attachment 409010 [details]
proposed patch.

Add 1 more mitigation, and moved the ASSERT into speculationFromCell() after discussing with Yusuke offline.

Comment 10 Mark Lam 2020-09-17 02:04:04 PDT

Comment on attachment 409010 [details]
proposed patch.

Uploaded the wrong patch.

Comment 11 Mark Lam 2020-09-17 02:05:16 PDT

Created attachment 409011 [details]
proposed patch.

Comment 12 Yusuke Suzuki 2020-09-17 02:10:56 PDT

Comment on attachment 409011 [details]
proposed patch.

r=me if EWS gets green.

Comment 13 Mark Lam 2020-09-17 02:11:04 PDT

Created attachment 409012 [details]
proposed patch.

Comment 14 Yusuke Suzuki 2020-09-17 02:17:37 PDT

Comment on attachment 409012 [details]
proposed patch.

r=me

Comment 15 Mark Lam 2020-09-17 08:25:01 PDT

Every test on the jsc-armv7-tests bot is failing with this message: “jsc: error while loading shared libraries: libicudata.so.63: cannot open shared object file: No such file or directory”.  These are not due to this patch.

Comment 16 Mark Lam 2020-09-17 08:25:25 PDT

Comment on attachment 409012 [details]
proposed patch.

Landing now.

Comment 17 EWS 2020-09-17 08:43:23 PDT

Committed r267192: <https://trac.webkit.org/changeset/267192>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409012 [details].

Comment 18 Saam Barati 2020-09-17 12:11:08 PDT

Comment on attachment 409012 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=409012&action=review

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:557
> +    uintptr_t nonCanonicalPointerBits = pointerAsInt >> 48;

we should use the effective address width constant instead of 48

Comment 19 Mark Lam 2020-09-17 12:35:16 PDT

Reopening to address Saam's feedback.

Comment 20 Mark Lam 2020-09-17 13:33:45 PDT

Created attachment 409061 [details]
addressing Saam's feedback.

Comment 21 Mark Lam 2020-09-17 19:07:22 PDT

Thanks for the reviews.  Landed in 267221: <http://trac.webkit.org/r267221>.