Re: [PATCH v2 5/6] PCI: Align proc_bus_pci_write() with pci_write_config()

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

Pre-existing issues:
- [High] Missing error handling for `__get_user()` causes silent hardware configuration corruption.
- [High] Unaligned user pointer dereferences cause alignment faults and silent failures on strict architectures.
--

--- Patch [5]: [PATCH v2 5/6] PCI: Align proc_bus_pci_write() with pci_write_config() ---
commit 9ab25f02e0980d093d99dfa257a71fcb18afa9ca
Author: Ziming Du <[email protected]>

PCI: Align proc_bus_pci_write() with pci_write_config()

This commit aligns proc_bus_pci_write() with pci_write_config() to improve
consistency across the PCI subsystem. It updates variable names, simplifies
bounds checking, and modifies transfer loop conditions.

> diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
> index 3f041765da978..71d1f34e13a7d 100644
> --- a/drivers/pci/proc.c
> +++ b/drivers/pci/proc.c
[ ... ]
> @@ -114,80 +114,79 @@ static ssize_t proc_bus_pci_write(struct file *file, const char __user *buf,
[ ... ]
> -	if ((pos & 3) && cnt > 2) {
> +	if ((off & 3) && size > 2) {
>  		__le16 val;
>  		__get_user(val, (__le16 __user *) buf);

[Severity: High]
This isn't a bug introduced by this patch, but can this cast to a multi-byte
type result in an unaligned user pointer dereference?

If a user calls pwrite() with a buffer whose alignment phase doesn't match
the file offset (e.g., a page-aligned buffer with offset=1), synchronizing
the buf and off increments guarantees that when off reaches a 16-bit or
32-bit boundary, buf will be unaligned.

On architectures with strict alignment requirements, could this trigger
alignment faults in kernel space, causing __get_user() to abort and return
-EFAULT?

> -		pci_user_write_config_word(dev, pos, le16_to_cpu(val));
> +		pci_user_write_config_word(dev, off, le16_to_cpu(val));
>  		buf += 2;
> -		pos += 2;
> -		cnt -= 2;
> +		off += 2;
> +		size -= 2;
>  	}
>  
> -	while (cnt >= 4) {
> +	while (size > 3) {
>  		__le32 val;
>  		__get_user(val, (__le32 __user *) buf);
> -		pci_user_write_config_dword(dev, pos, le32_to_cpu(val));

[Severity: High]
This is a pre-existing issue, but does this code ignore the return value of
__get_user() before executing the hardware write?

If a user passes a buffer that spans into unmapped memory, __get_user()
will encounter a page fault, return -EFAULT, and set the output value to
zero. Because proc_bus_pci_write() completely ignores this return value,
could it proceed to silently write these zeros to critical PCI configuration
space registers (like BARs or the Command register) and potentially lock up
the bus or crash the system?

> +		pci_user_write_config_dword(dev, off, le32_to_cpu(val));
>  		buf += 4;
> -		pos += 4;
> -		cnt -= 4;
> +		off += 4;
> +		size -= 4;
>  	}

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