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()
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.
--- arch/powerpc/kernel/pci-common.c | 9 ++------- drivers/pci/pci-sysfs.c | 18 +++++++++++++++--- 2 files changed, 17 insertions(+), 10 deletions(-)
arch/powerpc/kernel/pci-common.c +2 -7
@@ -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 )
drivers/pci/pci-sysfs.c +15 -3
@@ -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); }/**
Signature · 1 line
2.54.0
PATCH 2/2 PCI/sysfs: Fix read byte order in pci_read_legacy_io()
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.
--- drivers/pci/pci-sysfs.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/pci/pci-sysfs.c +19 -1
@@ -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; }/**
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH 2/2] PCI/sysfs: Fix read byte order in pci_read_legacy_io()
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 20 lines
pci_read_legacy_io()passes the sysfs buffer directly topci_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 usingput_unaligned_le16()andput_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.
Quoted 7 lines
No changes are needed for the Alpha platform.
The
legacy_iofile is root-only and exists only on Alpha and PowerPC, the two architectures that defineHAVE_PCI_LEGACY.Signed-off-by Krzysztof Wilczyński <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
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 <[email protected]>
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.
Quoted 11 lines
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()andout_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_iofile is root-only and exists only on Alpha and PowerPC, the two architectures that defineHAVE_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?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Hello,
Quoted 27 lines
[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()inarch/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; } ... }
Quoted 3 lines
Since Alpha's
outb(),outw(), andoutl()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/[email protected]/
Thank you!
Krzysztof
Krzysztof Wilczyński
AppliedHello,
Quoted 31 lines
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 thelegacy_iosysfs 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()andget_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()andout_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.
Quoted 4 lines
No changes are needed for the Alpha platform.
The
legacy_iofile is root-only and exists only on Alpha and PowerPC, the two architectures that defineHAVE_PCI_LEGACY.
Applied to the sysfs branch. I would like for the 0-day bot to pick this up, and also to have some linux-next soak time, if possible.
Thank you!
Krzysztof