| Message ID | 20220703104806.27192-1-pali@kernel.org |
|---|---|
| State | Accepted |
| Commit | d9f554b62440de542c482fecf9374e8da3ea3602 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | pci: Add checks to prevent config space overflow | expand |
On 03.07.22 12:48, Pali Rohár wrote: > PCIe config space has address range 0-4095. So do not allow reading from > addresses outside of this range. Lot of U-Boot drivers do not expect that > passed value is not in this range. PCI DM read function is exetended to s/exetended/extended > fill read value to all ones or zeros when it fails as U-Boot callers > ignores return value. > > Calling U-Boot command 'pci display.b 0.0.0 0 0x2000' now stops printing > config space at the end (before 0x1000 address). > > Signed-off-by: Pali Rohár <pali@kernel.org> > --- > cmd/pci.c | 16 ++++++++++++++-- > drivers/pci/pci-uclass.c | 10 +++++++++- > 2 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/cmd/pci.c b/cmd/pci.c > index a99e8f8ad6e0..6258699fec81 100644 > --- a/cmd/pci.c > +++ b/cmd/pci.c > @@ -358,6 +358,9 @@ static int pci_cfg_display(struct udevice *dev, ulong addr, > if (length == 0) > length = 0x40 / byte_size; /* Standard PCI config space */ > > + if (addr >= 4096) > + return 1; > + > /* Print the lines. > * once, and all accesses are with the specified bus width. > */ > @@ -378,7 +381,10 @@ static int pci_cfg_display(struct udevice *dev, ulong addr, > rc = 1; > break; > } > - } while (nbytes > 0); > + } while (nbytes > 0 && addr < 4096); > + > + if (rc == 0 && nbytes > 0) > + return 1; > > return (rc); > } > @@ -390,6 +396,9 @@ static int pci_cfg_modify(struct udevice *dev, ulong addr, ulong size, > int nbytes; > ulong val; > > + if (addr >= 4096) > + return 1; > + > /* Print the address, followed by value. Then accept input for > * the next value. A non-converted value exits. > */ > @@ -427,7 +436,10 @@ static int pci_cfg_modify(struct udevice *dev, ulong addr, ulong size, > addr += size; > } > } > - } while (nbytes); > + } while (nbytes && addr < 4096); > + > + if (nbytes) > + return 1; > > return 0; > } > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c > index 89245a271e16..7402079471c8 100644 > --- a/drivers/pci/pci-uclass.c > +++ b/drivers/pci/pci-uclass.c > @@ -288,6 +288,8 @@ int pci_bus_write_config(struct udevice *bus, pci_dev_t bdf, int offset, > ops = pci_get_ops(bus); > if (!ops->write_config) > return -ENOSYS; > + if (offset < 0 || offset >= 4096) > + return -EINVAL; > return ops->write_config(bus, bdf, offset, value, size); > } > > @@ -366,8 +368,14 @@ int pci_bus_read_config(const struct udevice *bus, pci_dev_t bdf, int offset, > struct dm_pci_ops *ops; > > ops = pci_get_ops(bus); > - if (!ops->read_config) > + if (!ops->read_config) { > + *valuep = pci_conv_32_to_size(~0, offset, size); > return -ENOSYS; > + } > + if (offset < 0 || offset >= 4096) { > + *valuep = pci_conv_32_to_size(0, offset, size); > + return -EINVAL; > + } How about introducing a macro for this 4096 max value instead? Other than this: Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan
On Monday 04 July 2022 08:00:23 Stefan Roese wrote: > On 03.07.22 12:48, Pali Rohár wrote: > > PCIe config space has address range 0-4095. So do not allow reading from > > addresses outside of this range. Lot of U-Boot drivers do not expect that > > passed value is not in this range. PCI DM read function is exetended to > > s/exetended/extended Should I send a new patch version? > > fill read value to all ones or zeros when it fails as U-Boot callers > > ignores return value. > > > > Calling U-Boot command 'pci display.b 0.0.0 0 0x2000' now stops printing > > config space at the end (before 0x1000 address). > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > --- > > cmd/pci.c | 16 ++++++++++++++-- > > drivers/pci/pci-uclass.c | 10 +++++++++- > > 2 files changed, 23 insertions(+), 3 deletions(-) > > > > diff --git a/cmd/pci.c b/cmd/pci.c > > index a99e8f8ad6e0..6258699fec81 100644 > > --- a/cmd/pci.c > > +++ b/cmd/pci.c > > @@ -358,6 +358,9 @@ static int pci_cfg_display(struct udevice *dev, ulong addr, > > if (length == 0) > > length = 0x40 / byte_size; /* Standard PCI config space */ > > + if (addr >= 4096) > > + return 1; > > + > > /* Print the lines. > > * once, and all accesses are with the specified bus width. > > */ > > @@ -378,7 +381,10 @@ static int pci_cfg_display(struct udevice *dev, ulong addr, > > rc = 1; > > break; > > } > > - } while (nbytes > 0); > > + } while (nbytes > 0 && addr < 4096); > > + > > + if (rc == 0 && nbytes > 0) > > + return 1; > > return (rc); > > } > > @@ -390,6 +396,9 @@ static int pci_cfg_modify(struct udevice *dev, ulong addr, ulong size, > > int nbytes; > > ulong val; > > + if (addr >= 4096) > > + return 1; > > + > > /* Print the address, followed by value. Then accept input for > > * the next value. A non-converted value exits. > > */ > > @@ -427,7 +436,10 @@ static int pci_cfg_modify(struct udevice *dev, ulong addr, ulong size, > > addr += size; > > } > > } > > - } while (nbytes); > > + } while (nbytes && addr < 4096); > > + > > + if (nbytes) > > + return 1; > > return 0; > > } > > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c > > index 89245a271e16..7402079471c8 100644 > > --- a/drivers/pci/pci-uclass.c > > +++ b/drivers/pci/pci-uclass.c > > @@ -288,6 +288,8 @@ int pci_bus_write_config(struct udevice *bus, pci_dev_t bdf, int offset, > > ops = pci_get_ops(bus); > > if (!ops->write_config) > > return -ENOSYS; > > + if (offset < 0 || offset >= 4096) > > + return -EINVAL; > > return ops->write_config(bus, bdf, offset, value, size); > > } > > @@ -366,8 +368,14 @@ int pci_bus_read_config(const struct udevice *bus, pci_dev_t bdf, int offset, > > struct dm_pci_ops *ops; > > ops = pci_get_ops(bus); > > - if (!ops->read_config) > > + if (!ops->read_config) { > > + *valuep = pci_conv_32_to_size(~0, offset, size); > > return -ENOSYS; > > + } > > + if (offset < 0 || offset >= 4096) { > > + *valuep = pci_conv_32_to_size(0, offset, size); > > + return -EINVAL; > > + } > > How about introducing a macro for this 4096 max value instead? > > Other than this: > > Reviewed-by: Stefan Roese <sr@denx.de> > > Thanks, > Stefan
On Sun, Jul 03, 2022 at 12:48:06PM +0200, Pali Rohár wrote: > PCIe config space has address range 0-4095. So do not allow reading from > addresses outside of this range. Lot of U-Boot drivers do not expect that > passed value is not in this range. PCI DM read function is extended to > fill read value to all ones or zeros when it fails as U-Boot callers > ignores return value. > > Calling U-Boot command 'pci display.b 0.0.0 0 0x2000' now stops printing > config space at the end (before 0x1000 address). > > Signed-off-by: Pali Rohár <pali@kernel.org> > Reviewed-by: Stefan Roese <sr@denx.de> Applied to u-boot/master, thanks!
diff --git a/cmd/pci.c b/cmd/pci.c index a99e8f8ad6e0..6258699fec81 100644 --- a/cmd/pci.c +++ b/cmd/pci.c @@ -358,6 +358,9 @@ static int pci_cfg_display(struct udevice *dev, ulong addr, if (length == 0) length = 0x40 / byte_size; /* Standard PCI config space */ + if (addr >= 4096) + return 1; + /* Print the lines. * once, and all accesses are with the specified bus width. */ @@ -378,7 +381,10 @@ static int pci_cfg_display(struct udevice *dev, ulong addr, rc = 1; break; } - } while (nbytes > 0); + } while (nbytes > 0 && addr < 4096); + + if (rc == 0 && nbytes > 0) + return 1; return (rc); } @@ -390,6 +396,9 @@ static int pci_cfg_modify(struct udevice *dev, ulong addr, ulong size, int nbytes; ulong val; + if (addr >= 4096) + return 1; + /* Print the address, followed by value. Then accept input for * the next value. A non-converted value exits. */ @@ -427,7 +436,10 @@ static int pci_cfg_modify(struct udevice *dev, ulong addr, ulong size, addr += size; } } - } while (nbytes); + } while (nbytes && addr < 4096); + + if (nbytes) + return 1; return 0; } diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index 89245a271e16..7402079471c8 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -288,6 +288,8 @@ int pci_bus_write_config(struct udevice *bus, pci_dev_t bdf, int offset, ops = pci_get_ops(bus); if (!ops->write_config) return -ENOSYS; + if (offset < 0 || offset >= 4096) + return -EINVAL; return ops->write_config(bus, bdf, offset, value, size); } @@ -366,8 +368,14 @@ int pci_bus_read_config(const struct udevice *bus, pci_dev_t bdf, int offset, struct dm_pci_ops *ops; ops = pci_get_ops(bus); - if (!ops->read_config) + if (!ops->read_config) { + *valuep = pci_conv_32_to_size(~0, offset, size); return -ENOSYS; + } + if (offset < 0 || offset >= 4096) { + *valuep = pci_conv_32_to_size(0, offset, size); + return -EINVAL; + } return ops->read_config(bus, bdf, offset, valuep, size); }
PCIe config space has address range 0-4095. So do not allow reading from addresses outside of this range. Lot of U-Boot drivers do not expect that passed value is not in this range. PCI DM read function is exetended to fill read value to all ones or zeros when it fails as U-Boot callers ignores return value. Calling U-Boot command 'pci display.b 0.0.0 0 0x2000' now stops printing config space at the end (before 0x1000 address). Signed-off-by: Pali Rohár <pali@kernel.org> --- cmd/pci.c | 16 ++++++++++++++-- drivers/pci/pci-uclass.c | 10 +++++++++- 2 files changed, 23 insertions(+), 3 deletions(-)