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

From: Lai Jiangshan
Date: Thu Apr 07 2022 - 03:03:35 EST

On Thu, Apr 7, 2022 at 3:00 AM Borislav Petkov <bp@xxxxxxxxx> wrote:
> On Fri, Mar 18, 2022 at 10:30:10PM +0800, Lai Jiangshan wrote:
> > From: Lai Jiangshan <jiangshan.ljs@xxxxxxxxxxxx>
> >
> > fixup_bad_iret() and sync_regs() have similar arguments and do similar
> > work that copies full or partial pt_regs to a place and switches stack
> > after return. They are quite the same, but fixup_bad_iret() not only
> > copies the pt_regs but also the return address of error_entry() while
> What return address of error_entry()? You lost me here.

"return address" is the return address of a function which is
error_entry() here.

Or error_entry_ret in struct bad_iret_stack which is being removed
in the change.

> So fixup_bad_iret() gets the stack ptr passed in by doing:
> mov %rsp, %rdi
> call fixup_bad_iret
> mov %rax, %rsp
> and error_regs()
> movq %rsp, %rdi /* arg0 = pt_regs pointer */
> call sync_regs
> movq %rax, %rsp /* switch stack */
> the same way.

They are not the same way.

sync_regs() is called before the return address of error_entry()
popped into %r12 while fixup_bad_iret() is called with the return
address of error_entry() still on the stack. And the primitives of
fixup_bad_iret() and sync_regs() are different which also means
they are not the same way.

After this change, they become the same way.

IMO, sync_regs() is grace while fixup_bad_iret() is a bad C function
or is not a pure C function because it is handling the return address
of its parent function which is better done by the compiler or ASM

> Confused.
> > It is prepared for later patch to do the stack switch after the
> > error_entry() which simplifies the code further.
> Looking at your next patch, is all this dance done just so that you can
> do
> leaq 8(%rsp), %rdi
> in order to pass in pt_regs to both functions?
> And get rid of the saving/restoring %r12?
> Is that what the whole noise is about?

The point is to make fixup_bad_iret() a normal C function and
the same as sync_regs() in calling.

The next patch makes error_entry() as a bunch of ASM code compiled
from a C function and pave the road to really convert it to a C

Getting rid of the saving/restoring the return address in %r12
is necessary since a C function can't save/restore the return


> --
> Regards/Gruss,
> Boris.

I'm sorry for using top-posting and "This patch". I remember it.