Re: [PATCH v2 2/6] PCI/proc: Serialize config inode size updates

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

Pre-existing issues:
- [High] The return values of `__get_user()` and `__put_user()` are silently ignored, risking hardware configuration corruption and silent failures on page faults.
--

--- Patch [2]: [PATCH v2 2/6] PCI/proc: Serialize config inode size updates ---
commit 3aca0ed476ea2dd3079bd4be2a68be7793fdde7b
Author: Ziming Du <[email protected]>

PCI/proc: Serialize config inode size updates

proc_bus_pci_write() calls i_size_write() without serializing concurrent
writers. On 32-bit SMP systems, concurrent updates can corrupt
i_size_seqcount and make later i_size_read() calls spin indefinitely.
This patch protects the inode size update with the inode lock.

> diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
> index cad749924dd89..ec2338c1e667e 100644
> --- a/drivers/pci/proc.c
> +++ b/drivers/pci/proc.c
> @@ -181,7 +181,9 @@ static ssize_t proc_bus_pci_write(struct file *file, const char __user *buf,
>  	pci_config_pm_runtime_put(dev);
>  
>  	*ppos = pos;
> +	inode_lock(ino);
>  	i_size_write(ino, dev->cfg_size);

[Severity: High]
This isn't a bug introduced by this patch, but looking at
proc_bus_pci_write() earlier in this function, the return value of
__get_user() is silently ignored:

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

Does this code fail to handle page faults correctly?

If a process calls write() with a buffer that fails memory access,
__get_user() sets the target variable to zero and returns -EFAULT.
Because this return value is ignored, the kernel proceeds to write these
uninitialized zeroes directly into the PCI device's configuration space.

Can this cause hardware misconfiguration, system lockups, or kernel panics?

Similarly, in proc_bus_pci_read(), the return value of __put_user() is
discarded:

drivers/pci/proc.c:proc_bus_pci_read() {
	...
		unsigned char val;
		pci_user_read_config_byte(dev, pos, &val);
		__put_user(val, buf);
	...
}

Could this lead to silent failures where data is not successfully copied to
userspace despite returning success, leaving the user program to consume
uninitialized memory?

Is it possible to check the return values of __get_user() and __put_user()
in these functions and return -EFAULT if they fail?

> +	inode_unlock(ino);
>  	return nbytes;
>  }

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