Re: [PATCH 1/2] PCI: Allow drivers to request exclusive config regions

[Greg Kroah-Hartman]

On Sun, Aug 21, 2022 at 08:52:36PM -0400, ira.weiny@xxxxxxxxx wrote:
> From: Ira Weiny <ira.weiny@xxxxxxxxx>
> 
> PCI config space access from user space has traditionally been
> unrestricted with writes being an understood risk for device operation.
> 
> Unfortunately, device breakage or odd behavior from config writes lacks
> indicators that can leave driver writers confused when evaluating
> failures.  This is especially true with the new PCIe Data Object
> Exchange (DOE) mailbox protocol where backdoor shenanigans from user
> space through things such as vendor defined protocols may affect device
> operation without complete breakage.

What userspace tools are out there messing with PCI config space through
userspace on these devices today?  How is this the kernel's fault if
someone runs such a thing?

> A prior proposal restricted read and writes completely.[1]  Greg and
> Bjorn pointed out that proposal is flawed for a couple of reasons.
> First, lspci should always be allowed and should not interfere with any
> device operation.  Second, setpci is a valuable tool that is sometimes
> necessary and it should not be completely restricted.[2]  Finally
> methods exist for full lock of device access if required.
> 
> Even though access should not be restricted it would be nice for driver
> writers to be able to flag critical parts of the config space such that
> interference from user space can be detected.
> 
> Introduce pci_request_config_region_exclusive() to mark exclusive config
> regions.  Such regions trigger a warning and kernel taint if accessed
> via user space.
> 
> [1] https://lore.kernel.org/all/161663543465.1867664.5674061943008380442.stgit@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> [2] https://lore.kernel.org/all/YF8NGeGv9vYcMfTV@xxxxxxxxx/
> 
> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Suggested-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> 
> ---
> Changes from[1]:
> 	Change name to pci_request_config_region_exclusive()
> 	Don't flag reads at all.
> 	Allow writes with a warn and taint of the kernel.
> 	Update commit message
> 	Forward port to latest tree.
> ---
>  drivers/pci/pci-sysfs.c |  6 ++++++
>  drivers/pci/probe.c     |  6 ++++++
>  include/linux/ioport.h  |  2 ++
>  include/linux/pci.h     | 16 ++++++++++++++++
>  kernel/resource.c       | 13 ++++++++-----
>  5 files changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index fc804e08e3cb..de41d761bdf5 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -755,6 +755,12 @@ static ssize_t pci_write_config(struct file *filp, struct kobject *kobj,
>  	if (ret)
>  		return ret;
>  
> +	if (resource_is_exclusive(&dev->config_resource, off,
> +				  count)) {
> +		pci_warn(dev, "Write to restricted range %llx detected", off);

Will this allow any user to spam the kernel log from userspace?  You
might want to rate-limit it, right?

> +		add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> +	}
> +
>  	if (off > dev->cfg_size)
>  		return 0;
>  	if (off + count > dev->cfg_size) {
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 6280e780a48c..d81d7457058b 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2303,6 +2303,12 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
>  	INIT_LIST_HEAD(&dev->bus_list);
>  	dev->dev.type = &pci_dev_type;
>  	dev->bus = pci_bus_get(bus);
> +	dev->config_resource = (struct resource) {
> +		.name = "PCI Config",
> +		.start = 0,
> +		.end = -1,
> +	};
> +
>  #ifdef CONFIG_PCI_MSI
>  	raw_spin_lock_init(&dev->msi_lock);
>  #endif
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 616b683563a9..cf1de55d14da 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -312,6 +312,8 @@ extern void __devm_release_region(struct device *dev, struct resource *parent,
>  				  resource_size_t start, resource_size_t n);
>  extern int iomem_map_sanity_check(resource_size_t addr, unsigned long size);
>  extern bool iomem_is_exclusive(u64 addr);
> +extern bool resource_is_exclusive(struct resource *resource, u64 addr,
> +				  resource_size_t size);
>  
>  extern int
>  walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 81a57b498f22..dde37bfa0ca5 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -409,6 +409,7 @@ struct pci_dev {
>  	 */
>  	unsigned int	irq;
>  	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
> +	struct resource config_resource;		 /* driver exclusive config ranges */

So the driver only gets 1 range to mark this way?

What are the ranges for typical devices that have this problem?

This still feels very odd to me, how far do we have to go to protect
userspace from doing bad things on systems when they have the
permissions and access to do those bad things?

What are you trying to protect yourself from, bogus bug reports by
people doing bad things and then blaming you?  That's easy to handle,
just ignore them :)

thanks,

greg k-h