linux-kernel - Re: [PATCH] vfio/pci: Some buggy virtual functions incorrectly report 1 for intx.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180809134417.50de7fe7@t450s.home>
Date:   Thu, 9 Aug 2018 13:44:17 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Ashok Raj <ashok.raj@...el.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        iommu@...ts.linux-foundation.org, Joerg Roedel <joro@...tes.org>,
        Bjorn Helgaas <helgaas@...nel.org>,
        Gage Eads <gage.eads@...el.com>
Subject: Re: [PATCH] vfio/pci: Some buggy virtual functions incorrectly
 report 1 for intx.

On Thu,  9 Aug 2018 12:37:06 -0700
Ashok Raj <ashok.raj@...el.com> wrote:

> PCI_INTERRUPT_PIN should always read  0 for SRIOV Virtual Functions.
> 
> Some SRIOV devices have some bugs in RTL and VF's end up reading 1
> instead of 0 for the PIN.

Hi Ashok,

One question, can we identify which VFs are known to have this issue so
that users (and downstreams) can know how to prioritize this patch?
Thanks,

Alex

> Since this is a spec required value, rather than having a device specific
> quirk, we could fix it permanently in vfio.
> 
> Reworked suggestions from Alex https://lkml.org/lkml/2018/7/16/1052
> 
> Reported-by: Gage Eads <gage.eads@...el.com>
> Tested-by: Gage Eads <gage.eads@...el.com>
> Signed-off-by: Ashok Raj <ashok.raj@...el.com>
> Cc: kvm@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Cc: iommu@...ts.linux-foundation.org
> Cc: Joerg Roedel <joro@...tes.org>
> Cc: Bjorn Helgaas <helgaas@...nel.org>
> Cc: Gage Eads <gage.eads@...el.com>
> ---
>  drivers/vfio/pci/vfio_pci.c        | 12 +++++++++---
>  drivers/vfio/pci/vfio_pci_config.c |  3 ++-
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index b423a30..32943dd 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -433,10 +433,16 @@ static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_type)
>  {
>  	if (irq_type == VFIO_PCI_INTX_IRQ_INDEX) {
>  		u8 pin;
> -		pci_read_config_byte(vdev->pdev, PCI_INTERRUPT_PIN, &pin);
> -		if (IS_ENABLED(CONFIG_VFIO_PCI_INTX) && !vdev->nointx && pin)
> -			return 1;
> +		/*
> +		 * INTx must be 0 for all VF's. Enforce that for all
> +		 * VF's since this is a spec requirement.
> +		 */
> +		if (!IS_ENABLED(CONFIG_VFIO_PCI_INTX) || vdev->nointx ||
> +			vdev->pdev->is_virtfn)
> +			return 0;
>  
> +		pci_read_config_byte(vdev->pdev, PCI_INTERRUPT_PIN, &pin);
> +		return (pin ? 1 : 0);
>  	} else if (irq_type == VFIO_PCI_MSI_IRQ_INDEX) {
>  		u8 pos;
>  		u16 flags;
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 115a36f..e36b7c3 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -1676,7 +1676,8 @@ int vfio_config_init(struct vfio_pci_device *vdev)
>  		*(__le16 *)&vconfig[PCI_DEVICE_ID] = cpu_to_le16(pdev->device);
>  	}
>  
> -	if (!IS_ENABLED(CONFIG_VFIO_PCI_INTX) || vdev->nointx)
> +	if (!IS_ENABLED(CONFIG_VFIO_PCI_INTX) || vdev->nointx ||
> +		pdev->is_virtfn)
>  		vconfig[PCI_INTERRUPT_PIN] = 0;
>  
>  	ret = vfio_cap_init(vdev);