diff mbox series

[v2,5/9] nvram: add nrf51_soc flash read method

Message ID 20200624185523.762240-6-ppandit@redhat.com (mailing list archive)
State New, archived
Headers show
Series memory: assert and define MemoryRegionOps callbacks | expand

Commit Message

Prasad Pandit June 24, 2020, 6:55 p.m. UTC 
From: Prasad J Pandit <pjp@fedoraproject.org>

Add nrf51_soc mmio read method to avoid NULL pointer dereference
issue.

Reported-by: Lei Sun <slei.casper@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/nvram/nrf51_nvm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Update v2: return ldl_le_p()
  -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg04972.html

Comments

Li Qiang June 29, 2020, 11:17 a.m. UTC | #1 
P J P <ppandit@redhat.com> 于2020年6月25日周四 上午3:01写道：
>
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> Add nrf51_soc mmio read method to avoid NULL pointer dereference
> issue.
>
> Reported-by: Lei Sun <slei.casper@gmail.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/nvram/nrf51_nvm.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> Update v2: return ldl_le_p()
>   -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg04972.html
>
> diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c
> index f2283c1a8d..8000ed530a 100644
> --- a/hw/nvram/nrf51_nvm.c
> +++ b/hw/nvram/nrf51_nvm.c
> @@ -273,6 +273,13 @@ static const MemoryRegionOps io_ops = {
>          .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>
> +static uint64_t flash_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    NRF51NVMState *s = NRF51_NVM(opaque);
> +
> +    assert(offset + size <= s->flash_size);
> +    return ldl_le_p(s->storage + offset);
> +}

The 'flash_ops' is for ROM, though I don't see where it calls
'memory_region_rom_device_set_romd'
to ROMD, so this MR is in MMIO mode and it needs a read callback.

However as the origin code doesn't provide a read callback. So why
here we return something?

I prefer here just 'qemu_log_mask(LOG_UNIMP, "%s not implemented\n",
__func__);' as other
patches does.

Thanks,
Li Qiang


>
>  static void flash_write(void *opaque, hwaddr offset, uint64_t value,
>          unsigned int size)
> @@ -300,6 +307,7 @@ static void flash_write(void *opaque, hwaddr offset, uint64_t value,
>
>
>  static const MemoryRegionOps flash_ops = {
> +    .read = flash_read,
>      .write = flash_write,
>      .valid.min_access_size = 4,
>      .valid.max_access_size = 4,
> --
> 2.26.2
>
Prasad Pandit June 29, 2020, 11:55 a.m. UTC | #2 
Hello Li,

+-- On Mon, 29 Jun 2020, Li Qiang wrote --+
| P J P <ppandit@redhat.com> 于2020年6月25日周四 上午3:01写道：
| > Update v2: return ldl_le_p()
| >   -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg04972.html
| >
| > +static uint64_t flash_read(void *opaque, hwaddr offset, unsigned size)
| > +{
| > +    NRF51NVMState *s = NRF51_NVM(opaque);
| > +
| > +    assert(offset + size <= s->flash_size);
| > +    return ldl_le_p(s->storage + offset);
| > +}
| 
| However as the origin code doesn't provide a read callback. So why here we 
| return something?
| 
| I prefer here just 'qemu_log_mask(LOG_UNIMP, "%s not implemented\n", 
| __func__);' as other patches does.

Earlier patch v1 did that. It was suggested to return ldl_le_p(), as that's a 
valid return IIUC, instead of a zero(0), in case flash_read() is called.

Thanks so much for the reviews. I'll send a revised series with due updates.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Paolo Bonzini June 29, 2020, 3:31 p.m. UTC | #3 
On 29/06/20 13:17, Li Qiang wrote:
> The 'flash_ops' is for ROM, though I don't see where it calls
> 'memory_region_rom_device_set_romd'
> to ROMD, so this MR is in MMIO mode and it needs a read callback.
> 
> However as the origin code doesn't provide a read callback. So why
> here we return something?
> 
> I prefer here just 'qemu_log_mask(LOG_UNIMP, "%s not implemented\n",
> __func__);' as other
> patches does.

Even abort() would do (with a comment).

