LIR_jf can generate jump-to-location-zero code on i386

RESOLVED FIXED

Status

Core Graveyard
Nanojit
--
major
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Steven Johnson, Assigned: Steven Johnson)

Tracking

({crash})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:dos])

Attachments

(2 attachments)

(Assignee)

Description

7 years ago
LIR of the form

    eqd a,b 
    jf somewhere

can generate i386 code of the form

     ucomisd xmm0,xmm1
     jne   valid-address
     jp    0x0


This bug appears to have been injected in 1450:8cb17c62aaa6, "Bug 575850 - nanojit: generated better code for LIR_eqd on i386" by nnethercote.
(Assignee)

Comment 1

7 years ago
(Not sure of security implications; erring on safe side for now.)
(Assignee)

Comment 2

7 years ago
Created attachment 521061 [details] [diff] [review]
Patch

Naively, this fixes the crash case that I know of (Adobe internal Watson bug #2826811); someone with more nanojit-fu should look at it more closely and probably also scour similar code to see if there are other places we missed. (And probably generate a lirasm testcase too...)
Attachment #521061 - Flags: superreview?(nnethercote)
Attachment #521061 - Flags: review?(rreitmai)
(Assignee)

Comment 3

7 years ago
(Flash note: I've verified this bug is *not* in the nanojit version that's in Wasabi.)

Comment 4

7 years ago
> LIR of the form
> 
>     eqd a,b 
>     jf somewhere
> 
> can generate i386 code of the form
> 
>      ucomisd xmm0,xmm1
>      jne   valid-address
>      jp    0x0

sounds like the problem is when 'somewhere' is a backwards branch in LIR that requires patching, and the bug is in the patching code?  Two instructions would need to be patched.

the jt and jf (and xt/xf) cases with eqd are not symmetric due to the way the intel FPU condition codes are set (there are detailed comments in the x86 and x64 code).  If I'm reading the code right, the same bug might exist in X64 too.
(Assignee)

Comment 5

7 years ago
I hadn't thought about the nonsymmetric condition code possibility -- eww. I guess the patch just happened to work in my test case.

Comment 6

7 years ago
Yeah, I think the patch is probably correct, but if the patch code could update both branches, then the old code probably would be correct too.

Updated

7 years ago
Attachment #521061 - Flags: review?(rreitmai) → review+
(Assignee)

Comment 7

7 years ago
Anyone want to chime in on whether this is security-worthy or not?
(Assignee)

Comment 8

7 years ago
Comment on attachment 521061 [details] [diff] [review]
Patch

Changing SR? to Edwin at njn's request.
Attachment #521061 - Flags: superreview?(nnethercote) → superreview?(edwsmith)
(Assignee)

Comment 9

7 years ago
All: this bug is a conditional jump to location 0x00000000 ... basically, an unpatched jump, which (AFAIK) is always to location zero, rather than an arbitrary location.

If it is indeed loc zero always, then this is probably declassifiable (which will make it much easier to land), but opinions from people with more intimate nanojit knowledge would be welcome.

Updated

7 years ago
Attachment #521061 - Flags: superreview?(edwsmith) → superreview+

Comment 10

7 years ago
We should leave this locked down until njn responds about security.  We haven't shipped the bug yet but FF4 probably has.

also, the fix is incomplete:
- x64
- tests
(Assignee)

Comment 11

7 years ago
x64 is not susceptible, if I read the code correctly. re: test, yes, a lirasm test is appropriate.

I'll wait for njn to chime in before I land or declassify anything.
(In reply to comment #11)
> 
> I'll wait for njn to chime in before I land or declassify anything.

I spoke to you yesterday about this on IRC, that's what prompted comment 9.  To repeat:  if it's always a jump to 0, it's not security-sensitive.  Otherwise it is.
(Assignee)

Comment 13

7 years ago
Update: existing patch is suboptimal,  new one on the way, along with testcase. X64 *does* appear to have a similar problem after all.
Group: core-security
Keywords: crash
Whiteboard: [sg:dos]
(Assignee)

Comment 14

7 years ago
Extra fun note: this bug only turns up for backward jumps, but apparently lirasm doesn't handle backward jumps properly (it leaves the branch unpatched).
(Assignee)

Updated

7 years ago
Depends on: 644473
(Assignee)

Comment 15

7 years ago
Looks like X64 wasn't susceptible: nPatchBranch was doing some evil special-casing to handle this case. (I'll clean it up to be less hacky.)
(Assignee)

Comment 16

7 years ago
Created attachment 523435 [details] [diff] [review]
Proper patch, plus testcase
Assignee: nobody → stejohns
Attachment #523435 - Flags: superreview?(edwsmith)
Attachment #523435 - Flags: review?(rreitmai)
Attachment #523435 - Flags: feedback?(nnethercote)
Comment on attachment 523435 [details] [diff] [review]
Proper patch, plus testcase

>+            Branches branches = asm_branch(branchOnFalse, cond, 0);
>+printf("b1=%p %p\n",branches.branch1,branches.branch2);
>+            if (branches.branch1) {
>+                _patches.put(branches.branch1,to);
>+            }
>+            if (branches.branch2) {
>+                _patches.put(branches.branch2,to);
>+            }

I assume that's a debugging printf you'll remove?

I only did a light review, but it seems fine.
Attachment #523435 - Flags: feedback?(nnethercote) → feedback+
(Assignee)

Comment 18

7 years ago
(In reply to comment #17)
> I assume that's a debugging printf you'll remove?

Ooooops.

Comment 19

7 years ago
Comment on attachment 523435 [details] [diff] [review]
Proper patch, plus testcase

Should Branches ctor have a default value for b1?  b2 , I can see, but don't we want to enforce that at least one branch is set.  Unless I missed a use-case in the code, where we constructed it and then set branch1/2.
Attachment #523435 - Flags: review?(rreitmai) → review+
(Assignee)

Comment 20

7 years ago
pushed to nanojit-central as 1503:3b0667d8dc54.
leaving SR? to edwsmith pending per his request.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
A debugging printf was left behind that was causing test failures.  I removed it:

http://hg.mozilla.org/projects/nanojit-central/rev/8202c5872474
http://hg.mozilla.org/tracemonkey/rev/716f78be9df4
http://hg.mozilla.org/tracemonkey/rev/eefaab0fbe9a

Comment 24

7 years ago
changeset: 6228:2337c19085fb
user:      Steven Johnson <stejohns@adobe.com>
summary:   Bug 643969 - LIR_jf can generate jump-to-location-zero code on i386 and x64 (r=rreitmai)

http://hg.mozilla.org/tamarin-redux/rev/2337c19085fb

Comment 25

7 years ago
changeset: 6230:c94f8a962836
user:      Nicholas Nethercote <nnethercote@mozilla.com>
summary:   Remove debugging printf left behind by patch for bug 643969 that was causing test failures.

http://hg.mozilla.org/tamarin-redux/rev/c94f8a962836

Updated

7 years ago
Attachment #523435 - Flags: superreview?(edwsmith) → superreview+
Component: Nanojit → Nanojit
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.