Re: [PATCH v5 1/3] x86/msi: harden stale pdev handling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.10.2024 17:27, Stewart Hildebrand wrote:
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1243,7 +1243,12 @@ int pci_reset_msix_state(struct pci_dev *pdev)
>  {
>      unsigned int pos = pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_MSIX);
>  
> -    ASSERT(pos);
> +    if ( !pos )
> +    {
> +        pdev->broken = true;
> +        return -EFAULT;
> +    }
> +
>      /*
>       * Xen expects the device state to be the after reset one, and hence
>       * host_maskall = guest_maskall = false and all entries should have the
> @@ -1271,7 +1276,12 @@ int pci_msi_conf_write_intercept(struct pci_dev *pdev, 
> unsigned int reg,
>          entry = find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX);
>          pos = entry ? entry->msi_attrib.pos
>                      : pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_MSIX);
> -        ASSERT(pos);
> +
> +        if ( !pos )
> +        {
> +            pdev->broken = true;
> +            return -EFAULT;
> +        }
>  
>          if ( reg >= pos && reg < msix_pba_offset_reg(pos) + 4 )
>          {

There are more instances of pci_find_cap_offset(..., PCI_CAP_ID_MSIX)
which may want/need dealing with, even if there are no ASSERT()s there.

Setting ->broken is of course a perhaps desirable (side) effect. Nevertheless
I wonder whether latching the capability position once during device init
wouldn't be an alternative (better?) approach.

Finally I don't think -EFAULT is appropriate here. Imo it should be -ENODEV.

Jan