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

[Stefan Roese <sr@denx.de>]

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