Bug 221841 – [ARM64e] Harden Mach exception handling

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

This change is to make it more difficult to abuse mach exception handling on ARM64e hardware.

[Radar WebKit Bug Importer]

<rdar://problem/74289949>

[Michael Saboff]

Created attachment 420172 [details]
Patch

[Geoffrey Garen]

Comment on attachment 420172 [details]
Patch

r=me

[Mark Lam]

Comment on attachment 420172 [details]
Patch

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

r=me too

> Source/JavaScriptCore/llint/WebAssembly.asm:542
> +    move instance, a2

This is not needed.  In both cases, you are still passing the wasmInstance in via a2.  I think you can restore the comment here.

> Source/WTF/wtf/threads/Signals.cpp:164
> +    ptrauth_generic_signature_t hash = 0;

Why start with 0?  Why not some non-zero number?  How about initialize it to `ptrauth_string_discriminator("Mach Exception Signal State")`?

[Michael Saboff]

Comment on attachment 420172 [details]
Patch

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

>> Source/JavaScriptCore/llint/WebAssembly.asm:542
>> +    move instance, a2
> 
> This is not needed.  In both cases, you are still passing the wasmInstance in via a2.  I think you can restore the comment here.

I'll restore a similar comment and remove the instruction,

>> Source/WTF/wtf/threads/Signals.cpp:164
>> +    ptrauth_generic_signature_t hash = 0;
> 
> Why start with 0?  Why not some non-zero number?  How about initialize it to `ptrauth_string_discriminator("Mach Exception Signal State")`?

It is actually seeded below with the call to ptrauth_sign_generic_data(hash, mach_thread_self()).  Using the thread id allows it to have a dynamic seed instead of a static one.

[Michael Saboff]

Committed r272823 (234059@main): <https://commits.webkit.org/234059@main>

[Mark Lam]

(In reply to Michael Saboff from comment #5)
> >> Source/WTF/wtf/threads/Signals.cpp:164
> >> +    ptrauth_generic_signature_t hash = 0;
> > 
> > Why start with 0?  Why not some non-zero number?  How about initialize it to `ptrauth_string_discriminator("Mach Exception Signal State")`?
> 
> It is actually seeded below with the call to ptrauth_sign_generic_data(hash,
> mach_thread_self()).  Using the thread id allows it to have a dynamic seed
> instead of a static one.

Note: you're "seed" is produced by signing a 0 value.  Why not use a non-zero value?