Re: [PATCH v1] memory: assert MemoryRegionOps callbacks are defined

2020-06-19 Thread Paolo Bonzini 
On 19/06/20 12:48, P J P wrote:
> | - hw/nvram/nrf51_nvm.c's flash_ops which is fixed if ROMD regions are
> | changed not to require a read callback.
> | 
> | - designware_pci_host_msi_ops which is broken and should have a dummy
> | read callback.
> 
> 
> ie. we add these routines along with the assertions here, right?
> 
> Earlier patch series adds routines for nrf51_nvm and designware. I'll add 
> r/w routines for tz_ppc_dummy_ops.

In this case, please add "Based-on: " to the patch so that
it's clear to the reviewer that there are dependencies.

Thanks!

Paolo

Re: [PATCH v1] memory: assert MemoryRegionOps callbacks are defined

2020-06-19 Thread P J P 
+-- On Thu, 18 Jun 2020, Paolo Bonzini wrote --+
| On 18/06/20 15:12, Alex Bennée wrote:
| > If you look at memory_region_dispatch_write you can see that 
| > mr->ops->write being empty is acceptable because it implies 
| > mr->ops->write_with_attrs is set instead. I think the same is true for 
| > read so I think you need something more like:
| > 
| >  assert(ops->read || ops->read_with_attrs);
| >  assert(ops->write || ops->write_with_attrs);
| 
| Also, !ops is acceptable since you have a couple lines below:
| 
| mr->ops = ops ? ops : _mem_ops;

  Oops! :(
 
| Needless to say, this is something that the submitter should have done,
| not the reviewer.

  Sorry, I should've considered full effects of the patch, not just fixing the 
NULL dereference issue at hand. Sorry about the haste, I'll be careful in 
future.


| > Also doesn't this break a load of running stuff without fixes for all the 
| > various missing bits? How far does make check-acceptance get?

I tried to run '$ make check-acceptance', it's breaking with time-out error.

urllib.error.URLError: 
urllib.error.URLError: 
...
make: *** [../qemu/tests/Makefile.include:933: get-vm-image-fedora-31-ppc64le] 
Error 1


| This might actually be really close with the above assertions fixed.
| For example, commit 08565552f7 ("cputlb: Move NOTDIRTY handling from I/O
| path to TLB path", 2019-09-25) got rid of io_mem_notdirty.
| 
| The only cases I found with "git grep" are:
| 
| - tz_ppc_dummy_ops which is broken and should just use NULL ops
| 
| - hw/nvram/nrf51_nvm.c's flash_ops which is fixed if ROMD regions are
| changed not to require a read callback.
| 
| - designware_pci_host_msi_ops which is broken and should have a dummy
| read callback.


ie. we add these routines along with the assertions here, right?

Earlier patch series adds routines for nrf51_nvm and designware. I'll add 
r/w routines for tz_ppc_dummy_ops.

Thank you so much for the review.

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

Re: [PATCH v1] memory: assert MemoryRegionOps callbacks are defined

2020-06-18 Thread Paolo Bonzini 
On 18/06/20 16:29, Peter Maydell wrote:
>> - tz_ppc_dummy_ops which is broken and should just use NULL ops
> Why is it broken? The intention is to create a MemoryRegion
> which asserts if it's ever used (because it is a QEMU bug if
> board code ever actually maps that MemoryRegion into anything).
> NULL ops doesn't do that, it creates a MemoryRegion whose accesses
> all report an error to the guest.

I meant "broken by the assertions" but yeah the NULL ops suggestion is
wrong. I misread tz_ppc_dummy_accepts as "return false" (which is
exactly what you get from NULL ops) instead of "g_assert_not_reached()".

Paolo

Re: [PATCH v1] memory: assert MemoryRegionOps callbacks are defined

2020-06-18 Thread Philippe Mathieu-Daudé 
On 6/18/20 3:12 PM, Alex Bennée wrote:
> 
> P J P  writes:
> 
>> From: Prasad J Pandit 
>>
>> When registering a MemoryRegionOps object, assert that its
>> read/write callback methods are defined. This avoids potential
>> guest crash via a NULL pointer dereference.
>>
>> Suggested-by: Peter Maydell 
>> Signed-off-by: Prasad J Pandit 
>> ---
>>  memory.c | 5 +
>>  1 file changed, 5 insertions(+)
>>
>> Update v1: add assert while registering MemoryRegionOps
>>   -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg05187.html
>>
>> diff --git a/memory.c b/memory.c
>> index 91ceaf9fcf..6e94fd5958 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -1495,6 +1495,9 @@ void memory_region_init_io(MemoryRegion *mr,
>> const char *name,
>> uint64_t size)
>>  {
>> +assert(ops);
>> +assert(ops->read);
>> +assert(ops->write);
> 
> If you look at memory_region_dispatch_write you can see that
> mr->ops->write being empty is acceptable because it implies
> mr->ops->write_with_attrs is set instead. I think the same is true for
> read so I think you need something more like:
> 
>  assert(ops->read || ops->read_with_attrs);
>  assert(ops->write || ops->write_with_attrs);
>  
> 
>>  memory_region_init(mr, owner, name, size);
>>  mr->ops = ops ? ops : _mem_ops;
>>  mr->opaque = opaque;
>> @@ -1674,6 +1677,8 @@ void 
>> memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
>>  {
>>  Error *err = NULL;
>>  assert(ops);
>> +assert(ops->read);
>> +assert(ops->write);
> 
> Do ROM devices need a ->write function?

This is how you put the device in I/O mode, isn't it?

> 
> Also doesn't this break a load of running stuff without fixes for all
> the various missing bits? How far does make check-acceptance get?
> 
>>  memory_region_init(mr, owner, name, size);
>>  mr->ops = ops;
>>  mr->opaque = opaque;
> 
>

Re: [PATCH v1] memory: assert MemoryRegionOps callbacks are defined

2020-06-18 Thread Peter Maydell 
On Thu, 18 Jun 2020 at 14:35, Paolo Bonzini  wrote:
> - tz_ppc_dummy_ops which is broken and should just use NULL ops

Why is it broken? The intention is to create a MemoryRegion
which asserts if it's ever used (because it is a QEMU bug if
board code ever actually maps that MemoryRegion into anything).
NULL ops doesn't do that, it creates a MemoryRegion whose accesses
all report an error to the guest.

thanks
-- PMM

Re: [PATCH v1] memory: assert MemoryRegionOps callbacks are defined

2020-06-18 Thread Paolo Bonzini 
On 18/06/20 15:12, Alex Bennée wrote:
>>
>> @@ -1495,6 +1495,9 @@ void memory_region_init_io(MemoryRegion *mr,
>> const char *name,
>> uint64_t size)
>>  {
>> +assert(ops);
>> +assert(ops->read);
>> +assert(ops->write);
> 
> If you look at memory_region_dispatch_write you can see that
> mr->ops->write being empty is acceptable because it implies
> mr->ops->write_with_attrs is set instead. I think the same is true for
> read so I think you need something more like:
> 
>  assert(ops->read || ops->read_with_attrs);
>  assert(ops->write || ops->write_with_attrs);

Also, !ops is acceptable since you have a couple lines below:

mr->ops = ops ? ops : _mem_ops;


>> +assert(ops->read);
>> +assert(ops->write);
> 
> Do ROM devices need a ->write function?

Yes, ROMD regions are treated as I/O regions for writes.  However they
don't need a read function.

> Also doesn't this break a load of running stuff without fixes for all
> the various missing bits? How far does make check-acceptance get?

This might actually be really close with the above assertions fixed.
For example, commit 08565552f7 ("cputlb: Move NOTDIRTY handling from I/O
path to TLB path", 2019-09-25) got rid of io_mem_notdirty.

The only cases I found with "git grep" are:

- tz_ppc_dummy_ops which is broken and should just use NULL ops

- hw/nvram/nrf51_nvm.c's flash_ops which is fixed if ROMD regions are
changed not to require a read callback.

- designware_pci_host_msi_ops which is broken and should have a dummy
read callback.

Needless to say, this is something that the submitter should have done,
not the reviewer.

Paolo

Re: [PATCH v1] memory: assert MemoryRegionOps callbacks are defined

2020-06-18 Thread Alex Bennée 


P J P  writes:

> From: Prasad J Pandit 
>
> When registering a MemoryRegionOps object, assert that its
> read/write callback methods are defined. This avoids potential
> guest crash via a NULL pointer dereference.
>
> Suggested-by: Peter Maydell 
> Signed-off-by: Prasad J Pandit 
> ---
>  memory.c | 5 +
>  1 file changed, 5 insertions(+)
>
> Update v1: add assert while registering MemoryRegionOps
>   -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg05187.html
>
> diff --git a/memory.c b/memory.c
> index 91ceaf9fcf..6e94fd5958 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1495,6 +1495,9 @@ void memory_region_init_io(MemoryRegion *mr,
> const char *name,
> uint64_t size)
>  {
> +assert(ops);
> +assert(ops->read);
> +assert(ops->write);

If you look at memory_region_dispatch_write you can see that
mr->ops->write being empty is acceptable because it implies
mr->ops->write_with_attrs is set instead. I think the same is true for
read so I think you need something more like:

 assert(ops->read || ops->read_with_attrs);
 assert(ops->write || ops->write_with_attrs);
 

>  memory_region_init(mr, owner, name, size);
>  mr->ops = ops ? ops : _mem_ops;
>  mr->opaque = opaque;
> @@ -1674,6 +1677,8 @@ void 
> memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
>  {
>  Error *err = NULL;
>  assert(ops);
> +assert(ops->read);
> +assert(ops->write);

Do ROM devices need a ->write function?

Also doesn't this break a load of running stuff without fixes for all
the various missing bits? How far does make check-acceptance get?

>  memory_region_init(mr, owner, name, size);
>  mr->ops = ops;
>  mr->opaque = opaque;


-- 
Alex Bennée

Re: [PATCH v1] memory: assert MemoryRegionOps callbacks are defined

2020-06-18 Thread no-reply 
Patchew URL: 
https://patchew.org/QEMU/20200618121218.215808-1-ppan...@redhat.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC  qga/commands-posix.o
  CC  qga/channel-posix.o
  CC  qga/qapi-generated/qga-qapi-types.o
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `  CC  qga/qapi-generated/qga-qapi-visit.o
__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  CC  qga/qapi-generated/qga-qapi-commands.o
  CC  qga/qapi-generated/qga-qapi-init-commands.o
---
  GEN docs/interop/qemu-ga-ref.html
  GEN docs/interop/qemu-ga-ref.txt
  GEN docs/interop/qemu-ga-ref.7
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  AS  pc-bios/optionrom/pvh.o
  AS  pc-bios/optionrom/multiboot.o
  CC  pc-bios/optionrom/pvh_main.o
---
  SIGNpc-bios/optionrom/pvh.bin
  LINKqemu-ga
  LINKqemu-keymap
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKivshmem-client
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKivshmem-server
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-nbd
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-storage-daemon
  LINKqemu-img
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-io
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-edid
  LINKfsdev/virtfs-proxy-helper
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKscsi/qemu-pr-helper
  LINKqemu-bridge-helper
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKvirtiofsd
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from

[PATCH v1] memory: assert MemoryRegionOps callbacks are defined

2020-06-18 Thread P J P 
From: Prasad J Pandit 

When registering a MemoryRegionOps object, assert that its
read/write callback methods are defined. This avoids potential
guest crash via a NULL pointer dereference.

Suggested-by: Peter Maydell 
Signed-off-by: Prasad J Pandit 
---
 memory.c | 5 +
 1 file changed, 5 insertions(+)

Update v1: add assert while registering MemoryRegionOps
  -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg05187.html

diff --git a/memory.c b/memory.c
index 91ceaf9fcf..6e94fd5958 100644
--- a/memory.c
+++ b/memory.c
@@ -1495,6 +1495,9 @@ void memory_region_init_io(MemoryRegion *mr,
const char *name,
uint64_t size)
 {
+assert(ops);
+assert(ops->read);
+assert(ops->write);
 memory_region_init(mr, owner, name, size);
 mr->ops = ops ? ops : _mem_ops;
 mr->opaque = opaque;
@@ -1674,6 +1677,8 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion 
*mr,
 {
 Error *err = NULL;
 assert(ops);
+assert(ops->read);
+assert(ops->write);
 memory_region_init(mr, owner, name, size);
 mr->ops = ops;
 mr->opaque = opaque;
-- 
2.26.2