On Thu, May 14, 2020 at 12:17 AM Michael Ellerman <m...@ellerman.id.au> wrote:
>
> Jordan Niethe <jniet...@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
