Re: [PATCH v8 16/30] powerpc: Define and use __get_user_instr{, inatomic}()

From: Jordan Niethe <jniethe5@gmail.com>
To: Michael Ellerman <mpe@ellerman.id.au>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>,
	Alistair Popple <alistair@popple.id.au>,
	Nicholas Piggin <npiggin@gmail.com>,
	Balamuruhan S <bala24@linux.ibm.com>,
	naveen.n.rao@linux.vnet.ibm.com, linuxppc-dev@lists.ozlabs.org,
	Daniel Axtens <dja@axtens.net>
Subject: Re: [PATCH v8 16/30] powerpc: Define and use __get_user_instr{, inatomic}()
Date: Thu, 14 May 2020 09:54:48 +1000	[thread overview]
Message-ID: <CACzsE9od2GFXBiy5imy_dGutx7POOnCx7+k-Ynx+UMcNzyTsTw@mail.gmail.com> (raw)
In-Reply-To: <878shvc40x.fsf@mpe.ellerman.id.au>

On Thu, May 14, 2020 at 12:17 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Jordan Niethe <jniethe5@gmail.com> writes:
> > Define specific __get_user_instr() and __get_user_instr_inatomic()
> > macros for reading instructions from user space.
>
> At least for fix_alignment() we could be coming from the kernel, not
> sure about the other cases.
>
> I can tweak the change log.
>
> > diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> > index 2f500debae21..c0a35e4586a5 100644
> > --- a/arch/powerpc/include/asm/uaccess.h
> > +++ b/arch/powerpc/include/asm/uaccess.h
> > @@ -105,6 +105,11 @@ static inline int __access_ok(unsigned long addr, unsigned long size,
> >  #define __put_user_inatomic(x, ptr) \
> >       __put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
> >
> > +#define __get_user_instr(x, ptr) \
> > +     __get_user_nocheck((x).val, (u32 *)(ptr), sizeof(u32), true)
> > +
> > +#define __get_user_instr_inatomic(x, ptr) \
> > +     __get_user_nosleep((x).val, (u32 *)(ptr), sizeof(u32))
>
> I'm not super keen on adding new __ versions, which lack the access_ok()
> check, but I guess we have to.
>
> > diff --git a/arch/powerpc/kernel/vecemu.c b/arch/powerpc/kernel/vecemu.c
> > index 3dd70eeb10c5..60ed5aea8d4e 100644
> > --- a/arch/powerpc/kernel/vecemu.c
> > +++ b/arch/powerpc/kernel/vecemu.c
> > @@ -266,7 +266,7 @@ int emulate_altivec(struct pt_regs *regs)
> >       unsigned int va, vb, vc, vd;
> >       vector128 *vrs;
> >
> > -     if (get_user(instr.val, (unsigned int __user *)regs->nip))
> > +     if (__get_user_instr(instr, (void __user *)regs->nip))
> >               return -EFAULT;
>
> That drops the access_ok() check, which is not OK, at least without a
> reasonable justification.
>
> Given it's regs->nip I guess it should be safe, but it should still be
> called out. Or preferably switched to __get_user() in a precursor patch.
Or should I add a get_user_instr() that includes the check?
>
> cheers