Re: [PATCH] PCI/sysfs: Use kstrtobool() to parse the ROM attribute input

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unprotected modification of the `rom_attr_enabled` bitfield causes a read-modify-write data race with other device state flags in `struct pci_dev`.
--

commit f2195cc9fc7471560c4231d0af873e622171b383
Author: Krzysztof Wilczyński <kwilczynski@kernel.org>

PCI/sysfs: Use kstrtobool() to parse the ROM attribute input

This commit changes the sysfs PCI ROM attribute handler to use kstrtobool()
for parsing user input instead of manually checking for "0\n". It ensures
that common boolean inputs are handled correctly, with or without trailing
newlines, and rejects invalid inputs with -EINVAL.

> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index d37860841260c..1ddd6be25d956 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c

[ ... ]

> @@ -1309,18 +1309,19 @@ void __weak pci_remove_resource_files(struct pci_dev *dev) { return; }
>   * @off: file offset
>   * @count: number of byte in input
>   *
> - * writing anything except 0 enables it
> + * Writing a boolean value enables or disables the ROM display.
>   */
>  static ssize_t pci_write_rom(struct file *filp, struct kobject *kobj,
>  			     const struct bin_attribute *bin_attr, char *buf,
>  			     loff_t off, size_t count)
>  {
>  	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> +	bool enable;
>  
> -	if ((off ==  0) && (*buf == '0') && (count == 2))
> -		pdev->rom_attr_enabled = 0;
> -	else
> -		pdev->rom_attr_enabled = 1;
> +	if (kstrtobool(buf, &enable))
> +		return -EINVAL;
> +
> +	pdev->rom_attr_enabled = enable;

[Severity: High]
This isn't a bug introduced by this patch, but does modifying the
rom_attr_enabled bitfield without holding device_lock() cause a
read-modify-write data race with other bits in the same word?

The rom_attr_enabled flag is defined as a 1-bit bitfield in struct pci_dev
alongside other critical operational flags:

include/linux/pci.h:struct pci_dev {
    ...
	unsigned int	irq_managed:1;
    ...
	unsigned int	is_probed:1;
    ...
	unsigned int	rom_attr_enabled:1;
    ...
}

Because pci_write_rom() is invoked directly from the sysfs VFS write path,
it does not implicitly hold device_lock() or any PCI-specific synchronization.

If a privileged userspace process writes to the rom sysfs attribute while
another kernel thread is binding a driver (which modifies is_probed) or
enabling the device (which modifies irq_managed), the two CPUs might race
on the shared memory word.

Could this data race silently discard concurrent updates, potentially causing
issues like a nested work_on_cpu() deadlock if is_probed is lost, or an IRQ
resource leak if irq_managed is lost? Would it be safer to use atomic bitops
or acquire device_lock() when modifying this state?

>  
>  	return count;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612182448.552406-1-kwilczynski@kernel.org?part=1