VFIO driver: Non-privileged user level PCI drivers
in [Kernel]
|
From: Michael S. Tsirkin on 30 May 2010 11:00 On Sun, May 30, 2010 at 04:13:59PM +0300, Avi Kivity wrote: > On 05/30/2010 04:03 PM, Michael S. Tsirkin wrote: >> >> >>>>>> IMO this was because this driver does two things: programming iommu and >>>>>> handling interrupts. uio does interrupt handling. >>>>>> We could have moved iommu / DMA programming to >>>>>> a separate driver, and have uio work with it. >>>>>> This would solve limitation of the current driver >>>>>> that is needs an iommu domain per device. >>>>>> >>>>>> >>>>>> >>>>> How do we enforce security then? We need to ensure that unprivileged >>>>> users can only use the device with an iommu. >>>>> >>>>> >>>> Force assigning to iommu before we allow any other operation? >>>> >>>> >>> That means the driver must be aware of the iommu. >>> >> The userspace driver? Yes. And It is a good thing to be explicit >> there anyway, since this lets userspace map a non-contigious >> virtual address list into a contiguous bus address range. >> > > No, the kernel driver. It cannot allow userspace to enable bus > mastering unless it knows the iommu is enabled for the device and remaps > dma to user pages. So what I suggested is failing any kind of access until iommu is assigned. > > -- > error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Avi Kivity on 31 May 2010 08:00 On 05/30/2010 05:53 PM, Michael S. Tsirkin wrote: > > So what I suggested is failing any kind of access until iommu > is assigned. > So, the kernel driver must be aware of the iommu. In which case it may as well program it. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Alan Cox on 31 May 2010 13:20 > +/* > + * Map usr buffer at specific IO virtual address > + */ > +static int vfio_dma_map_iova( > + mlp = kzalloc(sizeof *mlp, GFP_KERNEL); Not good at that point. I think you need to allocate it first, error if it can't be allocated and then do the work and free it on error ? > + mlp = kzalloc(sizeof *mlp, GFP_KERNEL); > + mlp->pages = pages; Ditto > +int vfio_enable_msix(struct vfio_dev *vdev, int nvec, void __user *uarg) > +{ > + struct pci_dev *pdev = vdev->pdev; > + struct eventfd_ctx *ctx; > + int ret = 0; > + int i; > + int fd; > + > + vdev->msix = kzalloc(nvec * sizeof(struct msix_entry), > + GFP_KERNEL); > + vdev->ev_msix = kzalloc(nvec * sizeof(struct eventfd_ctx *), > + GFP_KERNEL); These don't seem to get freed on the error path - or indeed protected against being allocated twice (eg two parallel ioctls ?) > + case VFIO_DMA_MAP_ANYWHERE: > + case VFIO_DMA_MAP_IOVA: > + if (copy_from_user(&dm, uarg, sizeof dm)) > + return -EFAULT; > + ret = vfio_dma_map_common(listener, cmd, &dm); > + if (!ret && copy_to_user(uarg, &dm, sizeof dm)) So the vfio_dma_map is untrusted. That seems to be checked ok later but the dma_map_common code then plays in current->mm-> without apparently holding any locks to stop the values getting corrupted by a parallel mlock ? Actually no I take that back dmp->size is 64bit So npage can end up with values like 0xFFFFFFFF and cause 32bit boxes to go kerblam > + > + case VFIO_EVENTFD_IRQ: > + if (copy_from_user(&fd, uarg, sizeof fd)) > + return -EFAULT; > + if (vdev->ev_irq) > + eventfd_ctx_put(vdev->ev_irq); These paths need locking - suppose two EVENTFD irq ioctls occur at once (in general these paths seem not to be covered) > > + case VFIO_BAR_LEN: > + if (copy_from_user(&bar, uarg, sizeof bar)) > + return -EFAULT; > + if (bar < 0 || bar > PCI_ROM_RESOURCE) > + return -EINVAL; > + bar = pci_resource_len(pdev, bar); > + if (copy_to_user(uarg, &bar, sizeof bar)) > + return -EFAULT; How does this all work out if the device is a bridge ? > + pci_read_config_byte(pdev, PCI_INTERRUPT_LINE, &line); > + if (line == 0) > + goto out; That may produce some interestingly wrong answers. Firstly the platform has interrupt abstraction so dev->irq may not match PCI_INTERRUPT_LINE, secondly you have devices that report their IRQ via other paths as per spec (notably IDE class devices in non-native mode) So that would also want extra checks. > + pci_read_config_word(pdev, PCI_COMMAND, &orig); > + ret = orig & PCI_COMMAND_MASTER; > + if (!ret) { > + new = orig | PCI_COMMAND_MASTER; > + pci_write_config_word(pdev, PCI_COMMAND, new); > + pci_read_config_word(pdev, PCI_COMMAND, &new); > + ret = new & PCI_COMMAND_MASTER; > + pci_write_config_word(pdev, PCI_COMMAND, orig); The master bit on some devices can be turned on but not off. Not sure it matters here. > + vdev->pdev = pdev; Probably best to take/drop a reference. Not needed if you can prove your last use is before the end of the remove path though. Does look like it needs a locking audit, some memory and error checks reviewing and some further review of the ioctl security and overflows/trusted values. Rather a nice way of attacking the user space PCI problem. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Michael S. Tsirkin on 31 May 2010 13:20 On Mon, May 31, 2010 at 02:50:29PM +0300, Avi Kivity wrote: > On 05/30/2010 05:53 PM, Michael S. Tsirkin wrote: >> >> So what I suggested is failing any kind of access until iommu >> is assigned. >> > > So, the kernel driver must be aware of the iommu. In which case it may > as well program it. It's a kernel driver anyway. Point is that the *device* driver is better off not programming iommu, this way we do not need to reprogram it for each device. > -- > I have a truly marvellous patch that fixes the bug which this > signature is too narrow to contain. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Avi Kivity on 1 Jun 2010 04:20
On 05/31/2010 08:10 PM, Michael S. Tsirkin wrote: > On Mon, May 31, 2010 at 02:50:29PM +0300, Avi Kivity wrote: > >> On 05/30/2010 05:53 PM, Michael S. Tsirkin wrote: >> >>> So what I suggested is failing any kind of access until iommu >>> is assigned. >>> >>> >> So, the kernel driver must be aware of the iommu. In which case it may >> as well program it. >> > It's a kernel driver anyway. Point is that > the *device* driver is better off not programming iommu, > this way we do not need to reprogram it for each device. > The device driver is in userspace. It can't program the iommu. What the patch proposes is that userspace tells vfio about the needed mappings, and vfio programs the iommu. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |