Re: [PATCH] hw/nvme: fix mmio read

[Klaus Jensen]

On Jul 13 11:07, Peter Maydell wrote:
> On Tue, 13 Jul 2021 at 06:44, Klaus Jensen <its@irrelevant.dk> wrote:
> >
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > The new PMR test unearthed a long-standing issue with MMIO reads on
> > big-endian hosts.
> >
> > Fix by using the ldn_he_p helper instead of memcpy.
> >
> > Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/nvme/ctrl.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 2f0524e12a36..dd81c3b19c7e 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -5951,7 +5951,6 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr 
> > addr, unsigned size)
> >  {
> >      NvmeCtrl *n = (NvmeCtrl *)opaque;
> >      uint8_t *ptr = (uint8_t *)&n->bar;
> > -    uint64_t val = 0;
> >
> >      trace_pci_nvme_mmio_read(addr, size);
> >
> > @@ -5977,14 +5976,15 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr 
> > addr, unsigned size)
> >              (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
> >              memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
> >          }
> > -        memcpy(&val, ptr + addr, size);
> > -    } else {
> > -        NVME_GUEST_ERR(pci_nvme_ub_mmiord_invalid_ofs,
> > -                       "MMIO read beyond last register,"
> > -                       " offset=0x%"PRIx64", returning 0", addr);
> > +
> > +        return ldn_he_p(ptr + addr, size);
> 
> I don't think this will do the right thing for accesses which aren't
> of the same width as whatever the field in NvmeBar is defined as.
> For instance, if the guest does a 32-bit access to offset 0,
> because the first field is defined as 'uint64_t cap', on a
> big-endian host they will end up reading the high 4 bytes of the
> 64-bit value, and on a little-endian host they will get the low 4 bytes.
> 

Thanks for taking a look Peter, I wondered if I actually fixed it
correctly or not, and I obviously didnt.

I guess I can't get around handling 64 bit registers explicitly and
convert them to little endian explicitly then.

> >      }
> >
> > -    return val;
> > +    NVME_GUEST_ERR(pci_nvme_ub_mmiord_invalid_ofs,
> > +                   "MMIO read beyond last register,"
> > +                   " offset=0x%"PRIx64", returning 0", addr);
> > +
> > +    return 0;
> >  }
> 
> Looking at the surrounding code, I notice that we guard this "read size bytes
> from &n->bar + addr" with
>     if (addr < sizeof(n->bar)) {
> 
> but that doesn't account for 'size', so if the guest asks to read
> 4 bytes starting at offset sizeof(n->bar)-1 then we'll still read
> 3 bytes beyond the end of the buffer...

The buffer is at least sizeof(n->bar) + 8 bytes (there are two doorbell
registers following the controller registers). It is wrong for the host
to read those, but as per the spec it is undefined behavior. I did
consider reversing the condition to `(addr > sizeof(n->bar) - size)`. I
guess that would be the proper thing to do.

signature.asc
Description: PGP signature