Bug 210616 – [iOS] Deny iokit open access to graphics related classes

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

Deny iokit open access to graphics related classes in the WebContent process on iOS, but issue extensions for these for some devices which still need access to them.

[Per Arne Vollan]

Created attachment 396696 [details]
Patch

[Per Arne Vollan]

Created attachment 396702 [details]
Patch

[Per Arne Vollan]

Created attachment 396707 [details]
Patch

[Per Arne Vollan]

rdar://problem/61897550

[Darin Adler]

Comment on attachment 396707 [details]
Patch

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

> Source/WebCore/platform/cocoa/AGXCompilerService.cpp:48
> +        if (uname(&systemInfo))
> +            return false;

Is this the behavior we want, to return a potentially incorrect "false" result and to also repeatedly call uname every time this function is called?

> Source/WebCore/platform/cocoa/AGXCompilerService.cpp:53
> +        if (!strcmp(machine, "iPad5,1") || !strcmp(machine, "iPad5,2") || !strcmp(machine, "iPad5,3") || !strcmp(machine, "iPad5,4"))
> +            hasAGXCompilerService = true;
> +        else
> +            hasAGXCompilerService = false;

Apparently this code was just moved, not new, but we want this to be false for all future hardware, including future iPads?

> Source/WebCore/testing/Internals.mm:127
> +        RELEASE_ASSERT_NOT_REACHED();

Seems peculiar to put RELEASE_ASSERT in an internals function, since they are present only for testing.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:378
> +        static const char* ioKitClasses[] = {

One more const here would be good, after the *. Or constexpr before the whole thing.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:388
> +            "AGXCommandQueue",
> +            "AGXDevice",
> +            "AGXSharedUserClient",
> +            "IOAccelContext",
> +            "IOAccelDevice",
> +            "IOAccelSharedUserClient",
> +            "IOAccelSubmitter2",
> +            "IOAccelContext2",
> +            "IOAccelDevice2",
> +            "IOAccelSharedUserClient2"

Why not sort alphabetically instead of having them almost sorted?

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:300
> +    for (size_t i = 0, size = parameters.dynamicIOKitExtensionHandles.size(); i < size; ++i)

Modern for loop would be good here.

    for (auto& handle : parameters.dynamicIOKitExtensionHandles)

> Tools/TestWebKitAPI/Tests/WebKit/AGXCompilerService.mm:37
> +    WKRetainPtr<WKContextRef> context = adoptWK(TestWebKitAPI::Util::createContextForInjectedBundleTest("InternalsInjectedBundleTest"));

auto

[Per Arne Vollan]

Created attachment 396717 [details]
Patch

[Per Arne Vollan]

(In reply to Darin Adler from comment #5)
> Comment on attachment 396707 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396707&action=review
> 
> > Source/WebCore/platform/cocoa/AGXCompilerService.cpp:48
> > +        if (uname(&systemInfo))
> > +            return false;
> 
> Is this the behavior we want, to return a potentially incorrect "false"
> result and to also repeatedly call uname every time this function is called?
> 
> > Source/WebCore/platform/cocoa/AGXCompilerService.cpp:53
> > +        if (!strcmp(machine, "iPad5,1") || !strcmp(machine, "iPad5,2") || !strcmp(machine, "iPad5,3") || !strcmp(machine, "iPad5,4"))
> > +            hasAGXCompilerService = true;
> > +        else
> > +            hasAGXCompilerService = false;
> 
> Apparently this code was just moved, not new, but we want this to be false
> for all future hardware, including future iPads?
> 

Yes, I believe that this will be the case going forward.

> > Source/WebCore/testing/Internals.mm:127
> > +        RELEASE_ASSERT_NOT_REACHED();
> 
> Seems peculiar to put RELEASE_ASSERT in an internals function, since they
> are present only for testing.
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:378
> > +        static const char* ioKitClasses[] = {
> 
> One more const here would be good, after the *. Or constexpr before the
> whole thing.
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:388
> > +            "AGXCommandQueue",
> > +            "AGXDevice",
> > +            "AGXSharedUserClient",
> > +            "IOAccelContext",
> > +            "IOAccelDevice",
> > +            "IOAccelSharedUserClient",
> > +            "IOAccelSubmitter2",
> > +            "IOAccelContext2",
> > +            "IOAccelDevice2",
> > +            "IOAccelSharedUserClient2"
> 
> Why not sort alphabetically instead of having them almost sorted?
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:300
> > +    for (size_t i = 0, size = parameters.dynamicIOKitExtensionHandles.size(); i < size; ++i)
> 
> Modern for loop would be good here.
> 
>     for (auto& handle : parameters.dynamicIOKitExtensionHandles)
> 
> > Tools/TestWebKitAPI/Tests/WebKit/AGXCompilerService.mm:37
> > +    WKRetainPtr<WKContextRef> context = adoptWK(TestWebKitAPI::Util::createContextForInjectedBundleTest("InternalsInjectedBundleTest"));
> 
> auto

Thanks for reviewing! The latest patch addresses all of the issues above.

[Per Arne Vollan]

Created attachment 396724 [details]
Patch

[EWS]

Committed r260247: <https://trac.webkit.org/changeset/260247>

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

[Daniel Bates]

Comment on attachment 396707 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit/AGXCompilerService.mm:28
> +#if WK_HAVE_C_SPI && PLATFORM(IOS)

This test is never run: WK_HAVE_C_SPI is only defined when building on Mac.

[Per Arne Vollan]

(In reply to Daniel Bates from comment #10)
> Comment on attachment 396707 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396707&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKit/AGXCompilerService.mm:28
> > +#if WK_HAVE_C_SPI && PLATFORM(IOS)
> 
> This test is never run: WK_HAVE_C_SPI is only defined when building on Mac.

Ah, good point! I will look into making this test run on iOS.