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

[BALATON Zoltan]

On Thu, 4 Jun 2020, Michael S. Tsirkin wrote:
> 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.

I wonder why isn't the check added to pci_default_read_config() right 
away? If we have an assert there the overhead is the same and adding the 
check there would make it unnecessary to patch all callers so it's just 
one patch instead of a whole series.

Regards,
BALATON Zoltan