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

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Steven Johnson]

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.

Comment 1

[Steven Johnson]

(Not sure of security implications; erring on safe side for now.)

Comment 2

[Steven Johnson]

Attachment: 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...)

Comment 3

[Steven Johnson]

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

Comment 4

[Edwin Smith]

> 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.

Comment 5

[Steven Johnson]

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

[Edwin Smith]

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.

Comment 7

[Steven Johnson]

Anyone want to chime in on whether this is security-worthy or not?

Comment 8

[Steven Johnson]

Comment on attachment 521061 [details] [diff] [review]
Patch

Changing SR? to Edwin at njn's request.

Comment 9

[Steven Johnson]

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.

Comment 10

[Edwin Smith]

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

Comment 11

[Steven Johnson]

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.

Comment 12

[Nicholas Nethercote [inactive]]

(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.

Comment 13

[Steven Johnson]

Update: existing patch is suboptimal,  new one on the way, along with testcase. X64 *does* appear to have a similar problem after all.

Comment 14

[Steven Johnson]

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).

Comment 15

[Steven Johnson]

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.)

Comment 16

[Steven Johnson]

Attachment: Proper patch, plus testcase

Comment 17

[Nicholas Nethercote [inactive]]

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.

Comment 18

[Steven Johnson]

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

Ooooops.

Comment 19

[Rick Reitmaier]

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.

Comment 20

[Steven Johnson]

pushed to nanojit-central as 1503:3b0667d8dc54.
leaving SR? to edwsmith pending per his request.

Comment 21

[Nicholas Nethercote [inactive]]

A debugging printf was left behind that was causing test failures.  I removed it:

http://hg.mozilla.org/projects/nanojit-central/rev/8202c5872474

Comment 22

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/716f78be9df4

Comment 23

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/eefaab0fbe9a

Comment 24

[Tamarin Bot]

changeset: 6228:2337c19085fb
user:      Steven Johnson <[e-mail]>
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

[Tamarin Bot]

changeset: 6230:c94f8a962836
user:      Nicholas Nethercote <[e-mail]>
summary:   Remove debugging printf left behind by patch for bug 643969 that was causing test failures.

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