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

["Cédric Le Goater" <clg@redhat.com>]

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;