RE: [RFC v4 5/8] hisi_acc_vfio_pci: Restrict access to VF dev BAR2 migration region
From: Shameerali Kolothum Thodi
Date: Thu Feb 10 2022 - 10:01:59 EST
> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
> Sent: 09 February 2022 21:42
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-crypto@xxxxxxxxxxxxxxx; jgg@xxxxxxxxxx; cohuck@xxxxxxxxxx;
> mgurtovoy@xxxxxxxxxx; yishaih@xxxxxxxxxx; Linuxarm
> <linuxarm@xxxxxxxxxx>; liulongfang <liulongfang@xxxxxxxxxx>; Zengtao (B)
> <prime.zeng@xxxxxxxxxxxxx>; Jonathan Cameron
> <jonathan.cameron@xxxxxxxxxx>; Wangzhou (B) <wangzhou1@xxxxxxxxxxxxx>
> Subject: Re: [RFC v4 5/8] hisi_acc_vfio_pci: Restrict access to VF dev BAR2
> migration region
>
> On Tue, 8 Feb 2022 13:34:22 +0000
> Shameer Kolothum <shameerali.kolothum.thodi@xxxxxxxxxx> wrote:
>
> > HiSilicon ACC VF device BAR2 region consists of both functional
> > register space and migration control register space. From a
> > security point of view, it's not advisable to export the migration
> > control region to Guest.
> >
> > Hence, override the ioctl/read/write/mmap methods to hide the
> > migration region and limit the access only to the functional register
> > space.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@xxxxxxxxxx>
> > ---
> > drivers/vfio/pci/hisi_acc_vfio_pci.c | 122 ++++++++++++++++++++++++++-
> > 1 file changed, 118 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/vfio/pci/hisi_acc_vfio_pci.c
> b/drivers/vfio/pci/hisi_acc_vfio_pci.c
> > index 8b59e628110e..563ed2cc861f 100644
> > --- a/drivers/vfio/pci/hisi_acc_vfio_pci.c
> > +++ b/drivers/vfio/pci/hisi_acc_vfio_pci.c
> > @@ -13,6 +13,120 @@
> > #include <linux/vfio.h>
> > #include <linux/vfio_pci_core.h>
> >
> > +static int hisi_acc_pci_rw_access_check(struct vfio_device *core_vdev,
> > + size_t count, loff_t *ppos,
> > + size_t *new_count)
> > +{
> > + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> > + struct vfio_pci_core_device *vdev =
> > + container_of(core_vdev, struct vfio_pci_core_device, vdev);
> > +
> > + if (index == VFIO_PCI_BAR2_REGION_INDEX) {
> > + loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> > + resource_size_t end = pci_resource_len(vdev->pdev, index) / 2;
>
> Be careful here, there are nested assignment use cases. This can only
> survive one level of assignment before we've restricted more than we
> intended. If migration support is dependent on PF access, can we use
> that to determine when to when to expose only half the BAR and when to
> expose the full BAR?
Ok. I will add a check here.
> We should also follow the mlx5 lead to use a vendor sub-directory below
> drivers/vfio/pci/
Sure.
Thanks,
Shameer
>
> Alex
>
> > +
> > + /* Check if access is for migration control region */
> > + if (pos >= end)
> > + return -EINVAL;
> > +
> > + *new_count = min(count, (size_t)(end - pos));
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int hisi_acc_vfio_pci_mmap(struct vfio_device *core_vdev,
> > + struct vm_area_struct *vma)
> > +{
> > + struct vfio_pci_core_device *vdev =
> > + container_of(core_vdev, struct vfio_pci_core_device, vdev);
> > + unsigned int index;
> > +
> > + index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT);
> > + if (index == VFIO_PCI_BAR2_REGION_INDEX) {
> > + u64 req_len, pgoff, req_start;
> > + resource_size_t end = pci_resource_len(vdev->pdev, index) / 2;
> > +
> > + req_len = vma->vm_end - vma->vm_start;
> > + pgoff = vma->vm_pgoff &
> > + ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
> > + req_start = pgoff << PAGE_SHIFT;
> > +
> > + if (req_start + req_len > end)
> > + return -EINVAL;
> > + }
> > +
> > + return vfio_pci_core_mmap(core_vdev, vma);
> > +}
> > +
> > +static ssize_t hisi_acc_vfio_pci_write(struct vfio_device *core_vdev,
> > + const char __user *buf, size_t count,
> > + loff_t *ppos)
> > +{
> > + size_t new_count = count;
> > + int ret;
> > +
> > + ret = hisi_acc_pci_rw_access_check(core_vdev, count, ppos,
> &new_count);
> > + if (ret)
> > + return ret;
> > +
> > + return vfio_pci_core_write(core_vdev, buf, new_count, ppos);
> > +}
> > +
> > +static ssize_t hisi_acc_vfio_pci_read(struct vfio_device *core_vdev,
> > + char __user *buf, size_t count,
> > + loff_t *ppos)
> > +{
> > + size_t new_count = count;
> > + int ret;
> > +
> > + ret = hisi_acc_pci_rw_access_check(core_vdev, count, ppos,
> &new_count);
> > + if (ret)
> > + return ret;
> > +
> > + return vfio_pci_core_read(core_vdev, buf, new_count, ppos);
> > +}
> > +
> > +static long hisi_acc_vfio_pci_ioctl(struct vfio_device *core_vdev, unsigned
> int cmd,
> > + unsigned long arg)
> > +{
> > + struct vfio_pci_core_device *vdev =
> > + container_of(core_vdev, struct vfio_pci_core_device, vdev);
> > +
> > + if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
> > + struct pci_dev *pdev = vdev->pdev;
> > + struct vfio_region_info info;
> > + unsigned long minsz;
> > +
> > + minsz = offsetofend(struct vfio_region_info, offset);
> > +
> > + if (copy_from_user(&info, (void __user *)arg, minsz))
> > + return -EFAULT;
> > +
> > + if (info.argsz < minsz)
> > + return -EINVAL;
> > +
> > + if (info.index == VFIO_PCI_BAR2_REGION_INDEX) {
> > + info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
> > +
> > + /*
> > + * ACC VF dev BAR2 region consists of both functional
> > + * register space and migration control register space.
> > + * Report only the functional region to Guest.
> > + */
> > + info.size = pci_resource_len(pdev, info.index) / 2;
> > +
> > + info.flags = VFIO_REGION_INFO_FLAG_READ |
> > + VFIO_REGION_INFO_FLAG_WRITE |
> > + VFIO_REGION_INFO_FLAG_MMAP;
> > +
> > + return copy_to_user((void __user *)arg, &info, minsz) ?
> > + -EFAULT : 0;
> > + }
> > + }
> > + return vfio_pci_core_ioctl(core_vdev, cmd, arg);
> > +}
> > +
> > static int hisi_acc_vfio_pci_open_device(struct vfio_device *core_vdev)
> > {
> > struct vfio_pci_core_device *vdev =
> > @@ -32,10 +146,10 @@ static const struct vfio_device_ops
> hisi_acc_vfio_pci_ops = {
> > .name = "hisi-acc-vfio-pci",
> > .open_device = hisi_acc_vfio_pci_open_device,
> > .close_device = vfio_pci_core_close_device,
> > - .ioctl = vfio_pci_core_ioctl,
> > - .read = vfio_pci_core_read,
> > - .write = vfio_pci_core_write,
> > - .mmap = vfio_pci_core_mmap,
> > + .ioctl = hisi_acc_vfio_pci_ioctl,
> > + .read = hisi_acc_vfio_pci_read,
> > + .write = hisi_acc_vfio_pci_write,
> > + .mmap = hisi_acc_vfio_pci_mmap,
> > .request = vfio_pci_core_request,
> > .match = vfio_pci_core_match,
> > };