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

[Peter Maydell]

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.

>      }
>
> -    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...

thanks
-- PMM