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

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

[Krzysztof Wilczyński]

pci_write_rom() controls access to the ROM content through the
corresponding sysfs attribute, and treats the input as a request to
disable only when it matches the string "0\n" exactly:

  if ((off ==  0) && (*buf == '0') && (count == 2))

The count == 2 condition encodes the trailing newline that echo(1)
appends.  This was found when software in userspace wrote "0" without
a trailing newline aiming to disable access, which failed to match the
condition above and enabled access instead.  For example:

  $ echo 0 > rom     <- "0\n", count 2, access disabled
  $ echo -n 0 > rom  <- "0", count 1, access enabled
  $ echo > rom       <- "", count 1, access enabled (likely not be desirable)

Thus, parse the input with kstrtobool(), which handles common boolean
inputs such as "0", "1", "n", "y" or "off", "on", with or without a
trailing newline, so both of the above disable access, and update the
now stale comment.

As a side effect, input that does not parse as a boolean is rejected
with -EINVAL rather than enabling access.  The documented "0" and "1"
continue to work as before, and rejecting malformed input brings the
attribute in line with how sysfs attributes typically handle it.

Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
---
 drivers/pci/pci-sysfs.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index d37860841260..1ddd6be25d95 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;
 
 	return count;
 }
-- 
2.54.0

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

[sashiko-bot]

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

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

[Bjorn Helgaas]

On Fri, Jun 12, 2026 at 06:24:48PM +0000, Krzysztof Wilczyński wrote:
> pci_write_rom() controls access to the ROM content through the
> corresponding sysfs attribute, and treats the input as a request to
> disable only when it matches the string "0\n" exactly:
> 
>   if ((off ==  0) && (*buf == '0') && (count == 2))
> 
> The count == 2 condition encodes the trailing newline that echo(1)
> appends.  This was found when software in userspace wrote "0" without
> a trailing newline aiming to disable access, which failed to match the
> condition above and enabled access instead.  For example:
> 
>   $ echo 0 > rom     <- "0\n", count 2, access disabled
>   $ echo -n 0 > rom  <- "0", count 1, access enabled
>   $ echo > rom       <- "", count 1, access enabled (likely not be desirable)
> 
> Thus, parse the input with kstrtobool(), which handles common boolean
> inputs such as "0", "1", "n", "y" or "off", "on", with or without a
> trailing newline, so both of the above disable access, and update the
> now stale comment.
> 
> As a side effect, input that does not parse as a boolean is rejected
> with -EINVAL rather than enabling access.  The documented "0" and "1"
> continue to work as before, and rejecting malformed input brings the
> attribute in line with how sysfs attributes typically handle it.
> 
> Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>

Applied to pci/sysfs for v7.2, thanks!

> ---
>  drivers/pci/pci-sysfs.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index d37860841260..1ddd6be25d95 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;
>  
>  	return count;
>  }
> -- 
> 2.54.0
> 

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

[Krzysztof Wilczyński]

Hello,

> > Thus, parse the input with kstrtobool(), which handles common boolean
> > inputs such as "0", "1", "n", "y" or "off", "on", with or without a
> > trailing newline, so both of the above disable access, and update the
> > now stale comment.
> > 
> > As a side effect, input that does not parse as a boolean is rejected
> > with -EINVAL rather than enabling access.  The documented "0" and "1"
> > continue to work as before, and rejecting malformed input brings the
> > attribute in line with how sysfs attributes typically handle it.
[...]
> 
> Applied to pci/sysfs for v7.2, thanks!

Thank you!

I will follow-up with a series that updates other sysfs objects we have
accordingly with this preferred API over some of the hand-rolled ones we
currently have.

All the best,

	Krzysztof