Bug 210136

Summary: [macOS] Switch unused IOKit classes from allow-with-report to deny-with-report
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit2Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, pvollan, sabouhallawa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch
none
Patch none

Description Brent Fulgham 2020-04-07 11:49:57 PDT
After collecting several weeks of telemetry, we can now switch from allowing (with reports) to denying access (with reports).
Comment 1 Radar WebKit Bug Importer 2020-04-07 11:50:23 PDT
<rdar://problem/61403216>
Comment 2 Brent Fulgham 2020-04-07 11:55:27 PDT
Created attachment 395717 [details]
Patch
Comment 3 Brent Fulgham 2020-04-07 12:03:05 PDT
Tested on macOS and iOS.
Comment 4 Per Arne Vollan 2020-04-07 15:00:58 PDT
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?
Comment 5 Per Arne Vollan 2020-04-10 10:51:25 PDT
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'?
Comment 6 Brent Fulgham 2020-04-10 12:12:38 PDT
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.
Comment 7 Brent Fulgham 2020-04-10 12:31:57 PDT
Created attachment 396113 [details]
Patch for landing
Comment 8 EWS 2020-04-10 12:55:23 PDT
Committed r259891: <https://trac.webkit.org/changeset/259891>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396113 [details].
Comment 9 Said Abou-Hallawa 2020-04-15 01:54:07 PDT
Reverted r259891 for reason:

Causes significant iOS MotionMark regression

Committed r260120: <https://trac.webkit.org/changeset/260120>
Comment 10 Brent Fulgham 2020-04-15 14:18:15 PDT
MotionMark on iPad Pro (original sandbox): 894.52 +/- 9.44%
MotionMark on iPad Pro (revised sandbox): 885.32 +- 10.50%
Comment 11 Brent Fulgham 2020-04-15 15:02:06 PDT
Created attachment 396579 [details]
Patch
Comment 12 Per Arne Vollan 2020-04-15 15:19:28 PDT
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?
Comment 13 Brent Fulgham 2020-04-16 12:03:19 PDT
Let's use this radar for the macOS bits. We'll do a separate one for the iOS changes.
Comment 14 Brent Fulgham 2020-04-16 12:03:38 PDT
Created attachment 396683 [details]
Patch
Comment 15 Per Arne Vollan 2020-04-16 12:09:24 PDT
Comment on attachment 396683 [details]
Patch

R=me.
Comment 16 EWS 2020-04-16 13:54:35 PDT
Committed r260221: <https://trac.webkit.org/changeset/260221>

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