Re: [PATCH net-next v3 02/14] libie: add PCI device initialization helpers to libie

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Integer overflow in MMIO region bounds check allows out-of-bounds physical memory mapping.
- [High] Use of unmanaged `ioremap()` in PCI API forces manual cleanup, leading to Use-After-Unmap crashes in driver teardown.
- [Medium] Missing bounds check for MMIO access size in `__libie_pci_get_mmio_addr` allows out-of-bounds memory accesses.
- [Low] Missing `depends on PCI` in `CONFIG_LIBIE_PCI` causes build failures.
--

commit 20e33f0ecbedce390794d8ede7c6cdd2f15cacf9
Author: Phani R Burra <phani.r.burra@intel.com>
Subject: libie: add PCI device initialization helpers to libie

This commit introduces a new PCI library under libie to help network
drivers map and manage non-linear MMIO regions and perform standard
PCI initialization.

> diff --git a/drivers/net/ethernet/intel/libie/Kconfig b/drivers/net/ethernet/intel/libie/Kconfig
> index 70831c7e336e9..500a95c944a8c 100644
> --- a/drivers/net/ethernet/intel/libie/Kconfig
> +++ b/drivers/net/ethernet/intel/libie/Kconfig
> @@ -23,3 +23,9 @@ config LIBIE_FWLOG
>  	  for it. Firmware logging is using admin queue interface to communicate
>  	  with the device. Debugfs is a user interface used to config logging
>  	  and dump all collected logs.
> +
> +config LIBIE_PCI
> +	tristate
> +	help
> +	  Helper functions for management of PCI resources belonging
> +	  to networking devices.

Will this build successfully if CONFIG_PCI is disabled and a user manually
enables CONFIG_LIBIE_PCI=y?

While some PCI core APIs have stubs for when PCI is disabled,
pcim_request_region does not appear to be stubbed in include/linux/pci.h.

Should this config depend on PCI to prevent linker errors?

> diff --git a/drivers/net/ethernet/intel/libie/pci.c b/drivers/net/ethernet/intel/libie/pci.c
> new file mode 100644
> index 0000000000000..7276a3533b54d
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/libie/pci.c
> @@ -0,0 +1,208 @@

[ ... ]

> +void __iomem *__libie_pci_get_mmio_addr(struct libie_mmio_info *mmio_info,
> +					resource_size_t offset,
> +					int num_args, ...)
> +{
> +	struct libie_pci_mmio_region *mr;
> +	int bar_idx = 0;
> +	va_list args;
> +
> +	if (num_args) {
> +		va_start(args, num_args);
> +		bar_idx = va_arg(args, int);
> +		va_end(args);
> +	}
> +
> +	list_for_each_entry(mr, &mmio_info->mmio_list, list)
> +		if (bar_idx == mr->bar_idx && offset >= mr->offset &&
> +		    offset < mr->offset + mr->size) {

Does this verification prevent partial out-of-bounds accesses?

If a caller requests an offset at the very end of the region, such as
mr->offset + mr->size - 1, the check passes. If the driver then performs a
32-bit or 64-bit MMIO read, it would read past the mapped boundary.

Should the function take an expected access size parameter to verify the
entire access falls within the region?

> +			offset -= mr->offset;
> +
> +			return mr->addr + offset;
> +		}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_NS_GPL(__libie_pci_get_mmio_addr, "LIBIE_PCI");
> +
> +bool __libie_pci_map_mmio_region(struct libie_mmio_info *mmio_info,
> +				 resource_size_t offset,
> +				 resource_size_t size, int num_args, ...)
> +{
> +	struct pci_dev *pdev = mmio_info->pdev;
> +	struct libie_pci_mmio_region *mr;
> +	resource_size_t pa;
> +	void __iomem *va;
> +	int bar_idx = 0;
> +	va_list args;
> +
> +	if (num_args) {
> +		va_start(args, num_args);
> +		bar_idx = va_arg(args, int);
> +		va_end(args);
> +	}
> +
> +	if (offset + size > pci_resource_len(pdev, bar_idx))
> +		return false;

Could this addition overflow if a compromised firmware or control plane
provides very large values for offset and size?

If offset + size wraps around, the result could evaluate to a small value,
bypassing the length check. This might allow mapping arbitrary physical
memory via ioremap.

Would it be safer to check this as:
if (size > pci_resource_len(...) || offset > pci_resource_len(...) - size)

> +
> +	mr = libie_find_mmio_region(&mmio_info->mmio_list, offset, size,
> +				    bar_idx);
> +	if (mr) {
> +		pci_warn(pdev,
> +			 "Mapping of BAR%u (offset=%llu, size=%llu) intersecting region (offset=%llu, size=%llu) already exists\n",
> +			 bar_idx, (unsigned long long)mr->offset,
> +			 (unsigned long long)mr->size,
> +			 (unsigned long long)offset, (unsigned long long)size);
> +		return mr->offset <= offset &&
> +		       mr->offset + mr->size >= offset + size;
> +	}
> +
> +	pa = pci_resource_start(pdev, bar_idx) + offset;
> +	va = ioremap(pa, size);
> +	if (!va) {
> +		pci_err(pdev, "Failed to map BAR%u region\n", bar_idx);
> +		return false;
> +	}
> +
> +	mr = kvzalloc_obj(*mr);

Since the initial device setup in libie_pci_init_dev uses managed resources
like pcim_request_region, is there a reason this mapping relies on manual
ioremap and kvzalloc_obj?

Forcing drivers to manually unmap these regions during teardown can lead to
crashes if the driver unmaps the registers before the network device is fully
unregistered, as concurrent network traffic or NAPI polling might write to
the unmapped virtual addresses.

Could this use devm_ioremap and devm_kvzalloc to tie the mapping lifecycle
safely to the device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515224443.2772147-1-anthony.l.nguyen@intel.com?part=2