From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45154) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bm3QS-0003oZ-QS for qemu-devel@nongnu.org; Mon, 19 Sep 2016 14:36:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bm3QO-0000f6-6h for qemu-devel@nongnu.org; Mon, 19 Sep 2016 14:36:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43734) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bm3QO-0000ef-0U for qemu-devel@nongnu.org; Mon, 19 Sep 2016 14:36:16 -0400 Date: Mon, 19 Sep 2016 12:36:12 -0600 From: Alex Williamson Message-ID: <20160919123612.61b81ece@t450s.home> In-Reply-To: <1a5e06cb-35bf-e7e3-0c0c-1ebf0fa95dcd@nvidia.com> References: <1472097235-6332-1-git-send-email-kwankhede@nvidia.com> <1472097235-6332-3-git-send-email-kwankhede@nvidia.com> <20160825172226.2cd06d71@oc7835276234> <72e3baa7-70d1-c5dd-6cd7-3874e7ea4c01@nvidia.com> <1a5e06cb-35bf-e7e3-0c0c-1ebf0fa95dcd@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 2/4] vfio: VFIO driver for mediated devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kirti Wankhede Cc: Dong Jia , kevin.tian@intel.com, cjia@nvidia.com, kvm@vger.kernel.org, qemu-devel@nongnu.org, jike.song@intel.com, kraxel@redhat.com, pbonzini@redhat.com On Mon, 19 Sep 2016 23:52:36 +0530 Kirti Wankhede 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