[PATCH] pci: Add checks to prevent config space overflow

[Pali Rohár]

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 at 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 at denx.de>
> 
> Thanks,
> Stefan