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