Bug 210136 – [macOS] Switch unused IOKit classes from allow-with-report to deny-with-report

• Component: WebKit2
• Status: RESOLVED FIXED

[Brent Fulgham]

After collecting several weeks of telemetry, we can now switch from allowing (with reports) to denying access (with reports).

[Radar WebKit Bug Importer]

<rdar://problem/61403216>

[Brent Fulgham]

Created attachment 395717 [details]
Patch

[Brent Fulgham]

Tested on macOS and iOS.

[Per Arne Vollan]

Comment on attachment 395717 [details]
Patch

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

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:619
> +    (iokit-registry-entry-class "IOFramebufferSharedUserClient")

This seems to be allowed earlier in the sandbox, is that expected?

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:727
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED > 101500
> +    (with report)
> +#endif

Would it make sense to also have telemetry-backtrace here?

[Per Arne Vollan]

Comment on attachment 395717 [details]
Patch

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

R=me.

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:603
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101500

Should this be '< 101600'?

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:612
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500

Should this be '>= 101600'?

[Brent Fulgham]

Comment on attachment 395717 [details]
Patch

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

I think I'll be more conservative and only impact 10.16, since we might want to back-port some of this to 10.15 and I don't want to introduce any issues.

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:123
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101600

This should have been < 101500

>> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:603
>> +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101500
> 
> Should this be '< 101600'?

I don't think so. We did not see these being used on 10.15, but could not collect telemetry on older systems. So this leaves older systems alone, and blocks on the systems we could measure.

>> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:619
>> +    (iokit-registry-entry-class "IOFramebufferSharedUserClient")
> 
> This seems to be allowed earlier in the sandbox, is that expected?

Ah! My earlier condition was wrong. I'll fix that.

>> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:727
>> +#endif
> 
> Would it make sense to also have telemetry-backtrace here?

Sure -- I'll change that.

[Brent Fulgham]

Created attachment 396113 [details]
Patch for landing

[EWS]

Committed r259891: <https://trac.webkit.org/changeset/259891>

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

[Said Abou-Hallawa]

Reverted r259891 for reason:

Causes significant iOS MotionMark regression

Committed r260120: <https://trac.webkit.org/changeset/260120>

[Brent Fulgham]

MotionMark on iPad Pro (original sandbox): 894.52 +/- 9.44%
MotionMark on iPad Pro (revised sandbox): 885.32 +- 10.50%

[Brent Fulgham]

Created attachment 396579 [details]
Patch

[Per Arne Vollan]

Comment on attachment 396579 [details]
Patch

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

Great! R=me, given the MotionMark regression is fixed :)

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:222
>                  "AGXCommandQueue"
>                  "AGXDevice"
> -                "AGXSharedUserClient"
> +                "AGXSharedUserClient"))

Do we need all of these?

[Brent Fulgham]

Let's use this radar for the macOS bits. We'll do a separate one for the iOS changes.

[Brent Fulgham]

Created attachment 396683 [details]
Patch

[Per Arne Vollan]

Comment on attachment 396683 [details]
Patch

R=me.

[EWS]

Committed r260221: <https://trac.webkit.org/changeset/260221>

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