[PATCH 1/2] PCI/sysfs: Fix out-of-bounds read in pci_write_legacy_io()

[PATCH 1/2] PCI/sysfs: Fix out-of-bounds read in pci_write_legacy_io()

[Krzysztof Wilczyński]

pci_write_legacy_io() loads 4 bytes from the kernfs write buffer
regardless of how many bytes userspace wrote:

  if (count != 1 && count != 2 && count != 4)
          return -EINVAL;

  return pci_legacy_write(bus, off, *(u32 *)buf, count);

kernfs_fop_write_iter() allocates the buffer with kmalloc(len + 1),
so a 1-byte write to the legacy_io sysfs file allocates 2 bytes and
the unconditional u32 load reads up to 2 bytes past the end of the
allocation, which KASAN reports as a slab-out-of-bounds read.
Similarly, a 2-byte write overreads by 1 byte.

Thus, read only the number of bytes requested using get_unaligned_le16()
and get_unaligned_le32() for the 2 and 4 byte cases, interpreting the
buffer as little-endian to match the byte ordering of PCI I/O port
space.

The PowerPC implementation previously compensated for the generic
code's native-endian 32-bit load by shifting the value into place
for the 1 and 2 byte cases.  The shifts were only correct on
big-endian kernels.

On little-endian PowerPC (POWER8 and later), they extracted the wrong
bytes, so a 1-byte write wrote an out-of-bounds byte instead of the
requested value.  On big-endian, the native load also caused out_le16()
and out_le32() to reverse the user's bytes on the wire for 2 and 4 byte
writes.  The little-endian helpers resolve both issues, so the shifts
are removed.

No changes are needed for the Alpha platform.

The legacy_io file is root-only and exists only on Alpha and PowerPC,
the two architectures that define HAVE_PCI_LEGACY.

Cc: stable@vger.kernel.org
Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
---
 arch/powerpc/kernel/pci-common.c |  9 ++-------
 drivers/pci/pci-sysfs.c          | 18 +++++++++++++++---
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 8efe95a0c4ff..fdc57fa2ece6 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -626,19 +626,14 @@ int pci_legacy_write(struct pci_bus *bus, loff_t port, u32 val, size_t size)
 		return -ENXIO;
 	addr = hose->io_base_virt + port;
 