Paolo
Paolo Bonzini June 29, 2020, 3:32 p.m. UTC | #4 
On 29/06/20 13:55, P J P wrote:
> | 
> | I prefer here just 'qemu_log_mask(LOG_UNIMP, "%s not implemented\n", 
> | __func__);' as other patches does.
> 
> Earlier patch v1 did that. It was suggested to return ldl_le_p(), as that's a 
> valid return IIUC, instead of a zero(0), in case flash_read() is called.
> 
> Thanks so much for the reviews. I'll send a revised series with due updates.

I think abort() is preferable (while LOG_UNIMP is wrong as it implies
there is something to do that QEMU is not doing).

Paolo
Li Qiang June 29, 2020, 4:05 p.m. UTC | #5 
Paolo Bonzini <pbonzini@redhat.com> 于2020年6月29日周一 下午11:32写道：
>
> On 29/06/20 13:55, P J P wrote:
> > |
> > | I prefer here just 'qemu_log_mask(LOG_UNIMP, "%s not implemented\n",
> > | __func__);' as other patches does.
> >
> > Earlier patch v1 did that. It was suggested to return ldl_le_p(), as that's a
> > valid return IIUC, instead of a zero(0), in case flash_read() is called.
> >
> > Thanks so much for the reviews. I'll send a revised series with due updates.
>
> I think abort() is preferable (while LOG_UNIMP is wrong as it implies
> there is something to do that QEMU is not doing).
>

Oh, here the UNIMP I understand as it will not be implemented, not the
thing 'should do but not do now'.

If. we use. abort(), the guest also. can trigger this abort(crash).
Though there is also other places we use this I think it is not very good.

In fact I would like to silent ignore(callback do nothing) if the
developer doesn't provide callback at the beginning.
But there are some cases we use LOG_UNIMP:
-->https://git.qemu.org/?p=qemu.git;a=commit;h=158b659451


Thanks,
Li Qiang


> Paolo
>
Peter Maydell July 16, 2020, 4:27 p.m. UTC | #6 
On Mon, 29 Jun 2020 at 12:18, Li Qiang <liq3ea@gmail.com> wrote:
>
> P J P <ppandit@redhat.com> 于2020年6月25日周四 上午3:01写道：
> >
> > From: Prasad J Pandit <pjp@fedoraproject.org>
> >
> > Add nrf51_soc mmio read method to avoid NULL pointer dereference
> > issue.
> >
> > Reported-by: Lei Sun <slei.casper@gmail.com>
> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> > ---
> >  hw/nvram/nrf51_nvm.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > Update v2: return ldl_le_p()
> >   -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg04972.html
> >
> > diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c
> > index f2283c1a8d..8000ed530a 100644
> > --- a/hw/nvram/nrf51_nvm.c
> > +++ b/hw/nvram/nrf51_nvm.c
> > @@ -273,6 +273,13 @@ static const MemoryRegionOps io_ops = {
> >          .endianness = DEVICE_LITTLE_ENDIAN,
> >  };
> >
> > +static uint64_t flash_read(void *opaque, hwaddr offset, unsigned size)
> > +{
> > +    NRF51NVMState *s = NRF51_NVM(opaque);
> > +
> > +    assert(offset + size <= s->flash_size);
> > +    return ldl_le_p(s->storage + offset);
> > +}
>
> The 'flash_ops' is for ROM, though I don't see where it calls
> 'memory_region_rom_device_set_romd'
> to ROMD, so this MR is in MMIO mode and it needs a read callback.

I think that 'romd mode' (ie reads-go-directly-to-RAM) is
the default: memory_region_initfn() sets romd_mode to true.
So unless the device actively calls memory_region_rom_device_set_romd(mr, false)
then the read callback can't be reached.

