Re: [PATCH v8 2/8] KVM: Extend the memslot to support fd-based private memory

[Chao Peng]

On Mon, Sep 26, 2022 at 11:26:45AM +0100, Fuad Tabba wrote:
...

> > +
> > +- KVM_MEM_PRIVATE can be set to indicate a new slot has private memory backed by
> > +  a file descirptor(fd) and the content of the private memory is invisible to
> 
> s/descirptor/descriptor

Thanks.

...

>  static long kvm_vm_ioctl(struct file *filp,
> >                            unsigned int ioctl, unsigned long arg)
> >  {
> > @@ -4645,14 +4672,20 @@ static long kvm_vm_ioctl(struct file *filp,
> >                 break;
> >         }
> >         case KVM_SET_USER_MEMORY_REGION: {
> > -               struct kvm_userspace_memory_region kvm_userspace_mem;
> > +               struct kvm_user_mem_region mem;
> > +               unsigned long size = sizeof(struct kvm_userspace_memory_region);
> 
> nit: should this be sizeof(struct mem)? That's more similar to the
> existing code and makes it dependent on the size of mem regardless of
> possible changes to its type in the future.

Unluckily no, the size we need copy_from_user() depends on the flags,
e.g. without KVM_MEM_PRIVATE, we can't safely copy that big size since
the 'extended' part may not even exist.

> 
> > +
> > +               kvm_sanity_check_user_mem_region_alias();
> >
> >                 r = -EFAULT;
> > -               if (copy_from_user(&kvm_userspace_mem, argp,
> > -                                               sizeof(kvm_userspace_mem)))
> > +               if (copy_from_user(&mem, argp, size);
> 
> It gets fixed in a future patch, but the ; should be a ).

Good catch, thanks!

Chao
> 
> Cheers,
> /fuad