diff mbox series

[RFC,v2,2/4] hisi_acc_vfio_pci: Override ioctl method to limit BAR2 region size

Message ID 20210702095849.1610-3-shameerali.kolothum.thodi@huawei.com
State New
Headers show
Series vfio/hisilicon: add acc live migration driver | expand

Commit Message

Shameerali Kolothum Thodi July 2, 2021, 9:58 a.m. UTC 
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, hide the migration region and report only the functional register
space.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

---
 drivers/vfio/pci/hisi_acc_vfio_pci.c | 42 +++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

-- 
2.17.1

Comments

Alex Williamson July 2, 2021, 8:29 p.m. UTC | #1 
On Fri, 2 Jul 2021 10:58:47 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> 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, hide the migration region and report only the functional register

> space.

> 

> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

> ---

>  drivers/vfio/pci/hisi_acc_vfio_pci.c | 42 +++++++++++++++++++++++++++-

>  1 file changed, 41 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/vfio/pci/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisi_acc_vfio_pci.c

> index a9e173098ab5..d57cf6d7adaf 100644

> --- a/drivers/vfio/pci/hisi_acc_vfio_pci.c

> +++ b/drivers/vfio/pci/hisi_acc_vfio_pci.c

> @@ -12,6 +12,46 @@

>  #include <linux/vfio.h>

>  #include <linux/vfio_pci_core.h>

>  

> +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(64K) consists of both functional

> +			 * register space and migration control register space.

> +			 * Report only the first 32K(functional region) to Guest.

> +			 */

> +			info.size = pci_resource_len(pdev, info.index) / 2;

> +


Great, but what actually prevents the user from accessing the full
extent of the BAR since you're re-using core code for read/write/mmap,
which are all basing access on pci_resource_len()?  Thanks,

Alex

> +			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(struct vfio_device *core_vdev)

>  {

>  	struct vfio_pci_core_device *vdev =

> @@ -33,7 +73,7 @@ static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {

>  	.name		= "hisi-acc-vfio-pci",

>  	.open		= hisi_acc_vfio_pci_open,

>  	.release	= vfio_pci_core_release,

> -	.ioctl		= vfio_pci_core_ioctl,

> +	.ioctl		= hisi_acc_vfio_pci_ioctl,

>  	.read		= vfio_pci_core_read,

>  	.write		= vfio_pci_core_write,

>  	.mmap		= vfio_pci_core_mmap,
Shameerali Kolothum Thodi July 5, 2021, 7:22 a.m. UTC | #2 
> -----Original Message-----

> From: Alex Williamson [mailto:alex.williamson@redhat.com]

> Sent: 02 July 2021 21:30

> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>

> Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org;

> linux-crypto@vger.kernel.org; jgg@nvidia.com; mgurtovoy@nvidia.com;

> Linuxarm <linuxarm@huawei.com>; liulongfang <liulongfang@huawei.com>;

> Zengtao (B) <prime.zeng@hisilicon.com>; yuzenghui

> <yuzenghui@huawei.com>; Jonathan Cameron

> <jonathan.cameron@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>

> Subject: Re: [RFC v2 2/4] hisi_acc_vfio_pci: Override ioctl method to limit

> BAR2 region size

> 

> On Fri, 2 Jul 2021 10:58:47 +0100

> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> 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, hide the migration region and report only the functional register

> > space.

> >

> > Signed-off-by: Shameer Kolothum

> <shameerali.kolothum.thodi@huawei.com>

> > ---

> >  drivers/vfio/pci/hisi_acc_vfio_pci.c | 42 +++++++++++++++++++++++++++-

> >  1 file changed, 41 insertions(+), 1 deletion(-)

> >

> > diff --git a/drivers/vfio/pci/hisi_acc_vfio_pci.c

> b/drivers/vfio/pci/hisi_acc_vfio_pci.c

> > index a9e173098ab5..d57cf6d7adaf 100644

> > --- a/drivers/vfio/pci/hisi_acc_vfio_pci.c

> > +++ b/drivers/vfio/pci/hisi_acc_vfio_pci.c

> > @@ -12,6 +12,46 @@

> >  #include <linux/vfio.h>

> >  #include <linux/vfio_pci_core.h>

> >

> > +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(64K) consists of both functional

> > +			 * register space and migration control register space.

> > +			 * Report only the first 32K(functional region) to Guest.

> > +			 */

> > +			info.size = pci_resource_len(pdev, info.index) / 2;

> > +

> 

> Great, but what actually prevents the user from accessing the full

> extent of the BAR since you're re-using core code for read/write/mmap,

> which are all basing access on pci_resource_len()?  Thanks,


Ah..true. Missed that. I will add overrides for read/write/mmap and limit
the access.

Thanks,
Shameer

> 

> Alex

> 

> > +			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(struct vfio_device *core_vdev)

> >  {

> >  	struct vfio_pci_core_device *vdev =

> > @@ -33,7 +73,7 @@ static const struct vfio_device_ops

> hisi_acc_vfio_pci_ops = {

> >  	.name		= "hisi-acc-vfio-pci",

> >  	.open		= hisi_acc_vfio_pci_open,

> >  	.release	= vfio_pci_core_release,

> > -	.ioctl		= vfio_pci_core_ioctl,

> > +	.ioctl		= hisi_acc_vfio_pci_ioctl,

> >  	.read		= vfio_pci_core_read,

> >  	.write		= vfio_pci_core_write,

> >  	.mmap		= vfio_pci_core_mmap,
diff mbox series

Patch

diff --git a/drivers/vfio/pci/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisi_acc_vfio_pci.c
index a9e173098ab5..d57cf6d7adaf 100644
--- a/drivers/vfio/pci/hisi_acc_vfio_pci.c
+++ b/drivers/vfio/pci/hisi_acc_vfio_pci.c
@@ -12,6 +12,46 @@ 
 #include <linux/vfio.h>
 #include <linux/vfio_pci_core.h>
 
+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(64K) consists of both functional
+			 * register space and migration control register space.
+			 * Report only the first 32K(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(struct vfio_device *core_vdev)
 {
 	struct vfio_pci_core_device *vdev =
@@ -33,7 +73,7 @@  static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {
 	.name		= "hisi-acc-vfio-pci",
 	.open		= hisi_acc_vfio_pci_open,
 	.release	= vfio_pci_core_release,
-	.ioctl		= vfio_pci_core_ioctl,
+	.ioctl		= hisi_acc_vfio_pci_ioctl,
 	.read		= vfio_pci_core_read,
 	.write		= vfio_pci_core_write,
 	.mmap		= vfio_pci_core_mmap,