Re: [PATCH 2/6] arch: x86: Wrap TIF_IA32 checks

From: Christoph Hellwig <hch@lst.de>
To: Andy Lutomirski <luto@kernel.org>
Cc: Gabriel Krisman Bertazi <krisman@collabora.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Christoph Hellwig <hch@lst.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Kees Cook <keescook@chromium.org>, X86 ML <x86@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	kernel@collabora.com
Subject: Re: [PATCH 2/6] arch: x86: Wrap TIF_IA32 checks
Date: Wed, 29 Jul 2020 08:59:55 +0200	[thread overview]
Message-ID: <20200729065955.GA32309@lst.de> (raw)
In-Reply-To: <CALCETrV_HJCrDLCKLrqNbLiOsoEcC9M7zn-v_hcVMvDgnWW8yw@mail.gmail.com>

On Tue, Jul 28, 2020 at 08:43:27PM -0700, Andy Lutomirski wrote:
> > index d4edf281fff4..d39f9b3ae683 100644
> > --- a/arch/x86/include/asm/compat.h
> > +++ b/arch/x86/include/asm/compat.h
> > @@ -181,7 +181,7 @@ static inline void __user *arch_compat_alloc_user_space(long len)
> >  {
> >         compat_uptr_t sp;
> >
> > -       if (test_thread_flag(TIF_IA32)) {
> > +       if (TASK_IA32(current)) {
> >                 sp = task_pt_regs(current)->sp;
> 
> Christoph, you spend a *lot* more time looking at this stuff lately
> than I do, but this looks totally wrong.  Shouldn't this be either:
> 
> sp = task_pt_regs(current)->sp;
> 
> /* This might be a compat syscall issued via int $0x80 from 64-bit-ABI code. */
> if (user_64bit_mode(task_pt_regs(current))
>   sp -= 128;
> 
> Or perhaps the same thing without the user_64bit_mode() check at all?
> There shouldn't be much if any harm done by respecting the redzone
> unnecessarily.

compat_alloc_user_space is only used when either called from compat
calls or if in_compat_syscall() is true (and there are very few callers
left, and we plan to kill it off entirely..).

Which means we are either called from an i386 or x32 syscall, but then
again IIRC user_64bit_mode would also return true for x32.  So your
above version looks correct, and I'd also be tempted to just always
respect the redzone.

> Surely this should be:
> 
> if (user_64bit_mode(task_pt_regs(regs))

s/regs/current/

Btw, I wonder if want a shorthand for

	user_64bit_mode(task_pt_regs(thread))

instead of always open coding it.