diff mbox series

[v2] bus/pci: fix VF bus error for memory access

Message ID 20200622063029.95985-1-haiyue.wang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series [v2] bus/pci: fix VF bus error for memory access |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation fail Compilation issues
ci/travis-robot success Travis build: passed

Commit Message

Wang, Haiyue June 22, 2020, 6:30 a.m. UTC 
  To fix CVE-2020-12888, the linux vfio-pci module will invalidate mmaps
and block MMIO access on disabled memory, it will send a SIGBUS to the
application:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=abafbc551fddede3e0a08dee1dcde08fc0eb8476

When the application opens the vfio PCI device, the vfio-pci module will
enable the memory bus command through PCI read/write access. According
to the PCIe specification, for VF, the 'Memory Space Enable' is always
zero:

             Table 9-13 Command Register Changes

Bit Location | PF and VF Register Differences | PF         | VF
             | From Base                      | Attributes | Attributes
-------------+--------------------------------+------------+-----------
             | Memory Space Enable - Does not |            |
             | apply to VFs. Must be hardwired|  Base      |  0b
     1       | to 0b for VFs. VF Memory Space |            |
             | is controlled by the VF MSE bit|            |
             | in the VF Control register.    |            |
-------------+--------------------------------+------------+-----------

Then the vfio-pci module initializes its own virtual PCI config space
data ('vconfig') by reading the VF's physical PCI config space, so the
'Memory Space Enable' bit in vconfig will also have 0b value. This will
make the vfio-pci find that the BAR memory is disabled, and the SIGBUS
will be triggerred if access these BARs.

So it needs to enable PCI bus memory command explicitly to avoid access
on disabled memory, which will call vfio-pci virtual PCI read/write API
to set the 'Memory Space Enable' in vconfig space to 1b.

Fixes: 33604c31354a ("vfio: refactor PCI BAR mapping")
Cc: stable@dpdk.org

Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
---
v2: Rewrite the commit log, and put the link into it even it is long.
---
 drivers/bus/pci/linux/pci_vfio.c | 37 ++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Burakov, Anatoly June 22, 2020, 8:52 a.m. UTC | #1 
On 22-Jun-20 7:30 AM, Haiyue Wang wrote:
> To fix CVE-2020-12888, the linux vfio-pci module will invalidate mmaps
> and block MMIO access on disabled memory, it will send a SIGBUS to the
> application:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=abafbc551fddede3e0a08dee1dcde08fc0eb8476
> 
> When the application opens the vfio PCI device, the vfio-pci module will
> enable the memory bus command through PCI read/write access. According
> to the PCIe specification, for VF, the 'Memory Space Enable' is always
> zero:
> 
>               Table 9-13 Command Register Changes
> 
> Bit Location | PF and VF Register Differences | PF         | VF
>               | From Base                      | Attributes | Attributes
> -------------+--------------------------------+------------+-----------
>               | Memory Space Enable - Does not |            |
>               | apply to VFs. Must be hardwired|  Base      |  0b
>       1       | to 0b for VFs. VF Memory Space |            |
>               | is controlled by the VF MSE bit|            |
>               | in the VF Control register.    |            |
> -------------+--------------------------------+------------+-----------
> 
> Then the vfio-pci module initializes its own virtual PCI config space
> data ('vconfig') by reading the VF's physical PCI config space, so the
> 'Memory Space Enable' bit in vconfig will also have 0b value. This will
> make the vfio-pci find that the BAR memory is disabled, and the SIGBUS
> will be triggerred if access these BARs.
> 
> So it needs to enable PCI bus memory command explicitly to avoid access
> on disabled memory, which will call vfio-pci virtual PCI read/write API
> to set the 'Memory Space Enable' in vconfig space to 1b.
> 
> Fixes: 33604c31354a ("vfio: refactor PCI BAR mapping")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>

The patch itself looks good, but i wonder how far back do these go, and 
do we need workarounds for older kernels. IIRC v17.11 is still a 
supported release, and its kernel support might go back all the way to 
v3.6 when VFIO was first introduced.

> ---
> v2: Rewrite the commit log, and put the link into it even it is long.
> ---
>   drivers/bus/pci/linux/pci_vfio.c | 37 ++++++++++++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index 64cd84a68..9b6e45da5 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -149,6 +149,38 @@ pci_vfio_get_msix_bar(int fd, struct pci_msix_table *msix_table)
>   	return 0;
>   }
>   
> +/* enable PCI bus memory command */
> +static int
> +pci_vfio_enable_bus_memory(int dev_fd)
> +{
> +	uint16_t cmd;
> +	int ret;
> +
> +	ret = pread64(dev_fd, &cmd, sizeof(cmd),
> +		      VFIO_GET_REGION_ADDR(VFIO_PCI_CONFIG_REGION_INDEX) +
> +		      PCI_COMMAND);
> +
> +	if (ret != sizeof(cmd)) {
> +		RTE_LOG(ERR, EAL, "Cannot read command from PCI config space!\n");
> +		return -1;
> +	}
> +
> +	if (cmd & PCI_COMMAND_MEMORY)
> +		return 0;
> +
> +	cmd |= PCI_COMMAND_MEMORY;
> +	ret = pwrite64(dev_fd, &cmd, sizeof(cmd),
> +		       VFIO_GET_REGION_ADDR(VFIO_PCI_CONFIG_REGION_INDEX) +
> +		       PCI_COMMAND);
> +
> +	if (ret != sizeof(cmd)) {
> +		RTE_LOG(ERR, EAL, "Cannot write command to PCI config space!\n");
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>   /* set PCI bus mastering */
>   static int
>   pci_vfio_set_bus_master(int dev_fd, bool op)
> @@ -427,6 +459,11 @@ pci_rte_vfio_setup_device(struct rte_pci_device *dev, int vfio_dev_fd)
>   		return -1;
>   	}
>   
> +	if (pci_vfio_enable_bus_memory(vfio_dev_fd)) {
> +		RTE_LOG(ERR, EAL, "Cannot enable bus memory command!\n");

Nitpick, but i think the word "command" is unneeded here :)

> +		return -1;
> +	}
> +
>   	/* set bus mastering for the device */
>   	if (pci_vfio_set_bus_master(vfio_dev_fd, true)) {
>   		RTE_LOG(ERR, EAL, "Cannot set up bus mastering!\n");
>
Wang, Haiyue June 22, 2020, 11:25 a.m. UTC | #2 
> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Monday, June 22, 2020 16:53
> To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: Re: [PATCH v2] bus/pci: fix VF bus error for memory access
> 
> On 22-Jun-20 7:30 AM, Haiyue Wang wrote:
> > To fix CVE-2020-12888, the linux vfio-pci module will invalidate mmaps
> > and block MMIO access on disabled memory, it will send a SIGBUS to the
> > application:
> >
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=abafbc551fddede3e0a08dee
> 1dcde08fc0eb8476
> >
> > When the application opens the vfio PCI device, the vfio-pci module will
> > enable the memory bus command through PCI read/write access. According
> > to the PCIe specification, for VF, the 'Memory Space Enable' is always
> > zero:
> >
> >               Table 9-13 Command Register Changes
> >
> > Bit Location | PF and VF Register Differences | PF         | VF
> >               | From Base                      | Attributes | Attributes
> > -------------+--------------------------------+------------+-----------
> >               | Memory Space Enable - Does not |            |
> >               | apply to VFs. Must be hardwired|  Base      |  0b
> >       1       | to 0b for VFs. VF Memory Space |            |
> >               | is controlled by the VF MSE bit|            |
> >               | in the VF Control register.    |            |
> > -------------+--------------------------------+------------+-----------
> >
> > Then the vfio-pci module initializes its own virtual PCI config space
> > data ('vconfig') by reading the VF's physical PCI config space, so the
> > 'Memory Space Enable' bit in vconfig will also have 0b value. This will
> > make the vfio-pci find that the BAR memory is disabled, and the SIGBUS
> > will be triggerred if access these BARs.
> >
> > So it needs to enable PCI bus memory command explicitly to avoid access
> > on disabled memory, which will call vfio-pci virtual PCI read/write API
> > to set the 'Memory Space Enable' in vconfig space to 1b.
> >
> > Fixes: 33604c31354a ("vfio: refactor PCI BAR mapping")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> 
> The patch itself looks good, but i wonder how far back do these go, and
> do we need workarounds for older kernels. IIRC v17.11 is still a
> supported release, and its kernel support might go back all the way to
> v3.6 when VFIO was first introduced.
> 

Seems not a workaround, since I found the VF in qemu has 1 for memory space,
please see the v3 commit log for detail. I added more investigation in it.

> > ---
> > v2: Rewrite the commit log, and put the link into it even it is long.
> > ---
> >   drivers/bus/pci/linux/pci_vfio.c | 37 ++++++++++++++++++++++++++++++++
> >   1 file changed, 37 insertions(+)
> >
> > diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> > index 64cd84a68..9b6e45da5 100644
> > --- a/drivers/bus/pci/linux/pci_vfio.c
> > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > @@ -149,6 +149,38 @@ pci_vfio_get_msix_bar(int fd, struct pci_msix_table *msix_table)
> >   	return 0;
> >   }
> >
> > +/* enable PCI bus memory command */
> > +static int
> > +pci_vfio_enable_bus_memory(int dev_fd)
> > +{
> > +	uint16_t cmd;
> > +	int ret;
> > +
> > +	ret = pread64(dev_fd, &cmd, sizeof(cmd),
> > +		      VFIO_GET_REGION_ADDR(VFIO_PCI_CONFIG_REGION_INDEX) +
> > +		      PCI_COMMAND);
> > +
> > +	if (ret != sizeof(cmd)) {
> > +		RTE_LOG(ERR, EAL, "Cannot read command from PCI config space!\n");
> > +		return -1;
> > +	}
> > +
> > +	if (cmd & PCI_COMMAND_MEMORY)
> > +		return 0;
> > +
> > +	cmd |= PCI_COMMAND_MEMORY;
> > +	ret = pwrite64(dev_fd, &cmd, sizeof(cmd),
> > +		       VFIO_GET_REGION_ADDR(VFIO_PCI_CONFIG_REGION_INDEX) +
> > +		       PCI_COMMAND);
> > +
> > +	if (ret != sizeof(cmd)) {
> > +		RTE_LOG(ERR, EAL, "Cannot write command to PCI config space!\n");
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >   /* set PCI bus mastering */
> >   static int
> >   pci_vfio_set_bus_master(int dev_fd, bool op)
> > @@ -427,6 +459,11 @@ pci_rte_vfio_setup_device(struct rte_pci_device *dev, int vfio_dev_fd)
> >   		return -1;
> >   	}
> >
> > +	if (pci_vfio_enable_bus_memory(vfio_dev_fd)) {
> > +		RTE_LOG(ERR, EAL, "Cannot enable bus memory command!\n");
> 
> Nitpick, but i think the word "command" is unneeded here :)
> 

Fixed in v3.

> > +		return -1;
> > +	}
> > +
> >   	/* set bus mastering for the device */
> >   	if (pci_vfio_set_bus_master(vfio_dev_fd, true)) {
> >   		RTE_LOG(ERR, EAL, "Cannot set up bus mastering!\n");
> >
> 
> 
> --
> Thanks,
> Anatoly
diff mbox series

Patch

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 64cd84a68..9b6e45da5 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -149,6 +149,38 @@  pci_vfio_get_msix_bar(int fd, struct pci_msix_table *msix_table)
 	return 0;
 }
 
