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

[Borislav Petkov]

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