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

Status: RESOLVED INVALID

(Core Graveyard :: Nanojit, defect)

Description

[Shu-yu Guo [:shu]]

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.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: fix?

Comment 2

[Edwin Smith]

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

Comment 3

[Shu-yu Guo [:shu]]

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

Comment 4

[Shu-yu Guo [:shu]]

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.

Comment 5

[Shu-yu Guo [:shu]]

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.

Comment 6

[Christian Holler (:decoder)]

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.

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Clarification comment

Comment 8

[Edwin Smith]

Comment on attachment 553869 [details] [diff] [review]
Clarification comment

I can push this to nanojit-central if you need someone to do it.

Comment 9

[Shu-yu Guo [:shu]]

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