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

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 19 Sep 2016 23:52:36 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 8/26/2016 7:43 PM, Kirti Wankhede wrote:
> > * PGP Signed: 08/26/2016 at 07:15:44 AM, Decrypted
> > On 8/25/2016 2:52 PM, Dong Jia wrote:  
> >> On Thu, 25 Aug 2016 09:23:53 +0530  
> 
> >>> +
> >>> +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."
> >   
> 
> Read/write callbacks are for slow path, emulation of mmio region which
> are mainly device registers. I do feel it shouldn't support arbitrary
> lengths.
> Alex, I would like to know your thoughts.

The exploit was that the mdev layer allocated a buffer and copied the
entire user buffer into kernel space before passing it to the vendor
driver.  The solution is to pass the __user *buf to the vendor driver
and let them sanitize and split it however makes sense for their
device.  We shouldn't be assuming naturally aligned, up to dword
accesses in the generic mdev layers.  Those sorts of semantics are
defined by the device type.  This is another case where making
the mdev layer as thin as possible is actually the best thing to
do to make it really device type agnostic.  Thanks,

Alex