[PATCH] hw/nvme: fix mmio read

[PATCH] hw/nvme: fix mmio read

[Klaus Jensen]

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);
     }
 
-    return val;
+    NVME_GUEST_ERR(pci_nvme_ub_mmiord_invalid_ofs,
+                   "MMIO read beyond last register,"
+                   " offset=0x%"PRIx64", returning 0", addr);
+
+    return 0;
 }
 
 static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
-- 
2.32.0

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

[Gollu Appalanaidu]

[-- Attachment #1: Type: text/plain, Size: 1792 bytes --]

On Tue, Jul 13, 2021 at 07:43:59AM +0200, Klaus Jensen 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);
>     }
>
>-    return val;
>+    NVME_GUEST_ERR(pci_nvme_ub_mmiord_invalid_ofs,
>+                   "MMIO read beyond last register,"
>+                   " offset=0x%"PRIx64", returning 0", addr);
>+
>+    return 0;
> }
>
> static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
>-- 
>2.32.0
>
>
LGTM.

Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

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

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 3194 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Peter Maydell]

On Tue, 13 Jul 2021 at 11:19, Klaus Jensen <its@irrelevant.dk> wrote:
>
> On Jul 13 11:07, Peter Maydell wrote:
> > 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 don't know about the doorbell registers, but with this code
(or the old memcpy()) you'll access whatever the next thing after
"NvmeBar bar" in the NvmeCtrl struct is, which looks like it's the
first part of 'struct NvmeParams".

thanks
-- PMM

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

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

On Jul 13 11:31, Peter Maydell wrote:
> On Tue, 13 Jul 2021 at 11:19, Klaus Jensen <its@irrelevant.dk> wrote:
> >
> > On Jul 13 11:07, Peter Maydell wrote:
> > > 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 don't know about the doorbell registers, but with this code
> (or the old memcpy()) you'll access whatever the next thing after
> "NvmeBar bar" in the NvmeCtrl struct is, which looks like it's the
> first part of 'struct NvmeParams".
> 

Sorry, you are of course right. I remembered how the bar was allocated
incorrectly.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1294 bytes --]

On Jul 13 12:34, Klaus Jensen wrote:
> On Jul 13 11:31, Peter Maydell wrote:
> > On Tue, 13 Jul 2021 at 11:19, Klaus Jensen <its@irrelevant.dk> wrote:
> > >
> > > On Jul 13 11:07, Peter Maydell wrote:
> > > > 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 don't know about the doorbell registers, but with this code
> > (or the old memcpy()) you'll access whatever the next thing after
> > "NvmeBar bar" in the NvmeCtrl struct is, which looks like it's the
> > first part of 'struct NvmeParams".
> > 
> 
> Sorry, you are of course right. I remembered how the bar was allocated
> incorrectly.

I fixed this properly by holding all bar values in little endian as per
the spec.

I'll clean it up and send it tonight.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]