linux-kernel - Re: [PATCH v2] vfio/pci: update igd matching conditions

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250103104427.55f1c73b.alex.williamson@redhat.com>
Date: Fri, 3 Jan 2025 10:44:27 -0700
From: Alex Williamson <alex.williamson@...hat.com>
To: Tomita Moeko <tomitamoeko@...il.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] vfio/pci: update igd matching conditions

On Tue, 31 Dec 2024 00:10:54 +0800
Tomita Moeko <tomitamoeko@...il.com> wrote:

> igd device can either expose as a VGA controller or display controller
> depending on whether it is configured as the primary display device in
> BIOS. In both cases, the OpRegion may be present. Also checks if the
> device is at bdf 00:02.0 to avoid setting up igd-specific regions on
> Intel discrete GPUs.
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@...il.com>
> ---
> Changelog:
> v2:
> Fix misuse of pci_get_domain_bus_and_slot(), now only compares bdf
> without touching device reference count.
> Link: https://lore.kernel.org/all/20241229155140.7434-1-tomitamoeko@gmail.com/
> 
>  drivers/vfio/pci/vfio_pci.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index e727941f589d..906a1db46d15 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -111,9 +111,11 @@ static int vfio_pci_open_device(struct vfio_device *core_vdev)
>  	if (ret)
>  		return ret;
>  
> -	if (vfio_pci_is_vga(pdev) &&
> -	    pdev->vendor == PCI_VENDOR_ID_INTEL &&
> -	    IS_ENABLED(CONFIG_VFIO_PCI_IGD)) {
> +	if (IS_ENABLED(CONFIG_VFIO_PCI_IGD) &&
> +	    (pdev->vendor == PCI_VENDOR_ID_INTEL) &&
> +	    (((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) ||
> +	     ((pdev->class >> 8) == PCI_CLASS_DISPLAY_OTHER)) &&
> +	    (pci_dev_id(pdev) == PCI_DEVID(0, PCI_DEVFN(2, 0)))) {

Sorry I wasn't available to reply on previous thread before v2 was
posted, but given that we have vfio_pci_is_vga() we should use it
rather than duplicate the contents.  I think that suggests we should
create a similar helper for display_other.  Alternatively we should
maybe consider if it's sufficient to use just the base class.

The DEVID of course does not include the domain, which make it a rather
suspect check already.  What do the discrete cards report at 0xfc in
config space?  If it's zero or -1 or points to something that we can't
memremap() or points to contents that doesn't include the opregion
signature, then we'll already exit out of vfio_pci_igd_init().  Is
there actually a case that we're actually configuring IGD specific
regions for a discrete card?  Thanks,

Alex

>  		ret = vfio_pci_igd_init(vdev);
>  		if (ret && ret != -ENODEV) {
>  			pci_warn(pdev, "Failed to setup Intel IGD regions\n");