[PATCH] hw/vfio/pci-quirks: Sanitize capability pointer

[PATCH] hw/vfio/pci-quirks: Sanitize capability pointer

[Alex Williamson]

Coverity reports a tained scalar when traversing the capabilities
chain (CID 1516589).  In practice I've never seen a device with a
chain so broken as to cause an issue, but it's also pretty easy to
sanitize.

Fixes: f6b30c1984f7 ("hw/vfio/pci-quirks: Support alternate offset for GPUDirect Cliques")
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/vfio/pci-quirks.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index 0ed2fcd53152..f4ff83680572 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1530,6 +1530,12 @@ const PropertyInfo qdev_prop_nv_gpudirect_clique = {
     .set = set_nv_gpudirect_clique_id,
 };
 
+static bool is_valid_std_cap_offset(uint8_t pos)
+{
+    return (pos >= PCI_STD_HEADER_SIZEOF &&
+            pos <= (PCI_CFG_SPACE_SIZE - PCI_CAP_SIZEOF));
+}
+
 static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
 {
     PCIDevice *pdev = &vdev->pdev;
@@ -1563,7 +1569,7 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
      */
     ret = pread(vdev->vbasedev.fd, &tmp, 1,
                 vdev->config_offset + PCI_CAPABILITY_LIST);
-    if (ret != 1 || !tmp) {
+    if (ret != 1 || !is_valid_std_cap_offset(tmp)) {
         error_setg(errp, "NVIDIA GPUDirect Clique ID: error getting cap list");
         return -EINVAL;
     }
@@ -1575,7 +1581,7 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
             d4_conflict = true;
         }
         tmp = pdev->config[tmp + PCI_CAP_LIST_NEXT];
-    } while (tmp);
+    } while (is_valid_std_cap_offset(tmp));
 
     if (!c8_conflict) {
         pos = 0xC8;
-- 
2.40.1

Re: [PATCH] hw/vfio/pci-quirks: Sanitize capability pointer

[Cédric Le Goater]

On 7/1/23 00:36, Alex Williamson wrote:
> Coverity reports a tained scalar when traversing the capabilities
> chain (CID 1516589).  In practice I've never seen a device with a
> chain so broken as to cause an issue, but it's also pretty easy to
> sanitize.
> 
> Fixes: f6b30c1984f7 ("hw/vfio/pci-quirks: Support alternate offset for GPUDirect Cliques")
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Nice. I was just looking at the coverity report ! You beat me at it.

is_valid_std_cap_offset() could be reused in other places where QEMU
scans the PCI capabilities I think.

Thanks,

C.

> ---
>   hw/vfio/pci-quirks.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> index 0ed2fcd53152..f4ff83680572 100644
> --- a/hw/vfio/pci-quirks.c
> +++ b/hw/vfio/pci-quirks.c
> @@ -1530,6 +1530,12 @@ const PropertyInfo qdev_prop_nv_gpudirect_clique = {
>       .set = set_nv_gpudirect_clique_id,
>   };
>   
> +static bool is_valid_std_cap_offset(uint8_t pos)
> +{
> +    return (pos >= PCI_STD_HEADER_SIZEOF &&
> +            pos <= (PCI_CFG_SPACE_SIZE - PCI_CAP_SIZEOF));
> +}
> +
>   static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
>   {
>       PCIDevice *pdev = &vdev->pdev;
> @@ -1563,7 +1569,7 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
>        */
>       ret = pread(vdev->vbasedev.fd, &tmp, 1,
>                   vdev->config_offset + PCI_CAPABILITY_LIST);
> -    if (ret != 1 || !tmp) {
> +    if (ret != 1 || !is_valid_std_cap_offset(tmp)) {
>           error_setg(errp, "NVIDIA GPUDirect Clique ID: error getting cap list");
>           return -EINVAL;
>       }
> @@ -1575,7 +1581,7 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
>               d4_conflict = true;
>           }
>           tmp = pdev->config[tmp + PCI_CAP_LIST_NEXT];
> -    } while (tmp);
> +    } while (is_valid_std_cap_offset(tmp));
>   
>       if (!c8_conflict) {
>           pos = 0xC8;