Re: [PATCH] x86/asm/entry/64: better check for canonical address

[Ingo Molnar <mingo@kernel.org>]

* Denys Vlasenko <dvlasenk@redhat.com> wrote:

> On 03/27/2015 09:11 AM, Ingo Molnar wrote:
> > 
> > * Denys Vlasenko <dvlasenk@redhat.com> wrote:
> > 
> >> This change makes the check exact (no more false positives
> >> on kernel addresses).
> >>
> >> It isn't really important to be fully correct here -
> >> almost all addresses we'll ever see will be userspace ones,
> >> but OTOH it looks to be cheap enough:
> >> the new code uses two more ALU ops but preserves %rcx,
> >> allowing to not reload it from pt_regs->cx again.
> >> On disassembly level, the changes are:
> >>
> >> cmp %rcx,0x80(%rsp) -> mov 0x80(%rsp),%r11; cmp %rcx,%r11
> >> shr $0x2f,%rcx      -> shl $0x10,%rcx; sar $0x10,%rcx; cmp %rcx,%r11
> >> mov 0x58(%rsp),%rcx -> (eliminated)
> >>
> >> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> >> CC: Borislav Petkov <bp@alien8.de>
> >> CC: x86@kernel.org
> >> CC: linux-kernel@vger.kernel.org
> >> ---
> >>
> >> Andy, I'd undecided myself on the merits of doing this.
> >> If you like it, feel free to take it in your tree.
> >> I trimmed CC list to not bother too many people with this trivial
> >> and quite possibly "useless churn"-class change.
> >>
> >>  arch/x86/kernel/entry_64.S | 23 ++++++++++++-----------
> >>  1 file changed, 12 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
> >> index bf9afad..a36d04d 100644
> >> --- a/arch/x86/kernel/entry_64.S
> >> +++ b/arch/x86/kernel/entry_64.S
> >> @@ -688,26 +688,27 @@ retint_swapgs:		/* return to user-space */
> >>  	 * a completely clean 64-bit userspace context.
> >>  	 */
> >>  	movq RCX(%rsp),%rcx
> >> -	cmpq %rcx,RIP(%rsp)		/* RCX == RIP */
> >> +	movq RIP(%rsp),%r11
> >> +	cmpq %rcx,%r11			/* RCX == RIP */
> >>  	jne opportunistic_sysret_failed
> > 
> > Btw., in the normal syscall entry path, RIP(%rsp) == RCX(%rsp), 
> > because we set up pt_regs like that - and at this point RIP/RCX is 
> > guaranteed to be canonical, right?
> > 
> > So if there's a mismatch generated, it's the kernel's doing.
> 
> This is an optimization on IRET exit code path.
> 
> We go here if we know that pt_regs can be modified by .e.g. ptrace.
>
> I think we also go here even on interrupt return.

Yeah, missed that, this would kill any flag based approach.

> (Granted, chances that RCX was the same as RIP at the moment of 
> interrupt are slim, but we still would check that and (ab)use SYSRET 
> if it looks like it'll work).

Btw., there's a neat trick we could do: in the HLT, MWAIT and 
ACPI-idle code we could attempt to set up RCX to match RIP, to trigger 
this optimization in the common 'irq interrupted the idle task' case?

Thanks,

	Ingo