| Message ID | 20210713054359.831878-1-its@irrelevant.dk |
|---|---|
| State | New |
| Headers | show |
| Series | hw/nvme: fix mmio read | expand |
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>
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
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.
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
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.
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.
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)