Message-ID: <20250428150023.GA670069@bhelgaas>
Date: Mon, 28 Apr 2025 10:00:23 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Chathura Rajapaksha <chathura.abeyrathne.lk@...il.com>
Cc: kvm@...r.kernel.org, Chathura Rajapaksha <chath@...edu>,
	William Wang <xwill@...edu>,
	Alex Williamson <alex.williamson@...hat.com>,
	Paul Moore <paul@...l-moore.com>, Eric Paris <eparis@...hat.com>,
	Xin Zeng <xin.zeng@...el.com>, Yahui Cao <yahui.cao@...el.com>,
	Giovanni Cabiddu <giovanni.cabiddu@...el.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Niklas Schnelle <schnelle@...ux.ibm.com>,
	Yunxiang Li <Yunxiang.Li@....com>,
	Dongdong Zhang <zhangdongdong@...incomputing.com>,
	Kevin Tian <kevin.tian@...el.com>,
	Avihai Horon <avihaih@...dia.com>, linux-kernel@...r.kernel.org,
	audit@...r.kernel.org
Subject: Re: [RFC PATCH 1/2] block accesses to unassigned PCI config regions

Hint: observe subject line conventions for the file you're changing.
See "git log --oneline drivers/vfio/pci/vfio_pci_config.c".

On Sat, Apr 26, 2025 at 09:22:48PM +0000, Chathura Rajapaksha wrote:
> Some PCIe devices trigger PCI bus errors when accesses are made to
> unassigned regions within their PCI configuration space. On certain
> platforms, this can lead to host system hangs or reboots.

1) Define "unassigned regions".  From the patch, I infer that the
64-byte header, capabilities, and extended capabilities are considered
"assigned," and anything else would be "unassigned."

2) Can you expand on what these certain platforms are and the details
of what these errors are and how they're reported?  You mention
"PCIe," but I suppose this is not really PCIe-specific.  I suppose the
hang or reboot would be a consequence of the error being reported as
SERR# or a platform-specific System Error (PCIe r6.0, sec 6.2.6)?

In conventional PCI, we may not have control over SERR# assertion and
the effect on the system.  In PCIe, bits in the Root Control register
should control SERR# assertion.

> The current vfio-pci driver allows guests to access unassigned regions
> in the PCI configuration space. Therefore, when such a device is passed
> through to a guest, the guest can induce a host system hang or reboot
> through crafted configuration space accesses, posing a threat to
> system availability.
> 
> This patch introduces support for blocking guest accesses to unassigned
> PCI configuration space, and the ability to bypass this access control
> for specific devices. The patch introduces three module parameters:

We already know which patch this refers to (so "this patch" is
unnecessary) and typical style is to use imperative mood:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.14#n94

