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 <kwankhede@xxxxxxxxxx> 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
>>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
