Re: [PATCH V4 1/7] x86/traps: Move pt_regs only in fixup_bad_iret()

From: Borislav Petkov
Date: Thu Apr 07 2022 - 09:19:24 EST


On Thu, Apr 07, 2022 at 10:22:25AM +0200, Borislav Petkov wrote:
> Maybe there was a reason it was done this way:

Ok, I went and singlestepped this code so that I can see what's going
on.

The second memcpy in fixup_bad_iret() copies the remainder of pt_regs
from the current stack. The result in tmp looks like this:

(gdb) p/x tmp
$10 = {error_entry_ret = 0xffffffff81a00998, regs = {r15 = 0x0, r14 = 0x0, r13 = 0x7fffffffea30, r12 = 0x40002b, bp = 0x40,
bx = 0xa, r11 = 0x246, r10 = 0x8, r9 = 0x7fffffffe860, r8 = 0x0, ax = 0x0, cx = 0x0, dx = 0x0, si = 0x7fffffffe860,
di = 0x2, orig_ax = 0x30, ip = 0x403000, cs = 0x33, flags = 0x246, sp = 0x8badf00d5aadc0de, ss = 0x33}}

note error_entry_ret which is:

(gdb) x/10i 0xffffffff81a00998
0xffffffff81a00998 <asm_exc_general_protection+8>: mov %rsp,%rdi
0xffffffff81a0099b <asm_exc_general_protection+11>: mov 0x78(%rsp),%rsi
0xffffffff81a009a0 <asm_exc_general_protection+16>: movq $0xffffffffffffffff,0x78(%rsp)
0xffffffff81a009a9 <asm_exc_general_protection+25>: call 0xffffffff818c8960 <exc_general_protection>
0xffffffff81a009ae <asm_exc_general_protection+30>: jmp 0xffffffff81a01030 <error_return>
0xffffffff81a009b3: data16 nopw %cs:0x0(%rax,%rax,1)

i.e., the return address to the #GP handler that has been pushed on
the stack when the IRET fault has happened and former has called
error_entry().

fixup_bad_iret() then ends up returning this in new_stack:

(gdb) p/x *new_stack
$12 = {error_entry_ret = 0xffffffff81a00998, regs = {r15 = 0x0, r14 = 0x0, r13 = 0x7fffffffea30, r12 = 0x40002b, bp = 0x40,
bx = 0xa, r11 = 0x246, r10 = 0x8, r9 = 0x7fffffffe860, r8 = 0x0, ax = 0x0, cx = 0x0, dx = 0x0, si = 0x7fffffffe860,
di = 0x2, orig_ax = 0x30, ip = 0x403000, cs = 0x33, flags = 0x246, sp = 0x8badf00d5aadc0de, ss = 0x33}}

and when error_entry() does:

mov %rax, %rsp

The stack has:

=> 0xffffffff81a0102d <error_entry+173>: jmp 0xffffffff81a00fd2 <error_entry+82>
0xfffffe0000250f50: 0xffffffff81a00998 0x0000000000000000
0xfffffe0000250f60: 0x0000000000000000 0x00007fffffffea30
0xfffffe0000250f70: 0x000000000040002b 0x0000000000000040
0xfffffe0000250f80: 0x000000000000000a 0x0000000000000246
0xfffffe0000250f90: 0x0000000000000008 0x00007fffffffe860

and you can recognize new_stack there.

Then it does:

jmp error_entry_from_usermode_after_swapgs

where it does:

error_entry_from_usermode_after_swapgs:
/* Put us onto the real thread stack. */
popq %r12 /* save return addr in %12 */
movq %rsp, %rdi /* arg0 = pt_regs pointer */
call sync_regs
movq %rax, %rsp /* switch stack */
ENCODE_FRAME_POINTER
pushq %r12
RET

and in here it uses %r12 to stash the return address 0xffffffff81a00998
while sync_regs() runs.

So yeah, all your patch does is get rid of void *error_entry_ret in

struct bad_iret_stack {
void *error_entry_ret;
struct pt_regs regs;
};

So your commit message should have been as simple as:

"Always stash the address error_entry() is going to return to, in %r12
and get rid of the void *error_entry_ret; slot in struct bad_iret_stack
which was supposed to account for it and pt_regs pushed on the stack.

After this, both functions can work on a struct pt_regs pointer
directly."

In any case, I don't see why amluto would do this so this looks like a
sensible cleanup to do.

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette