Re: [PATCH kernel] vfio_pci: Make extended capabilities test more robust

[Alex Williamson]

On Thu, 28 Apr 2016 21:04:52 +1000
Alexey Kardashevskiy  wrote:

> VFIO reads a dword beyond the standard PCI config space (256 bytes) to
> know if there are extended config space (4096 bytes). It relies on
> the platform to return zero if there is no extended space.
> 
> However at least on PPC64/POWERNV platform, the system firmware (OPAL)
> returns 0x in this case. VFIO treats it as a proof that there is
> extended config space and calls vfio_ecap_init() which fails to parse
> capabilities (which is expected) but right before the exit, it writes
> zero at offset of 256 which is beyond the buffer allocated for
> vdev->vconfig - it is 256 bytes for a device without extended config
> space.
> 
> This adds an additional check that config space read returned non-zero
> and non- value.
> 
> Signed-off-by: Alexey Kardashevskiy 
> ---
>  drivers/vfio/pci/vfio_pci_config.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_config.c 
> b/drivers/vfio/pci/vfio_pci_config.c
> index 142c533..8a53421 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -1140,7 +1140,7 @@ static int vfio_cap_len(struct vfio_pci_device *vdev, 
> u8 cap, u8 pos)
>   case PCI_CAP_ID_EXP:
>   /* Test for extended capabilities */
>   pci_read_config_dword(pdev, PCI_CFG_SPACE_SIZE, );
> - vdev->extended_caps = (dword != 0);
> + vdev->extended_caps = (dword != 0) && (dword != 0x);
>  
>   /* length based on version */
>   if ((pcie_caps_reg(pdev) & PCI_EXP_FLAGS_VERS) == 1)

If the device is already telling us that cfg_size is less, why use
heuristics to figure out what the return value might be, just put the
whole thing in a 'if (pdev->cfg_size > PCI_CFG_SPACE_SIZE)' test.
Looks like the same should be done for PCI-X.  Thanks,

Alex

Re: [PATCH kernel] vfio_pci: Make extended capabilities test more robust

[Alex Williamson]

On Thu, 28 Apr 2016 21:04:52 +1000
Alexey Kardashevskiy  wrote:

> VFIO reads a dword beyond the standard PCI config space (256 bytes) to
> know if there are extended config space (4096 bytes). It relies on
> the platform to return zero if there is no extended space.
> 
> However at least on PPC64/POWERNV platform, the system firmware (OPAL)
> returns 0x in this case. VFIO treats it as a proof that there is
> extended config space and calls vfio_ecap_init() which fails to parse
> capabilities (which is expected) but right before the exit, it writes
> zero at offset of 256 which is beyond the buffer allocated for
> vdev->vconfig - it is 256 bytes for a device without extended config
> space.
> 
> This adds an additional check that config space read returned non-zero
> and non- value.
> 
> Signed-off-by: Alexey Kardashevskiy 
> ---
>  drivers/vfio/pci/vfio_pci_config.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_config.c 
> b/drivers/vfio/pci/vfio_pci_config.c
> index 142c533..8a53421 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -1140,7 +1140,7 @@ static int vfio_cap_len(struct vfio_pci_device *vdev, 
> u8 cap, u8 pos)
>   case PCI_CAP_ID_EXP:
>   /* Test for extended capabilities */
>   pci_read_config_dword(pdev, PCI_CFG_SPACE_SIZE, );
> - vdev->extended_caps = (dword != 0);
> + vdev->extended_caps = (dword != 0) && (dword != 0x);
>  
>   /* length based on version */
>   if ((pcie_caps_reg(pdev) & PCI_EXP_FLAGS_VERS) == 1)

If the device is already telling us that cfg_size is less, why use
heuristics to figure out what the return value might be, just put the
whole thing in a 'if (pdev->cfg_size > PCI_CFG_SPACE_SIZE)' test.
Looks like the same should be done for PCI-X.  Thanks,

Alex

[PATCH kernel] vfio_pci: Make extended capabilities test more robust

[Alexey Kardashevskiy]

VFIO reads a dword beyond the standard PCI config space (256 bytes) to
know if there are extended config space (4096 bytes). It relies on
the platform to return zero if there is no extended space.

However at least on PPC64/POWERNV platform, the system firmware (OPAL)
returns 0x in this case. VFIO treats it as a proof that there is
extended config space and calls vfio_ecap_init() which fails to parse
capabilities (which is expected) but right before the exit, it writes
zero at offset of 256 which is beyond the buffer allocated for
vdev->vconfig - it is 256 bytes for a device without extended config
space.

This adds an additional check that config space read returned non-zero
and non- value.

Signed-off-by: Alexey Kardashevskiy 
---
 drivers/vfio/pci/vfio_pci_config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vfio/pci/vfio_pci_config.c 
b/drivers/vfio/pci/vfio_pci_config.c
index 142c533..8a53421 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -1140,7 +1140,7 @@ static int vfio_cap_len(struct vfio_pci_device *vdev, u8 
cap, u8 pos)
case PCI_CAP_ID_EXP:
/* Test for extended capabilities */
pci_read_config_dword(pdev, PCI_CFG_SPACE_SIZE, );
-   vdev->extended_caps = (dword != 0);
+   vdev->extended_caps = (dword != 0) && (dword != 0x);
 
/* length based on version */
if ((pcie_caps_reg(pdev) & PCI_EXP_FLAGS_VERS) == 1)
-- 
2.5.0.rc3

[PATCH kernel] vfio_pci: Make extended capabilities test more robust

[Alexey Kardashevskiy]

VFIO reads a dword beyond the standard PCI config space (256 bytes) to
know if there are extended config space (4096 bytes). It relies on
the platform to return zero if there is no extended space.

However at least on PPC64/POWERNV platform, the system firmware (OPAL)
returns 0x in this case. VFIO treats it as a proof that there is
extended config space and calls vfio_ecap_init() which fails to parse
capabilities (which is expected) but right before the exit, it writes
zero at offset of 256 which is beyond the buffer allocated for
vdev->vconfig - it is 256 bytes for a device without extended config
space.

This adds an additional check that config space read returned non-zero
and non- value.

Signed-off-by: Alexey Kardashevskiy 
---
 drivers/vfio/pci/vfio_pci_config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vfio/pci/vfio_pci_config.c 
b/drivers/vfio/pci/vfio_pci_config.c
index 142c533..8a53421 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -1140,7 +1140,7 @@ static int vfio_cap_len(struct vfio_pci_device *vdev, u8 
cap, u8 pos)
case PCI_CAP_ID_EXP:
/* Test for extended capabilities */
pci_read_config_dword(pdev, PCI_CFG_SPACE_SIZE, );
-   vdev->extended_caps = (dword != 0);
+   vdev->extended_caps = (dword != 0) && (dword != 0x);
 
/* length based on version */
if ((pcie_caps_reg(pdev) & PCI_EXP_FLAGS_VERS) == 1)
-- 
2.5.0.rc3