1744525 - Crash [@ <missing>]

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Description

Testcase found while fuzzing mozilla-central rev 422c7594bf65 (built with: --enable-address-sanitizer --enable-fuzzing).

This testcase only reproduces on linux with xvfb. As this is the standard method for running our fuzz tests, bugs like these have a significant impact on overall fuzzing performance. Please prioritize accordingly.

Testcase can be reproduced using the following commands:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch --build 422c7594bf65 --asan --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.html --xvfb

[@ <missing>]

    =================================================================
    ==703078==ERROR: AddressSanitizer: SEGV on unknown address 0x7fb06a395320 (pc 0x7fb16aeac299 bp 0x7fff739b75d0 sp 0x7fff739b72e0 T0)
    ==703078==The signal is caused by a READ memory access.
        #0 0x7fb16aeac299  (<unknown module>)
        #1 0x7fb1800c57ab  (/usr/lib/x86_64-linux-gnu/dri/swrast_dri.so+0x7857ab)
    
    AddressSanitizer can not provide additional info.
    SUMMARY: AddressSanitizer: SEGV (<unknown module>) 
    ==703078==ABORTING

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: Detailed Crash Information

Comment 2

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20211206092340-422c7594bf65.
Failed to bisect testcase (Testcase reproduces on start build!):

Start: e090b0e10f5de9c444d145caeff7b166fbc17736 (20201207092823)
End: 422c7594bf65d1e930d52165356b0098402ff6fe (20211206092340)
BuildFlags: BuildFlags(asan=True, tsan=False, debug=False, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False)

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:jgilbert, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/XwfK8603nlCqyIRumBWajg/index.html

Comment 6

[Kelsey Gilbert [:jgilbert]]

The main suspicious line from the testcase:

drawArrays(context.TRIANGLES, 145346482, 21017)

Comment 7

[Jason Kratzer [:jkratzer]]

Kelsey, this bug is currently blocking our libfuzzer xpcshell fuzzing efforts. Could you please prioritize accordingly? If there's anything I can do to help, please let me know.

Comment 8

[Tyson Smith [:tsmith]]

Looking at the Pernosco session I think this might be due to stack-corruption, Kelsey do you agree?

Bug 1755448 might also be a duplicate or related (note the stack-less SEGV report on that bug).

Comment 9

[Andrew McCreight [:mccr8]]

bug 1749292 might also be similar.

Comment 10

[Jim Mathies [:jimm]]

This just showed up in the latest security report. Maybe we can find a webgl owner here.

Comment 11

[Kelsey Gilbert [:jgilbert]]

I think our fake-attrib0 glBufferData allocation is failing, and it doesn't look like we're checking it.

Comment 12

[Kelsey Gilbert [:jgilbert]]

Elsewhere, we have RBAB, but on Mesa, we disabled it in bug 1429963.
We need try harder to re-enable it, I think.

Comment 13

[Kelsey Gilbert [:jgilbert]]

(RBAB should cause this to fail more safely, even if we forget to check an allocation)

Comment 14

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1744525 - Check that FakeVertexAttrib0Buffer allocation succeeded.

Comment 15

[Kelsey Gilbert [:jgilbert]]

Can you try again with this patch? (Or should I have CI make builds for you?)

Comment 16

[Tyson Smith [:tsmith]]

I can do it right now.

Comment 17

[Kelsey Gilbert [:jgilbert]]

