Re: [PATCH v2 2/2] pci: ensure configuration access is within bounds

[BALATON Zoltan]

On Thu, 4 Jun 2020, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While reading PCI configuration bytes, a guest may send an
> address towards the end of the configuration space. It may lead
> to an OOB access issue. Assert that 'address + len' is within
> PCI configuration space.
>
> Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> hw/pci/pci.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> Update v2: assert PCI configuration access is within bounds
>  -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg00711.html
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 70c66965f5..173bec4fd5 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1381,6 +1381,8 @@ uint32_t pci_default_read_config(PCIDevice *d,
> {
>     uint32_t val = 0;
>
> +    assert(address + len <= pci_config_size(d));

Does this allow guest now to crash QEMU? I think it was suggested that 
assert should only be used for cases that can only arise from a 
programming error and not from values set by the guest. If this is 
considered to be an error now to call this function with wrong parameters 
did you check other callers? I've found a few such as:

hw/scsi/esp-pci.c
hw/watchdog/wdt_i6300esb.c
hw/ide/cmd646.c
hw/vfio/pci.c

and maybe others. Would it be better to not crash just log invalid access 
and either fix up parameters or return some garbage like 0?

Regards,
BALATON Zoltan

> +
>     if (pci_is_express_downstream_port(d) &&
>         ranges_overlap(address, len, d->exp.exp_cap + PCI_EXP_LNKSTA, 2)) {
>         pcie_sync_bridge_lnk(d);