X64 branch patch code seems to be wrong for jmp 64bit, but is actually fine: comment needed.

RESOLVED INVALID

Status

RESOLVED INVALID
7 years ago
5 years ago

People

(Reporter: shu, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
When patching jmp 64bit targets, we write the target 6 bytes from the start of the jmp. AIUI, jmp 64bit is FF/4, then the operand.
(Reporter)

Comment 1

7 years ago
Created attachment 553508 [details] [diff] [review]
fix?

Comment 2

7 years ago
That looks right to me, old code looks like a cut and paste error.  Do you have a test case to verify the fix?
(Reporter)

Comment 3

7 years ago
(In reply to Edwin Smith from comment #2)
> That looks right to me, old code looks like a cut and paste error.  Do you
> have a test case to verify the fix?

I found this while trying to understand how nanojit patches jumps. I'm not even sure how to generate 64bit jumps.

Updated

7 years ago
Group: mozilla-confidential, mozilla-corporation-confidential
(Reporter)

Comment 4

7 years ago
Edwin, I OOM trying to generate a 64bit jump on my Linux VM using the shell test from the fuzzer in bug 624439. Any suggestions on how to verify this fix?

I also asked Dave to mark this bug confidential since it probably can be used to perform heap sprays -- if the user doesn't OOM first.

I imagine this really needs to be marked a Security-Sensitive Core Bug, so if anyone can re-mark it as that, please do.
(Reporter)

Updated

7 years ago
Attachment #553508 - Flags: review?(edwsmith)
Group: mozilla-confidential, mozilla-corporation-confidential → core-security
(Reporter)

Updated

7 years ago
Attachment #553508 - Flags: review?(edwsmith)
(Reporter)

Comment 5

7 years ago
I jumped the gun on this.

Many thanks to Edwin for his help on this: 64bit jumps in nanojit x64 are encoded using RIP-relative addressing, so the first 4 bytes after FF 25 are the offset, which is kept as 0, so patch+6 is correct.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → INVALID
It still might make sense to add a comment about it in the code if it is not obvious to understand :) That way, other people looking at this will not run into this mistake again.
(Reporter)

Comment 7

7 years ago
Created attachment 553869 [details] [diff] [review]
Clarification comment
Attachment #553508 - Attachment is obsolete: true

Comment 8

7 years ago
Comment on attachment 553869 [details] [diff] [review]
Clarification comment

I can push this to nanojit-central if you need someone to do it.
Attachment #553869 - Flags: review+
(Reporter)

Comment 9

7 years ago
(In reply to Edwin Smith from comment #8)
> Comment on attachment 553869 [details] [diff] [review]
> Clarification comment
> 
> I can push this to nanojit-central if you need someone to do it.

Please do! Thanks.

Updated

7 years ago
Summary: X64 branch patch code seems to be wrong for jmp 64bit → X64 branch patch code seems to be wrong for jmp 64bit, but is actually fine: comment needed.
Group: core-security
(Assignee)

Updated

5 years ago
Component: Nanojit → Nanojit
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.