Bug 200292 – [ARM64E] Harden the diversity of the DOMJIT::Signature::unsafeFunction pointer.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

<rdar://problem/53706881>

[Radar WebKit Bug Importer]

<rdar://problem/53745182>

[Mark Lam]

<rdar://problem/53706881>

[Mark Lam]

Created attachment 375206 [details]
proposed patch.

[EWS Watchlist]

Attachment 375206 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 375207 [details]
proposed patch.

[EWS Watchlist]

Attachment 375207 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 375209 [details]
proposed patch.

[Geoffrey Garen]

Comment on attachment 375209 [details]
proposed patch.

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

> Source/JavaScriptCore/domjit/DOMJITSignature.h:44
> +    __ptrauth(ptrauth_key_process_independent_code, true, WTF_PTRTAG_HASH(DOMJITUnsafeFunctionPtrTag))

It seems reasonable to comment that we need to use the process independent key here because Signature represents a read-only data structure in a shared library.

> Source/JavaScriptCore/domjit/DOMJITSignature.h:62
> +    DOM_JIT_UNSAFE_FUNCTION UnsafeFunctionPtr unsafeFunction;

Can you make a named type that combines the attributes of DOM_JIT_UNSAFE_FUNCTION and UnsafeFunctionPtr?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:12584
> +        DOMJIT::UnsafeFunctionPtr unsafeFunctionPtr = signature->unsafeFunction;

Read aloud as an English sentence, this line of code does not make sense to me.

It's very subtle and surprising that assigning from one thing called "unsafe" to another thing called "unsafe" actually has the result of increasing safety in a specifically intended way. It's also subtle and surprising that these things called unsafe have very specific safety properties, which are not the same. That says to me that these are not great names, and possibly not great abstractions.

I think signature->unsafeFunction is a function pointer signed in a certain special way. Is that right? Let's come up with a name for that.

I think unsafeFunctionPtr is a function pointer signed in the C function way. Is that right? Let's come up with a name for that.

Meanwhile, I don't understand why assigning from a specially signed value to a C signed value *increases* security, or why our type system should even allow this kind of assignment, which seems to lose information. It seems to me that losing information should require an explicit constructor or cast.

[Filip Pizlo]

(In reply to Geoffrey Garen from comment #8)
> Comment on attachment 375209 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375209&action=review
> 
> > Source/JavaScriptCore/domjit/DOMJITSignature.h:44
> > +    __ptrauth(ptrauth_key_process_independent_code, true, WTF_PTRTAG_HASH(DOMJITUnsafeFunctionPtrTag))
> 
> It seems reasonable to comment that we need to use the process independent
> key here because Signature represents a read-only data structure in a shared
> library.
> 
> > Source/JavaScriptCore/domjit/DOMJITSignature.h:62
> > +    DOM_JIT_UNSAFE_FUNCTION UnsafeFunctionPtr unsafeFunction;
> 
> Can you make a named type that combines the attributes of
> DOM_JIT_UNSAFE_FUNCTION and UnsafeFunctionPtr?
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:12584
> > +        DOMJIT::UnsafeFunctionPtr unsafeFunctionPtr = signature->unsafeFunction;
> 
> Read aloud as an English sentence, this line of code does not make sense to
> me.
> 
> It's very subtle and surprising that assigning from one thing called
> "unsafe" to another thing called "unsafe" actually has the result of
> increasing safety in a specifically intended way. 

Is it the case that this assignment does something security related?

My understanding is that the only “safe” thing here is that the ptrs had already been signed. 

> It's also subtle and
> surprising that these things called unsafe have very specific safety
> properties, which are not the same. That says to me that these are not great
> names, and possibly not great abstractions.

I think that if you remove “unsafe” from these names then you end up with names that are more accurate but not perfect. 

Based on that I think we should just remove “unsafe” from the names. 

> 
> I think signature->unsafeFunction is a function pointer signed in a certain
> special way. Is that right? Let's come up with a name for that.

Or just call it “function”. 

> 
> I think unsafeFunctionPtr is a function pointer signed in the C function
> way. Is that right? Let's come up with a name for that.
> 
> Meanwhile, I don't understand why assigning from a specially signed value to
> a C signed value *increases* security, or why our type system should even
> allow this kind of assignment, which seems to lose information. It seems to
> me that losing information should require an explicit constructor or cast.

I don’t think that C signing increases security. I think that it so happens that we resign with C tag as part of doing what clang wants us to do. As Mark explained, that’s ok, so long as the C function pointers don’t end up in the heap.

[Mark Lam]

Created attachment 375384 [details]
proposed patch.

[EWS Watchlist]

Attachment 375384 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/domjit/DOMJITSignature.h:40:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:46:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:49:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:52:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:55:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:58:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:61:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:64:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:67:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:70:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:73:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:76:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:79:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:82:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:85:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:88:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:91:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:93:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:94:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:97:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:100:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:103:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:106:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:109:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:112:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:115:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:118:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:121:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:124:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:127:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:130:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 31 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 375384 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:71
> +        : m_ptr((Ptr)&ptr)

Should be static cast

> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:76
> +        : m_ptr((Ptr)ptr)

Should be static cast

> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:112
> +    constexpr Ptr ptr() const { return m_ptr; }

Smart pointers in WebKit usually call this function get()

[Mark Lam]

Comment on attachment 375384 [details]
proposed patch.

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

Thanks for the review.

>> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:71
>> +        : m_ptr((Ptr)&ptr)
> 
> Should be static cast

Clang says:
... error: static_cast from 'long long (*)(JSC::ExecState *, (anonymous namespace)::DOMJITNode *)' to 'JSC::CFunctionPtr::Ptr' (aka 'void (*)()') is not allowed
        : m_ptr(static_cast<Ptr>(&ptr))

Basically, we cannot static cast functions of one prototype into another prototype.

I thought we cannot use reinterpret_cast here either because it breaks constexpr, but I'm wrong.  I just retested it, and Clang is happy with the reinterpret_cast.  So, I'll go with that.

>> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:112
>> +    constexpr Ptr ptr() const { return m_ptr; }
> 
> Smart pointers in WebKit usually call this function get()

I changed this to get().

[Mark Lam]

Landed in r248192: <http://trac.webkit.org/r248192>.