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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jun 04, 2020 at 08:07:52AM +0200, Philippe Mathieu-Daudé wrote:
> On 6/4/20 12:13 AM, BALATON Zoltan wrote:
> > 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?
> 
> Yes, maybe I was not clear while reviewing v1, we need to audit the
> callers and fix them first, then we can safely add the assert here.

We can add assert here regardless of auditing callers. Doing that
will also make fuzzying easier. But the assert is unrelated to CVE imho.

> > 
> > 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);
> >>