Re: x86_64 RIP-relative addressing bug

[Roland McGrath <roland at redhat dot com>]

> "The person who says it cannot be done should not interrupt the person
> who is doing it." -- Chinese proverb :-}

In fact, I had no intention of doing it myself before I got weighed down
the discussion that was leading away from you guys just doing it.

> I'm looking at this.  So far, the only thing that looks wrong is that
> you assume that every instruction that uses RIP-relative addressing
> includes an SIB byte.  Of all the RIP instructions I've looked at, none
> has included an SIB byte.  

Actually, it assumes that they never include a SIB byte; the comment in my
code is incorrect.  I certainly won't claim to be positive that this
detection code is 100% bulletproof.  I expected it to be a little more work
to be sure whether or not there was a SIB byte and hadn't stared hard at
all the bits in the book yet to convince myself one way or another, when
the test harness told me that my function was already good enough.

> This should make your address-of-displacement off by one (usually,
> perhaps always).  I haven't run a kernel with this patch yet, so I can't
> verify that it's actually broken.

As I said, I did myself test the patch on an instruction where fouling up
the displacement would have caused an immediate and obvious problem.  Also
note that the test harness extracts the displacement and compares it to
what objdump said, so you can be sure this is correct for any kernel given
binary just using riptest.sh before you even try the kernel patch.

> Also, in arch_copy_probe() (or before you call it), I'd prefer that you
> verify (e.g., using long arithmetic) that the adjusted displacement does
> indeed fit in 4 bytes.

The calculation itself is done in 64 bits so that sign extension happens
correctly.  It's not possible for the adjusted displacement to overflow 32
bits if the original displacement yielded an effective address within the
text area, because the pages of copied instructions are located inside the
2GB region starting at the text area's base.  If you are concerned, I can
add a check that the high bits of the calculation are unused (except by
sign extension):

		s64 disp = (u8 *) p->addr + *ripdisp - (u8 *) p->ainsn.insn;
		BUG_ON((s64) (s32) disp != disp);
		*ripdisp = disp;

> One last thing to remember when we add support for probing user-mode
> programs: If the app was built with -m32, then what looks like a REX
> prefix (40-4f) could actually be an inc or dec opcode.  (There are
> undoubtedly other issues with non-64-bit modes, but that's the only one
> I know that's relevant here.)

Most issues at this level have to be reconsidered anew for the context of
user-mode probe insertion, but we aren't there yet.  This entire approach
for %rip-relative instructions cannot be used for user-mode at all, because
you do not have any control over the address space and there may not be any
location available for copied instructions that is within 2GB of the data
they access.  We only get away with it in kernel mode because the address
space layout is already tightly constrained with specific intent that all
the kernel and module code fit where %rip-relative addressing can be used
(otherwise module loading relocating module code that uses %rip-relative
addressing mode wouldn't work right).


Thanks,
Roland