linux-kernel - Re: [PATCH V2 3/7] vhost-vdpa: VHOST_NEW

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <acfccf72-2883-407d-977a-9c2566cf8cb3@oracle.com>
Date: Mon, 15 Jul 2024 10:27:31 -0400
From: Steven Sistare <steven.sistare@...cle.com>
To: Jason Wang <jasowang@...hat.com>
Cc: virtualization@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Si-Wei Liu <si-wei.liu@...cle.com>,
        Eugenio Perez Martin <eperezma@...hat.com>,
        Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
        Dragos Tatulea <dtatulea@...dia.com>
Subject: Re: [PATCH V2 3/7] vhost-vdpa: VHOST_NEW_OWNER

On 7/14/2024 10:26 PM, Jason Wang wrote:
> On Fri, Jul 12, 2024 at 9:19 PM Steve Sistare <steven.sistare@...cle.com> wrote:
>>
>> Add an ioctl to transfer file descriptor ownership and pinned memory
>> accounting from one process to another.
>>
>> This is more efficient than VHOST_RESET_OWNER followed by VHOST_SET_OWNER,
>> as that would unpin all physical pages, requiring them to be repinned in
>> the new process.  That would cost multiple seconds for large memories, and
>> be incurred during a virtual machine's pause time during live update.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@...cle.com>
>> ---
>>   drivers/vhost/vdpa.c       | 41 ++++++++++++++++++++++++++++++++++++++
>>   drivers/vhost/vhost.c      | 15 ++++++++++++++
>>   drivers/vhost/vhost.h      |  1 +
>>   include/uapi/linux/vhost.h | 10 ++++++++++
>>   4 files changed, 67 insertions(+)
>>
>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>> index b49e5831b3f0..5cf55ca4ec02 100644
>> --- a/drivers/vhost/vdpa.c
>> +++ b/drivers/vhost/vdpa.c
>> @@ -632,6 +632,44 @@ static long vhost_vdpa_resume(struct vhost_vdpa *v)
>>          return ret;
>>   }
>>
>> +static long vhost_vdpa_new_owner(struct vhost_vdpa *v)
>> +{
>> +       int r;
>> +       struct vhost_dev *vdev = &v->vdev;
>> +       struct mm_struct *mm_old = vdev->mm;
>> +       struct mm_struct *mm_new = current->mm;
>> +       long pinned_vm = v->pinned_vm;
>> +       unsigned long lock_limit = PFN_DOWN(rlimit(RLIMIT_MEMLOCK));
>> +
>> +       if (!mm_old)
>> +               return -EINVAL;
>> +       mmgrab(mm_old);
>> +
>> +       if (!v->vdpa->use_va &&
>> +           pinned_vm + atomic64_read(&mm_new->pinned_vm) > lock_limit) {
>> +               r = -ENOMEM;
>> +               goto out;
>> +       }
> 
> So this seems to allow an arbitrary process to execute this. Seems to be unsafe.
> 
> I wonder if we need to add some checks here, maybe PID or other stuff
> to only allow the owner process to do this.

The original owner must send the file descriptor to the new owner.
That constitutes permission to take ownership.

>> +       r = vhost_vdpa_bind_mm(v, mm_new);
>> +       if (r)
>> +               goto out;
>> +
>> +       r = vhost_dev_new_owner(vdev);
>> +       if (r) {
>> +               vhost_vdpa_bind_mm(v, mm_old);
>> +               goto out;
>> +       }
>> +
>> +       if (!v->vdpa->use_va) {
>> +               atomic64_sub(pinned_vm, &mm_old->pinned_vm);
>> +               atomic64_add(pinned_vm, &mm_new->pinned_vm);
>> +       }
>> +
>> +out:
>> +       mmdrop(mm_old);
>> +       return r;
>> +}
>> +
>>   static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
>>                                     void __user *argp)
>>   {
>> @@ -876,6 +914,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
>>          case VHOST_VDPA_RESUME:
>>                  r = vhost_vdpa_resume(v);
>>                  break;
>> +       case VHOST_NEW_OWNER:
>> +               r = vhost_vdpa_new_owner(v);
>> +               break;
>>          default:
>>                  r = vhost_dev_ioctl(&v->vdev, cmd, argp);
>>                  if (r == -ENOIOCTLCMD)
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index b60955682474..ab40ae50552f 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -963,6 +963,21 @@ long vhost_dev_set_owner(struct vhost_dev *dev)
>>   }
>>   EXPORT_SYMBOL_GPL(vhost_dev_set_owner);
>>
>> +/* Caller should have device mutex */
>> +long vhost_dev_new_owner(struct vhost_dev *dev)
>> +{
>> +       if (dev->mm == current->mm)
>> +               return -EBUSY;
>> +
>> +       if (!vhost_dev_has_owner(dev))
>> +               return -EINVAL;
>> +
>> +       vhost_detach_mm(dev);
>> +       vhost_attach_mm(dev);
> 
> This seems to do nothing unless I miss something.

vhost_detach mm drops dev->mm.
vhost_attach_mm grabs current->mm.

- Steve