Re: [PATCH v2 3/6] PCI/proc: Warn on writes to driver-exclusive config regions

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] The format string in pci_warn_once lacks a terminating newline character.

Pre-existing issues:
- [High] The return values of __get_user() are ignored in proc_bus_pci_write(), leading to silent hardware writes of zero or uninitialized data on fault.
- [High] proc_bus_pci_write() performs unaligned user memory accesses via __get_user(), violating API constraints and potentially triggering alignment faults.
--

--- Patch [3]: [PATCH v2 3/6] PCI/proc: Warn on writes to driver-exclusive config regions ---
commit 7011bc96a28549a7bd6d8b00360cce10cb406486
Author: Ziming Du <[email protected]>

PCI/proc: Warn on writes to driver-exclusive config regions

The sysfs configuration-space write path warns and taints the kernel
when userspace writes a region claimed exclusively by a driver. The
procfs path can access the same configuration space but does not perform
this check.

Add the same resource_is_exclusive() warning and taint to
proc_bus_pci_write() so the two userspace interfaces treat driver-owned
ranges consistently.

Fixes: 278294798ac9 ("PCI: Allow drivers to request exclusive config regions")

> diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
> index ec2338c1e667e..3f041765da978 100644
> --- a/drivers/pci/proc.c
> +++ b/drivers/pci/proc.c
> @@ -122,6 +122,13 @@ static ssize_t proc_bus_pci_write(struct file *file, const char __user *buf,
>  	if (ret)
>  		return ret;
>  
> +	if (resource_is_exclusive(&dev->driver_exclusive_resource, pos,
> +				  nbytes)) {
> +		pci_warn_once(dev, "%s: Unexpected write to kernel-exclusive config offset %llx",
> +			      current->comm, pos);

[Severity: Low]
Is a trailing \n character needed for this format string?

Without it, subsequent kernel log messages from any subsystem might be
appended to the same line, resulting in garbled dmesg output.

> +		add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> +	}
> +
>  	if (pos >= size)
>  		return 0;


drivers/pci/proc.c:proc_bus_pci_write() {
...
	if ((pos & 1) && cnt) {
		unsigned char val;
		__get_user(val, buf);
		pci_user_write_config_byte(dev, pos, val);
...
}

[Severity: High]
This is a pre-existing issue, but is the return value of __get_user() being
safely ignored here?

If __get_user() fails (e.g., due to an unmapped buffer or an unhandled page
fault), it returns -EFAULT.

By ignoring this return value, the kernel silently proceeds to write zero
(or potentially uninitialized stack data on older architectures) to the PCI
hardware configuration space instead of aborting.


drivers/pci/proc.c:proc_bus_pci_write() {
...
	if ((pos & 1) && cnt) {
		unsigned char val;
		__get_user(val, buf);
		pci_user_write_config_byte(dev, pos, val);
		buf++;
		pos++;
		cnt--;
	}

	if ((pos & 3) && cnt > 2) {
		__le16 val;
		__get_user(val, (__le16 __user *) buf);
...
}

[Severity: High]
This is also a pre-existing issue, but could this sequence lead to unaligned
user memory accesses?

If a privileged user issues a write with an odd offset (pos & 1) and
length > 2, the code reads the first byte and increments buf by 1.

This makes the pointer unaligned before casting it to (__le16 __user *) for
the next __get_user() call.

Passing an unaligned pointer can trigger a kernel oops on strict-alignment
architectures that lack unaligned kernel access fixups, or it may return a
silent -EFAULT.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3