-	/* WARNING: The generic code is idiotic. It gets passed a pointer
-	 * to what can be a 1, 2 or 4 byte quantity and always reads that
-	 * as a u32, which means that we have to correct the location of
-	 * the data read within those 32 bits for size 1 and 2
-	 */
 	switch(size) {
 	case 1:
-		out_8(addr, val >> 24);
+		out_8(addr, val);
 		return 1;
 	case 2:
 		if (port & 1)
 			return -EINVAL;
-		out_le16(addr, val >> 16);
+		out_le16(addr, val);
 		return 2;
 	case 4:
 		if (port & 3)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index d37860841260..b56000ba3a33 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -933,12 +933,24 @@ static ssize_t pci_write_legacy_io(struct file *filp, struct kobject *kobj,
 				   char *buf, loff_t off, size_t count)
 {
 	struct pci_bus *bus = to_pci_bus(kobj_to_dev(kobj));
+	u32 val;
 
-	/* Only support 1, 2 or 4 byte accesses */
-	if (count != 1 && count != 2 && count != 4)
+	/* Only support 1, 2 or 4 byte accesses. */
+	switch (count) {
+	case 1:
+		val = *(u8 *)buf;
+		break;
+	case 2:
+		val = get_unaligned_le16(buf);
+		break;
+	case 4:
+		val = get_unaligned_le32(buf);
+		break;
+	default:
 		return -EINVAL;
+	}
 
-	return pci_legacy_write(bus, off, *(u32 *)buf, count);
+	return pci_legacy_write(bus, off, val, count);
 }
 
 /**
-- 
2.54.0

[PATCH 2/2] PCI/sysfs: Fix read byte order in pci_read_legacy_io()

[Krzysztof Wilczyński]

pci_read_legacy_io() passes the sysfs buffer directly to pci_legacy_read():

  return pci_legacy_read(bus, off, (u32 *)buf, count);

The PowerPC implementation stores the result as a native-endian integer:

  *((u16 *)val) = in_le16(addr);

On big-endian PowerPC this stores the bytes in the wrong order, so
a 2-byte read of a device register returns different bytes than two
1-byte reads at the same addresses.  The same applies to 4-byte
reads.  On little-endian the native byte order already matches PCI
I/O port byte order, so the conversion is a no-op.

Thus, let pci_legacy_read() store into a local u32 variable, then
copy the I/O port value to the sysfs buffer using put_unaligned_le16()
and put_unaligned_le32() for the 2 and 4 byte cases, converting from
the native integer to little-endian byte order matching PCI I/O port
space.

No changes are needed for the Alpha platform.

The legacy_io file is root-only and exists only on Alpha and PowerPC,
the two architectures that define HAVE_PCI_LEGACY.

Cc: stable@vger.kernel.org
Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
---
 drivers/pci/pci-sysfs.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index b56000ba3a33..2354d09fd3fa 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -908,12 +908,30 @@ static ssize_t pci_read_legacy_io(struct file *filp, struct kobject *kobj,
 				  char *buf, loff_t off, size_t count)
 {
 	struct pci_bus *bus = to_pci_bus(kobj_to_dev(kobj));
+	u32 val = 0;
+	int ret;
 
 	/* Only support 1, 2 or 4 byte accesses */
 	if (count != 1 && count != 2 && count != 4)
 		return -EINVAL;
 
-	return pci_legacy_read(bus, off, (u32 *)buf, count);
+	ret = pci_legacy_read(bus, off, &val, count);
+	if (ret < 0)
+		return ret;
+
+	switch (count) {
+	case 1:
+		buf[0] = *(u8 *)&val;
+		break;
+	case 2:
+		put_unaligned_le16(*(u16 *)&val, buf);
+		break;
+	case 4:
+		put_unaligned_le32(val, buf);
+		break;
+	}
+
+	return ret;
 }
 
 /**
-- 
2.54.0

Re: [PATCH 2/2] PCI/sysfs: Fix read byte order in pci_read_legacy_io()

[sashiko-bot]

> pci_read_legacy_io() passes the sysfs buffer directly to pci_legacy_read():
> 
>   return pci_legacy_read(bus, off, (u32 *)buf, count);
> 
> The PowerPC implementation stores the result as a native-endian integer:
> 
>   *((u16 *)val) = in_le16(addr);
> 
> On big-endian PowerPC this stores the bytes in the wrong order, so
> a 2-byte read of a device register returns different bytes than two
> 1-byte reads at the same addresses.  The same applies to 4-byte
> reads.  On little-endian the native byte order already matches PCI
> I/O port byte order, so the conversion is a no-op.
> 
> Thus, let pci_legacy_read() store into a local u32 variable, then
> copy the I/O port value to the sysfs buffer using put_unaligned_le16()
> and put_unaligned_le32() for the 2 and 4 byte cases, converting from
> the native integer to little-endian byte order matching PCI I/O port
> space.
> 
> No changes are needed for the Alpha platform.
> 
> The legacy_io file is root-only and exists only on Alpha and PowerPC,
> the two architectures that define HAVE_PCI_LEGACY.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH 1/2] PCI/sysfs: Fix out-of-bounds read in pci_write_legacy_io()

[sashiko-bot]

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

Pre-existing issues:
- [High] The Alpha implementation of `pci_legacy_write` passes arguments to `outb`, `outw`, and `outl` in the wrong order, causing arbitrary I/O port writes.
--

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

PCI/sysfs: Fix out-of-bounds read in pci_write_legacy_io()

This commit addresses an out-of-bounds read in pci_write_legacy_io() by
only reading the requested number of bytes using little-endian accessors.
It also removes byte-shifting workarounds in the PowerPC architecture
code that are no longer necessary.

>     On little-endian PowerPC (POWER8 and later), they extracted the wrong
>     bytes, so a 1-byte write wrote an out-of-bounds byte instead of the
>     requested value.  On big-endian, the native load also caused out_le16()
>     and out_le32() to reverse the user's bytes on the wire for 2 and 4 byte
>     writes.  The little-endian helpers resolve both issues, so the shifts
>     are removed.
>     
>     No changes are needed for the Alpha platform.
>     
>     The legacy_io file is root-only and exists only on Alpha and PowerPC,
>     the two architectures that define HAVE_PCI_LEGACY.

[Severity: High]
This is a pre-existing issue, but the claim that no changes are needed for
Alpha doesn't seem quite right.

Looking at pci_legacy_write() in arch/alpha/kernel/pci-sysfs.c, it calls the
I/O port functions with the port and value reversed:

arch/alpha/kernel/pci-sysfs.c:pci_legacy_write() {
    ...
	switch(size) {
	case 1:
		outb(port, val);
		return 1;
	case 2:
		if (port & 1)
			return -EINVAL;
		outw(port, val);
		return 2;
	case 4:
		if (port & 3)
			return -EINVAL;
		outl(port, val);
		return 4;
	}
    ...
}

Since Alpha's outb(), outw(), and outl() expect arguments in the order
(value, port), doesn't passing port as the value and val as the port
address incorrectly use user-supplied data as the target I/O port address?

Could this allow arbitrary I/O port writes system-wide?

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

Re: [PATCH 1/2] PCI/sysfs: Fix out-of-bounds read in pci_write_legacy_io()

[Krzysztof Wilczyński]

Hello,

> [Severity: High]
> This is a pre-existing issue, but the claim that no changes are needed for
> Alpha doesn't seem quite right.
> 
> Looking at pci_legacy_write() in arch/alpha/kernel/pci-sysfs.c, it calls the
> I/O port functions with the port and value reversed:
> 
> arch/alpha/kernel/pci-sysfs.c:pci_legacy_write() {
>     ...
> 	switch(size) {
> 	case 1:
> 		outb(port, val);
> 		return 1;
> 	case 2:
> 		if (port & 1)
> 			return -EINVAL;
> 		outw(port, val);
> 		return 2;
> 	case 4:
> 		if (port & 3)
> 			return -EINVAL;
> 		outl(port, val);
> 		return 4;
> 	}
>     ...
> }
> 
> Since Alpha's outb(), outw(), and outl() expect arguments in the order
> (value, port), doesn't passing port as the value and val as the port
> address incorrectly use user-supplied data as the target I/O port address?

A known issue for which a fix has been sent:

  https://lore.kernel.org/linux-pci/20260612232400.585195-1-kwilczynski@kernel.org/

Thank you!

	Krzysztof