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

[Bjorn Helgaas <helgaas@kernel.org>]

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