thanks
-- PMM
Philippe Mathieu-Daudé July 21, 2020, 8:33 a.m. UTC | #7 
On 7/21/20 8:47 AM, P J P wrote:
> +-- On Thu, 16 Jul 2020, Peter Maydell wrote --+
> | > P J P <ppandit@redhat.com> ���2020���6���25��������� ������3:01���������
> | > > +static uint64_t flash_read(void *opaque, hwaddr offset, unsigned size)
> | > > +{
> | > > +    NRF51NVMState *s = NRF51_NVM(opaque);
> | > > +
> | > > +    assert(offset + size <= s->flash_size);
> | > > +    return ldl_le_p(s->storage + offset);
> | > > +}
> | >
> | > The 'flash_ops' is for ROM, though I don't see where it calls 
> | > 'memory_region_rom_device_set_romd' to ROMD, so this MR is in MMIO mode 
> | > and it needs a read callback.
> | 
> | I think that 'romd mode' (ie reads-go-directly-to-RAM) is the default: 
> | memory_region_initfn() sets romd_mode to true. So unless the device actively 
> | calls memory_region_rom_device_set_romd(mr, false) then the read callback 
> | can't be reached.
> 
> So, we go with g_assert_not_reached() ? We seem to have differing opinions 
> about these callbacks.

- Callback missing because we neglected to implement the
  hardware behavior:

  => qemu_log_mask(LOG_UNIMP, ...)

- Callback missing because the access is illegal on hardware
  (write on read-only register, read on write-only register):

  => qemu_log_mask(LOG_GUEST_ERROR, ...)

- Impossible situation unrelated to the hardware/guest behavior
  (problem in QEMU design)

  => g_assert_not_reached()


Note, when we runs QEMU with LOG_UNIMP/LOG_GUEST_ERROR enabled,
we are usually interested in what address the guest is accessing,
and in the write case, what value is written.

> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
>
Prasad Pandit July 21, 2020, 9:48 a.m. UTC | #8 
+-- On Tue, 21 Jul 2020, Philippe Mathieu-Daudé wrote --+
| On 7/21/20 8:47 AM, P J P wrote:
| > +-- On Thu, 16 Jul 2020, Peter Maydell wrote --+
| > | > The 'flash_ops' is for ROM, though I don't see where it calls 
| > | > 'memory_region_rom_device_set_romd' to ROMD, so this MR is in MMIO 
| > | > mode and it needs a read callback.
| > | 
| > | I think that 'romd mode' (ie reads-go-directly-to-RAM) is the default: 
| > | memory_region_initfn() sets romd_mode to true. So unless the device 
| > | actively calls memory_region_rom_device_set_romd(mr, false) then the 
| > | read callback can't be reached.
| > 
| > So, we go with g_assert_not_reached() ? We seem to have differing opinions 
| > about these callbacks.
| 
| - Callback missing because we neglected to implement the
|   hardware behavior:
| 
|   => qemu_log_mask(LOG_UNIMP, ...)
| 
| - Callback missing because the access is illegal on hardware
|   (write on read-only register, read on write-only register):
| 
|   => qemu_log_mask(LOG_GUEST_ERROR, ...)
| 
| - Impossible situation unrelated to the hardware/guest behavior
|   (problem in QEMU design)
| 
|   => g_assert_not_reached()
| 
| Note, when we runs QEMU with LOG_UNIMP/LOG_GUEST_ERROR enabled,
| we are usually interested in what address the guest is accessing,
| and in the write case, what value is written.

Okay, preparing a revised patch series.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
diff mbox series

Patch

diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c
index f2283c1a8d..8000ed530a 100644
--- a/hw/nvram/nrf51_nvm.c
+++ b/hw/nvram/nrf51_nvm.c
@@ -273,6 +273,13 @@  static const MemoryRegionOps io_ops = {
         .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static uint64_t flash_read(void *opaque, hwaddr offset, unsigned size)
+{
+    NRF51NVMState *s = NRF51_NVM(opaque);
+
+    assert(offset + size <= s->flash_size);
+    return ldl_le_p(s->storage + offset);
+}
 
 static void flash_write(void *opaque, hwaddr offset, uint64_t value,
         unsigned int size)
@@ -300,6 +307,7 @@  static void flash_write(void *opaque, hwaddr offset, uint64_t value,
 
 
 static const MemoryRegionOps flash_ops = {
+    .read = flash_read,
     .write = flash_write,
     .valid.min_access_size = 4,
     .valid.max_access_size = 4,