Re: [Qemu-devel] [PATCH v7 2/4] vfio: VFIO driver for mediated devices

[Jike Song]

On 08/26/2016 10:13 PM, Kirti Wankhede wrote:
> 
> 
> On 8/25/2016 2:52 PM, Dong Jia wrote:
>> On Thu, 25 Aug 2016 09:23:53 +0530
>> Kirti Wankhede <address@hidden> wrote:
>>
>> [...]
>>
>> Dear Kirti,
>>
>> I just rebased my vfio-ccw patches to this series.
>> With a little fix, which was pointed it out in my reply to the #3
>> patch, it works fine.
>>
> 
> Thanks for update. Glad to know this works for you.
> 
> 
>>> +static long vfio_mdev_unlocked_ioctl(void *device_data,
>>> +                                unsigned int cmd, unsigned long arg)
>>> +{
>>> +   int ret = 0;
>>> +   struct vfio_mdev *vmdev = device_data;
>>> +   struct parent_device *parent = vmdev->mdev->parent;
>>> +   unsigned long minsz;
>>> +
>>> +   switch (cmd) {
>>> +   case VFIO_DEVICE_GET_INFO:
>>> +   {
>>> +           struct vfio_device_info info;
>>> +
>>> +           minsz = offsetofend(struct vfio_device_info, num_irqs);
>>> +
>>> +           if (copy_from_user(&info, (void __user *)arg, minsz))
>>> +                   return -EFAULT;
>>> +
>>> +           if (info.argsz < minsz)
>>> +                   return -EINVAL;
>>> +
>>> +           if (parent->ops->get_device_info)
>>> +                   ret = parent->ops->get_device_info(vmdev->mdev, &info);
>>> +           else
>>> +                   return -EINVAL;
>>> +
>>> +           if (ret)
>>> +                   return ret;
>>> +
>>> +           if (parent->ops->reset)
>>> +                   info.flags |= VFIO_DEVICE_FLAGS_RESET;
>> Shouldn't this be done inside the get_device_info callback?
>>
> 
> I would like Vendor driver to set device type only. Reset flag should be
> set on basis of reset() callback provided.
> 
>>> +
>>> +           memcpy(&vmdev->dev_info, &info, sizeof(info));
>>> +
>>> +           return copy_to_user((void __user *)arg, &info, minsz);
>>> +   }
>> [...]
>>
>>> +
>>> +static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
>>> +                         size_t count, loff_t *ppos)
>>> +{
>>> +   struct vfio_mdev *vmdev = device_data;
>>> +   struct mdev_device *mdev = vmdev->mdev;
>>> +   struct parent_device *parent = mdev->parent;
>>> +   unsigned int done = 0;
>>> +   int ret;
>>> +
>>> +   if (!parent->ops->read)
>>> +           return -EINVAL;
>>> +
>>> +   while (count) {
>> Here, I have to say sorry to you guys for that I didn't notice the
>> bad impact of this change to my patches during the v6 discussion.
>>
>> For vfio-ccw, I introduced an I/O region to input/output I/O
>> instruction parameters and results for Qemu. The @count of these data
>> currently is 140. So supporting arbitrary lengths in one shot here, and
>> also in vfio_mdev_write, seems the better option for this case.
>>
>> I believe that if the pci drivers want to iterate in a 4 bytes step, you
>> can do that in the parent read/write callbacks instead.
>>
>> What do you think?
>>
> 
> I would like to know Alex's thought on this. He raised concern with this
> approach in v6 reviews:
> "But I think this is exploitable, it lets the user make the kernel
> allocate an arbitrarily sized buffer."

It is impossible to check count here, because one simply doesn't have the
knowledge of this region.

VFIO_DEVICE_GET_REGION_INFO was implemented in vfio-mdev.ko, while decoding
the vfio_mdev_read to a particular MMIO region was expected to be implemented
in vendor driver, that results in unbalanced interfaces.


To have balanced interfaces, you either:

        - call ioctl instead of GET_REGION_INFO
        - call read instead of decoding REGION

or:

        - call GET_REGION_INFO instead of ioctl
        - decode REGION in read, and check its validity, call region-specific
          read function


V6 was the latter, v7 is kind of a mixture of these two, while I believe
the former will completely address such problem :)


--
Thanks,
Jike


>>> +           size_t filled;
>>> +
>>> +           if (count >= 4 && !(*ppos % 4)) {
>>> +                   u32 val;
>>> +
>>> +                   ret = parent->ops->read(mdev, (char *)&val, sizeof(val),
>>> +                                           *ppos);
>>> +                   if (ret <= 0)
>>> +                           goto read_err;
>>> +
>>> +                   if (copy_to_user(buf, &val, sizeof(val)))
>>> +                           goto read_err;
>>> +
>>> +                   filled = 4;
>>> +           } else if (count >= 2 && !(*ppos % 2)) {
>>> +                   u16 val;
>>> +
>>> +                   ret = parent->ops->read(mdev, (char *)&val, sizeof(val),
>>> +                                           *ppos);
>>> +                   if (ret <= 0)
>>> +                           goto read_err;
>>> +
>>> +                   if (copy_to_user(buf, &val, sizeof(val)))
>>> +                           goto read_err;
>>> +
>>> +                   filled = 2;
>>> +           } else {
>>> +                   u8 val;
>>> +
>>> +                   ret = parent->ops->read(mdev, &val, sizeof(val), *ppos);
>>> +                   if (ret <= 0)
>>> +                           goto read_err;
>>> +
>>> +                   if (copy_to_user(buf, &val, sizeof(val)))
>>> +                           goto read_err;
>>> +
>>> +                   filled = 1;
>>> +           }
>>> +
>>> +           count -= filled;
>>> +           done += filled;
>>> +           *ppos += filled;
>>> +           buf += filled;
>>> +   }
>>> +
>>> +   return done;
>>> +
>>> +read_err:
>>> +   return -EFAULT;
>>> +}
>> [...]
>>
>> --------
>> Dong Jia
>>