>    block_pci_unassigned_write:
>    Blocks write accesses to unassigned config space regions.
> 
>    block_pci_unassigned_read:
>    Blocks read accesses to unassigned config space regions.
> 
>    uaccess_allow_ids:
>    Specifies the devices for which the above access control is bypassed.
>    The value is a comma-separated list of device IDs in
>    <vendor_id>:<device_id> format.
> 
>    Example usage:
>    To block guest write accesses to unassigned config regions for all
>    passed through devices except for the device with vendor ID 0x1234 and
>    device ID 0x5678:
> 
>    block_pci_unassigned_write=1 uaccess_allow_ids=1234:5678
> 
> Co-developed by: William Wang <xwill@...edu>
> Signed-off-by: William Wang <xwill@...edu>
> Signed-off-by: Chathura Rajapaksha <chath@...edu>
> ---
>  drivers/vfio/pci/vfio_pci_config.c | 122 ++++++++++++++++++++++++++++-
>  1 file changed, 121 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 8f02f236b5b4..cb4d11aa5598 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -120,6 +120,106 @@ struct perm_bits {
>  #define	NO_WRITE	0
>  #define	ALL_WRITE	0xFFFFFFFFU
>  
> +static bool block_pci_unassigned_write;
> +module_param(block_pci_unassigned_write, bool, 0644);
> +MODULE_PARM_DESC(block_pci_unassigned_write,
> +		 "Block write accesses to unassigned PCI config regions.");
> +
> +static bool block_pci_unassigned_read;
> +module_param(block_pci_unassigned_read, bool, 0644);
> +MODULE_PARM_DESC(block_pci_unassigned_read,
> +		 "Block read accesses from unassigned PCI config regions.");
> +
> +static char *uaccess_allow_ids;
> +module_param(uaccess_allow_ids, charp, 0444);
> +MODULE_PARM_DESC(uaccess_allow_ids, "PCI IDs to allow access to unassigned PCI config regions, format is \"vendor:device\" and multiple comma separated entries can be specified");
> +
> +static LIST_HEAD(allowed_device_ids);
> +static DEFINE_SPINLOCK(device_ids_lock);
> +
> +struct uaccess_device_id {
> +	struct list_head slot_list;
> +	unsigned short vendor;
> +	unsigned short device;
> +};
> +
> +static void pci_uaccess_add_device(struct uaccess_device_id *new,
> +				   int vendor, int device)
> +{
> +	struct uaccess_device_id *pci_dev_id;
> +	unsigned long flags;
> +	int found = 0;
> +
> +	spin_lock_irqsave(&device_ids_lock, flags);
> +
> +	list_for_each_entry(pci_dev_id, &allowed_device_ids, slot_list) {
> +		if (pci_dev_id->vendor == vendor &&
> +		    pci_dev_id->device == device) {
> +			found = 1;
> +			break;
> +		}
> +	}
> +
> +	if (!found) {
> +		new->vendor = vendor;
> +		new->device = device;
> +		list_add_tail(&new->slot_list, &allowed_device_ids);
> +	}
> +
> +	spin_unlock_irqrestore(&device_ids_lock, flags);
> +
> +	if (found)
> +		kfree(new);
> +}
> +
> +static bool pci_uaccess_lookup(struct pci_dev *dev)
> +{
> +	struct uaccess_device_id *pdev_id;
> +	unsigned long flags;
> +	bool found = false;
> +
> +	spin_lock_irqsave(&device_ids_lock, flags);
> +	list_for_each_entry(pdev_id, &allowed_device_ids, slot_list) {
> +		if (pdev_id->vendor == dev->vendor &&
> +		    pdev_id->device == dev->device) {
> +			found = true;
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&device_ids_lock, flags);
> +
> +	return found;
> +}
> +
> +static int __init pci_uaccess_init(void)
> +{
> +	char *p, *id;
> +	int fields;
> +	int vendor, device;
> +	struct uaccess_device_id *pci_dev_id;
> +
> +	/* add ids specified in the module parameter */
> +	p = uaccess_allow_ids;
> +	while ((id = strsep(&p, ","))) {
> +		if (!strlen(id))
> +			continue;
> +
> +		fields = sscanf(id, "%x:%x", &vendor, &device);
> +
> +		if (fields != 2) {
> +			pr_warn("Invalid id string \"%s\"\n", id);
> +			continue;
> +		}
> +
> +		pci_dev_id = kmalloc(sizeof(*pci_dev_id), GFP_KERNEL);
> +		if (!pci_dev_id)
> +			return -ENOMEM;
> +
> +		pci_uaccess_add_device(pci_dev_id, vendor, device);
> +	}
> +	return 0;
> +}
> +
>  static int vfio_user_config_read(struct pci_dev *pdev, int offset,
>  				 __le32 *val, int count)
>  {
> @@ -335,6 +435,18 @@ static struct perm_bits unassigned_perms = {
>  	.writefn = vfio_raw_config_write
>  };
>  
> +/*
> + * Read/write access to PCI unassigned config regions can be blocked
> + * using the block_pci_unassigned_read and block_pci_unassigned_write
> + * module parameters. The uaccess_allow_ids module parameter can be used
> + * to whitelist devices (i.e., bypass the block) when either of the
> + * above parameters is specified.
> + */
> +static struct perm_bits block_unassigned_perms = {
> +	.readfn = NULL,
> +	.writefn = NULL
> +};
> +
>  static struct perm_bits virt_perms = {
>  	.readfn = vfio_virt_config_read,
>  	.writefn = vfio_virt_config_write
> @@ -1108,6 +1220,9 @@ int __init vfio_pci_init_perm_bits(void)
>  	ecap_perms[PCI_EXT_CAP_ID_VNDR].writefn = vfio_raw_config_write;
>  	ecap_perms[PCI_EXT_CAP_ID_DVSEC].writefn = vfio_raw_config_write;
>  
> +	/* Device list allowed to access unassigned PCI regions */
> +	ret |= pci_uaccess_init();
> +
>  	if (ret)
>  		vfio_pci_uninit_perm_bits();
>  
> @@ -1896,7 +2011,12 @@ static ssize_t vfio_config_do_rw(struct vfio_pci_core_device *vdev, char __user
>  	cap_id = vdev->pci_config_map[*ppos];
>  
>  	if (cap_id == PCI_CAP_ID_INVALID) {
> -		perm = &unassigned_perms;
> +		if (((iswrite && block_pci_unassigned_write) ||
> +		     (!iswrite && block_pci_unassigned_read)) &&
> +		    !pci_uaccess_lookup(pdev))
> +			perm = &block_unassigned_perms;
> +		else
> +			perm = &unassigned_perms;
>  		cap_start = *ppos;
>  	} else if (cap_id == PCI_CAP_ID_INVALID_VIRT) {
>  		perm = &virt_perms;
> -- 
> 2.34.1
> 

Powered by blists - more mailing lists