Re: [PATCH v4 3/4] PCI: Prevent overflow in proc_bus_pci_write()

Bjorn Helgaas
2026-03-03T19:32:55Z

On Fri, Jan 16, 2026 at 04:17:20PM +0800, Ziming Du wrote:
> From: Yongqiang Liu <liuyongqiang13@huawei.com>
>
> When the value of *ppos over the INT_MAX, the pos is over set to a
> negative value which will be passed to get_user() or
> pci_user_write_config_dword(). Unexpected behavior such as a soft lockup
> will happen as follows:

I think it's crazy to worry about offsets overflowing INT_MAX. We're
doing PCI config accesses. Config space is only 4K at most, so we
already have a much smaller upper bound on the offset.

The procfs proc_bus_pci_write() is essentially the same as the sysfs
pci_write_config(). They should share some common implementation.

> watchdog: BUG: soft lockup - CPU#0 stuck for 130s! [syz.3.109:3444]
> RIP: 0010:_raw_spin_unlock_irq+0x17/0x30
> Call Trace:
> <TASK>
> pci_user_write_config_dword+0x126/0x1f0
> proc_bus_pci_write+0x273/0x470
> proc_reg_write+0x1b6/0x280
> do_iter_write+0x48e/0x790
> vfs_writev+0x125/0x4a0
> __x64_sys_pwritev+0x1e2/0x2a0
> do_syscall_64+0x59/0x110
> entry_SYSCALL_64_after_hwframe+0x78/0xe2
>
> Fix this by adding a non-negative check before assign *ppos to pos.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Yongqiang Liu <liuyongqiang13@huawei.com>
> Signed-off-by: Ziming Du <duziming2@huawei.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> drivers/pci/proc.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
> index 9348a0fb80847..2d51b26edbe74 100644
> --- a/drivers/pci/proc.c
> +++ b/drivers/pci/proc.c
> @@ -113,10 +113,14 @@ static ssize_t proc_bus_pci_write(struct file *file, const char __user *buf,
> {
> struct inode *ino = file_inode(file);
> struct pci_dev *dev = pde_data(ino);
> - int pos = *ppos;
> + int pos;
> int size = dev->cfg_size;
> int cnt, ret;
>
> + if (*ppos > INT_MAX)
> + return -EINVAL;
> + pos = *ppos;

The first issue here is that "*ppos" (loff_t) is long long, but "pos"
is int, so we're assigning a 64-bit value to a 32-bit container and
losing any high bits. So an offset of 0x100000000 is incorrectly
treated as valid.

A change like this should fix that and bring this closer to the
pci_write_config() implementation:

- int pos = *ppos;
+ loff_t pos = *ppos;

- if (pos >= size)
+ if (pos > dev->cfg_size)
return 0;

There's also a second issue here:

if (pos + nbytes > size)
nbytes = size - pos;

"pos" is a signed int, "nbytes" is size_t, which is an *unsigned* int,
so "pos" is implicitly converted to an unsigned value. I think this
is what causes the soft lockup you reported because an offset like the
0x80800001 in your test case is converted from signed -2139095039 to
unsigned 2155872257. "size" is dev->cfg_size, e.g., 4096, so
2155872257 + nbytes is certainly larger than 4096, so nbytes ends up
being set to some huge unsigned size_t value.

This issue would probably be avoided simply by returning early when
"pos" is out of range. But mixing signed and unsigned in that
"pos + nbytes" expression is just asking for trouble and we should
avoid it as pci_write_config() does.

So I'd like to see something that makes the procfs accessor
validations look like the sysfs accessors. It's a little messy
because they use different names, so the patches will be ugly. But I
think it's worth it to make them work the same way so we don't have to
analyze them separately.

Maybe could be done in a couple steps, e.g., one to simply rename
things and a second to make the functional changes.

Bjorn