+/* enable PCI bus memory command */
+static int
+pci_vfio_enable_bus_memory(int dev_fd)
+{
+	uint16_t cmd;
+	int ret;
+
+	ret = pread64(dev_fd, &cmd, sizeof(cmd),
+		      VFIO_GET_REGION_ADDR(VFIO_PCI_CONFIG_REGION_INDEX) +
+		      PCI_COMMAND);
+
+	if (ret != sizeof(cmd)) {
+		RTE_LOG(ERR, EAL, "Cannot read command from PCI config space!\n");
+		return -1;
+	}
+
+	if (cmd & PCI_COMMAND_MEMORY)
+		return 0;
+
+	cmd |= PCI_COMMAND_MEMORY;
+	ret = pwrite64(dev_fd, &cmd, sizeof(cmd),
+		       VFIO_GET_REGION_ADDR(VFIO_PCI_CONFIG_REGION_INDEX) +
+		       PCI_COMMAND);
+
+	if (ret != sizeof(cmd)) {
+		RTE_LOG(ERR, EAL, "Cannot write command to PCI config space!\n");
+		return -1;
+	}
+
+	return 0;
+}
+
 /* set PCI bus mastering */
 static int
 pci_vfio_set_bus_master(int dev_fd, bool op)
@@ -427,6 +459,11 @@  pci_rte_vfio_setup_device(struct rte_pci_device *dev, int vfio_dev_fd)
 		return -1;
 	}
 
+	if (pci_vfio_enable_bus_memory(vfio_dev_fd)) {
+		RTE_LOG(ERR, EAL, "Cannot enable bus memory command!\n");
+		return -1;
+	}
+
 	/* set bus mastering for the device */
 	if (pci_vfio_set_bus_master(vfio_dev_fd, true)) {
 		RTE_LOG(ERR, EAL, "Cannot set up bus mastering!\n");