(In reply to Kelsey Gilbert [:jgilbert] (previously Jeff) from comment #12)

Elsewhere, we have RBAB, but on Mesa, we disabled it in bug 1429963.
We need try harder to re-enable it, I think.

Actually this doesn't disable it, it just tells WebGL to act like it's not present, and do extra work to do validation ourselves, which is fine, probably. (though hurts perf)

Comment 18

[Tyson Smith [:tsmith]]

I am still able to trigger the crash with the attached test case with the patch applied.

I can get a Pernosco session but I will need to rebuilt with optimization disabled. Let me know if you'd like me to do so.

Comment 19

[Kelsey Gilbert [:jgilbert]]

I think I did it:

[2022-03-14 07:31:51] Result: [@ <missing>] (fe8e32ce:bbbd22ac)
[2022-03-14 07:31:51] Result successfully reproduced

Comment 20

[Kelsey Gilbert [:jgilbert]]

Yes, I'm working on minimizing the testcase.
Right now it looks very internal to mesa.
Current theory is that while our gecko math is proof against overflows, mesa isn't handling this large offset (~140M) very well.
Perhaps mesa is expanding a vec2 to a vec4, and then having i32 overflow issues?

Comment 21

[Kelsey Gilbert [:jgilbert]]

Attachment: testcase6.html (more minimal)

Looks like it is indeed just the attrib that's needed. Removing the attrib, or causing it to go unused (and thus eliminated by the compiler) prevents the issue.

Comment 22

[Kelsey Gilbert [:jgilbert]]

      let offset = 145346482; // bad
      offset = 0x8000 * 0x10000 / (4*4); // 134217728, bad
      offset -= 0x100; // good
      context.drawArrays(context.TRIANGLES, offset, 3)

Hypothesis:

• vec2 is padded to vec4, 16 bytes
• mesa has an i32 overflow if offset*16 >= i32_max

Comment 23

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1744525 - Prevent too-high vert-count draws on Mesa.

Comment 24

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9268157 [details]
Bug 1744525 - Prevent too-high vert-count draws on Mesa.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I think it would be hard. This is an out-of-bounds read seg-fault.
  This is more of a sec-vector than exploitable on its own.
  I believe Mesa allocates a too-small buffer in this case, and fails when it tries to read off the end. Alternatively, it might instead get addressing math wrong, and be reading from a negative offset, in which case it would be a user-controlled-offset read.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Easy and not risky.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions. Existing CI testing should be sufficient.

Comment 25

[Tom Ritter [:tjr]]

Comment on attachment 9268157 [details]
Bug 1744525 - Prevent too-high vert-count draws on Mesa.

Approved to land and uplift. For Advisory purposes, please comment if this is restricted to any particular operating system.

Comment 26

[Kelsey Gilbert [:jgilbert]]

This is restricted to Linux machines running Mesa as a GPU driver. (most Linux systems)

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Prevent too-high vert-count draws on Mesa. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/63b6391367f9b2d762505329151b9880596b54ca
https://hg.mozilla.org/mozilla-central/rev/63b6391367f9

Comment 28

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 29

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as fixed on rev mozilla-central 20220322051941-fb31185e427d.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Regarding uplifts, this conflicts with bug 1759942 as-landed. We'll need a rebased patch for Beta/ESR along with approval requests.

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 32

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1744525 - (beta) Prevent too-high vert-count draws on Mesa.

Comment 33

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9269105 [details]
Bug 1744525 - (beta) Prevent too-high vert-count draws on Mesa.

Beta/Release Uplift Approval Request

• User impact if declined: sec-high on Linux+Mesa
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): At worst this should over-reject, but it should be safe.
• String changes made/needed: none

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: sec-high on Linux+Mesa
• Fix Landed on Version: 100
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): At worst this should over-reject, but it should be safe.

Comment 34

[Donal Meehan [:dmeehan]]

Comment on attachment 9269105 [details]
Bug 1744525 - (beta) Prevent too-high vert-count draws on Mesa.

Approved for 99.0b8. Thanks.

Comment 35

[Donal Meehan [:dmeehan]]

Prevent too-high vert-count draws on Mesa. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/releases/mozilla-beta/rev/d608a830c35d

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9269105 [details]
Bug 1744525 - (beta) Prevent too-high vert-count draws on Mesa.

Approved for 91.8esr, thanks for the timely rebase.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/160f7156fea4

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Follow-up bustage fix for ESR91 (IsMesa() didn't exist until 93):
https://hg.mozilla.org/releases/mozilla-esr91/rev/6e352c94beadeef5e328d83dad715baefb147f85

Comment 39

[BugBot [:suhaib / :marco/ :calixte]]

:jgilbert, since this bug contains a bisection range, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 40

[Andrew McCreight [:mccr8]]

Comment 3 says "Failed to bisect testcase".

Comment 41

[Andrew McCreight [:mccr8]]

I filed an autonag issue for that.