[RFC PATCH 30/30] vfio: Allow to bind foreign task

[Jean-Philippe Brucker]

Hi Tomasz,

Thanks for looking at this.

On 26/04/17 08:25, Tomasz Nowicki wrote:
> On 27.02.2017 20:54, Jean-Philippe Brucker wrote:
>> Let the process that owns the device create an address space bond on
>> behalf of another process. We add a pid argument to the BIND_TASK ioctl,
>> allowing the caller to bind a foreign task. The expected program flow in
>> this case is:
>>
>> * Process A creates the VFIO context and initializes the device.
>> * Process B asks A to bind its address space.
>> * Process A issues an ioctl to the VFIO device fd with BIND_TASK(pid).
>>   It may communicate the given PASID back to process B or keep track of it
>>   internally.
>> * Process B asks A to perform transactions on its virtual address.
>> * Process A launches transaction tagged with the given PASID.
>>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker at arm.com>
>> ---
>>  drivers/vfio/vfio.c       | 35 +++++++++++++++++++++++++++++++++--
>>  include/uapi/linux/vfio.h | 15 +++++++++++++++
>>  2 files changed, 48 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>> index c4505d8f4c61..ecc5d07e3dbb 100644
>> --- a/drivers/vfio/vfio.c
>> +++ b/drivers/vfio/vfio.c
>> @@ -26,6 +26,7 @@
>>  #include <linux/module.h>
>>  #include <linux/mutex.h>
>>  #include <linux/pci.h>
>> +#include <linux/ptrace.h>
>>  #include <linux/rwsem.h>
>>  #include <linux/sched.h>
>>  #include <linux/slab.h>
>> @@ -1660,7 +1661,7 @@ static long vfio_svm_ioctl(struct vfio_device
>> *device, unsigned int cmd,
>>      struct vfio_device_svm svm;
>>      struct vfio_task *vfio_task;
>>
>> -    minsz = offsetofend(struct vfio_device_svm, pasid);
>> +    minsz = offsetofend(struct vfio_device_svm, pid);
>>
>>      if (copy_from_user(&svm, (void __user *)arg, minsz))
>>          return -EFAULT;
>> @@ -1669,9 +1670,39 @@ static long vfio_svm_ioctl(struct vfio_device
>> *device, unsigned int cmd,
>>          return -EINVAL;
>>
>>      if (cmd == VFIO_DEVICE_BIND_TASK) {
>> -        struct task_struct *task = current;
>> +        struct mm_struct *mm;
>> +        struct task_struct *task;
>> +
>> +        if (svm.flags & ~VFIO_SVM_PID)
>> +            return -EINVAL;
>> +
>> +        if (svm.flags & VFIO_SVM_PID) {
>> +            rcu_read_lock();
>> +            task = find_task_by_vpid(svm.pid);
>> +            if (task)
>> +                get_task_struct(task);
>> +            rcu_read_unlock();
>> +            if (!task)
>> +                return -ESRCH;
>> +
>> +            /*
>> +             * Ensure process has RW access on the task's mm
>> +             * FIXME:
>> +             * - I think this ought to be in the IOMMU API
>> +             * - I'm assuming permission is never revoked during the
>> +             *   task's lifetime. Might be mistaken.
>> +             */
>> +            mm = mm_access(task, PTRACE_MODE_ATTACH_REALCREDS);
>> +            if (!mm || IS_ERR(mm))
> 
> I know this is RFC patch but considering we will keep this as is, we need
> here:
> +put_task_struct(task);

Indeed. I considerably reworked the VFIO patches for next version, but
this bug was still in there.

Thanks,
Jean-Philippe