Re: [PATCH 05/19] KVM: arm64: Consolidate sysreg userspace accesses

[Marc Zyngier <maz@kernel.org>]

Hi Reiji,

On Sat, 09 Jul 2022 07:55:08 +0100,
Reiji Watanabe <reijiw@google.com> wrote:
> 
> Hi Marc,

[...]

> > @@ -2854,17 +2794,28 @@ static int demux_c15_set(u64 id, void __user *uaddr)
> >  int kvm_sys_reg_get_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
> >                          const struct sys_reg_desc table[], unsigned int num)
> >  {
> > -       void __user *uaddr = (void __user *)(unsigned long)reg->addr;
> > +       u64 __user *uaddr = (u64 __user *)(unsigned long)reg->addr;
> >         const struct sys_reg_desc *r;
> > +       u64 val;
> > +       int ret;
> >
> >         r = id_to_sys_reg_desc(vcpu, reg->id, table, num);
> >         if (!r)
> >                 return -ENOENT;
> >
> > -       if (r->get_user)
> > -               return (r->get_user)(vcpu, r, reg, uaddr);
> > +       if (r->get_user) {
> > +               ret = (r->get_user)(vcpu, r, &val);
> > +       } else {
> > +               val = __vcpu_sys_reg(vcpu, r->reg);
> > +               ret = 0;
> > +       }
> > +
> > +       if (!ret) {
> > +               if (put_user(val, uaddr))
> > +                       ret = -EFAULT;
> 
> Nit: Since put_user() returns -EFAULT when it fails,
> we can simply set 'ret' to the return value from put_user().

Indeed. I've now reworked all the instances of this that survive the
incremental rework. Thanks for the suggestion.

> (same for get_user())

This one is a harder sell, as get_user() usually occurs at the
beginning of the function, without a preexisting error context.

> 
> Reviewed-by: Reiji Watanabe <reijiw@google.com>
> 
> I like this fix!

Thanks!

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel