Re: [PATCH 0/6] Add various undefined MMIO r/w functions

2020-06-17 Thread P J P 
+-- On Wed, 17 Jun 2020, Paolo Bonzini wrote --+
| On 17/06/20 15:20, Philippe Mathieu-Daudé wrote:
| > On 6/17/20 3:06 PM, Alex Williamson wrote:
| >> On Wed, 17 Jun 2020 16:39:56 +1000
| >> David Gibson  wrote:
| >>> Hrm.  If this is such a common problem, maybe we should just add a NULL 
| >>> check in the common paths.
| >>
| >> +1, clearly the behavior is already expected.  Thanks,
| >
| > 20 months ago Peter suggested:
| > 
| > "assert that every MemoryRegionOps has pointers to callbacks
| >  in it, when it is registered in memory_region_init_io() and
| >  memory_region_init_rom_device_nomigrate()."
| > 
| > https://www.mail-archive.com/qemu-devel@nongnu.org/msg573310.html
| > 
| > Li Qiang refers to this post from Paolo:
| > 
| >>  static const MemoryRegionOps notdirty_mem_ops = {
| >> +.read = notdirty_mem_read,
| >>  .write = notdirty_mem_write,
| >>  .valid.accepts = notdirty_mem_accepts,
| >>  .endianness = DEVICE_NATIVE_ENDIAN,
| > 
| > "This cannot happen, since TLB_NOTDIRTY is only added
| >  to the addr_write member (see accel/tcg/cputlb.c)."
| 
| I'm now okay with asserting it, as long as notdirty_mem_read abort()s.

Okay, I'm preparing a revised patch.

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

Re: [PATCH 0/6] Add various undefined MMIO r/w functions

2020-06-17 Thread Paolo Bonzini 
On 17/06/20 15:20, Philippe Mathieu-Daudé wrote:
> On 6/17/20 3:06 PM, Alex Williamson wrote:
>> On Wed, 17 Jun 2020 16:39:56 +1000
>> David Gibson  wrote:
>>
>>> On Wed, Jun 17, 2020 at 11:09:27AM +0530, P J P wrote:
 From: Prasad J Pandit 

 Hello,

 This series adds various undefined MMIO read/write functions
 to avoid potential guest crash via a NULL pointer dereference.  
>>>
>>> Hrm.  If this is such a common problem, maybe we should just add a
>>> NULL check in the common paths.
>>
>> +1, clearly the behavior is already expected.  Thanks,
> 
> 20 months ago Peter suggested:
> 
> "assert that every MemoryRegionOps has pointers to callbacks
>  in it, when it is registered in memory_region_init_io() and
>  memory_region_init_rom_device_nomigrate()."
> 
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg573310.html
> 
> Li Qiang refers to this post from Paolo:
> 
>>  static const MemoryRegionOps notdirty_mem_ops = {
>> +.read = notdirty_mem_read,
>>  .write = notdirty_mem_write,
>>  .valid.accepts = notdirty_mem_accepts,
>>  .endianness = DEVICE_NATIVE_ENDIAN,
> 
> "This cannot happen, since TLB_NOTDIRTY is only added
>  to the addr_write member (see accel/tcg/cputlb.c)."

I'm now okay with asserting it, as long as notdirty_mem_read abort()s.

Paolo

Re: [PATCH 0/6] Add various undefined MMIO r/w functions

2020-06-17 Thread Philippe Mathieu-Daudé 
On 6/17/20 4:05 PM, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé  writes:
> 
>> On 6/17/20 3:06 PM, Alex Williamson wrote:
>>> On Wed, 17 Jun 2020 16:39:56 +1000
>>> David Gibson  wrote:
>>>
 On Wed, Jun 17, 2020 at 11:09:27AM +0530, P J P wrote:
> From: Prasad J Pandit 
>
> Hello,
>
> This series adds various undefined MMIO read/write functions
> to avoid potential guest crash via a NULL pointer dereference.  

 Hrm.  If this is such a common problem, maybe we should just add a
 NULL check in the common paths.
>>>
>>> +1, clearly the behavior is already expected.  Thanks,
>>
>> 20 months ago Peter suggested:
>>
>> "assert that every MemoryRegionOps has pointers to callbacks
>>  in it, when it is registered in memory_region_init_io() and
>>  memory_region_init_rom_device_nomigrate()."
>>
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg573310.html
>>
>> Li Qiang refers to this post from Paolo:
>>
>>>  static const MemoryRegionOps notdirty_mem_ops = {
>>> +.read = notdirty_mem_read,
>>>  .write = notdirty_mem_write,
>>>  .valid.accepts = notdirty_mem_accepts,
>>>  .endianness = DEVICE_NATIVE_ENDIAN,
>>
>> "This cannot happen, since TLB_NOTDIRTY is only added
>>  to the addr_write member (see accel/tcg/cputlb.c)."
>>
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg561345.html
> 
> What about catching it in memory_region_dispatch_write:
> 
> if (mr->ops->write) {
> return access_with_adjusted_size(addr, , size,
>  mr->ops->impl.min_access_size,
>  mr->ops->impl.max_access_size,
>  memory_region_write_accessor, mr,
>  attrs);
> } else if (mr->ops->write_with_attrs) {
> return
> access_with_adjusted_size(addr, , size,
>   mr->ops->impl.min_access_size,
>   mr->ops->impl.max_access_size,
>   memory_region_write_with_attrs_accessor,
>   mr, attrs);
> } else {
> qemu_log_mask(LOG_UNIMP|LOG_GUEST_ERROR, "%s: %s un-handled write\n",
>   __func__, mr->name);

The problem is what return value to return...
MEMTX_OK/MEMTX_ERROR/MEMTX_DECODE_ERROR? This is very
device-specific and can't be decided here for all the
cases.

Better to abort() and fix each device?

> }
> 
>

Re: [PATCH 0/6] Add various undefined MMIO r/w functions

2020-06-17 Thread Alex Bennée 


Philippe Mathieu-Daudé  writes:

> On 6/17/20 3:06 PM, Alex Williamson wrote:
>> On Wed, 17 Jun 2020 16:39:56 +1000
>> David Gibson  wrote:
>> 
>>> On Wed, Jun 17, 2020 at 11:09:27AM +0530, P J P wrote:
 From: Prasad J Pandit 

 Hello,

 This series adds various undefined MMIO read/write functions
 to avoid potential guest crash via a NULL pointer dereference.  
>>>
>>> Hrm.  If this is such a common problem, maybe we should just add a
>>> NULL check in the common paths.
>> 
>> +1, clearly the behavior is already expected.  Thanks,
>
> 20 months ago Peter suggested:
>
> "assert that every MemoryRegionOps has pointers to callbacks
>  in it, when it is registered in memory_region_init_io() and
>  memory_region_init_rom_device_nomigrate()."
>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg573310.html
>
> Li Qiang refers to this post from Paolo:
>
>>  static const MemoryRegionOps notdirty_mem_ops = {
>> +.read = notdirty_mem_read,
>>  .write = notdirty_mem_write,
>>  .valid.accepts = notdirty_mem_accepts,
>>  .endianness = DEVICE_NATIVE_ENDIAN,
>
> "This cannot happen, since TLB_NOTDIRTY is only added
>  to the addr_write member (see accel/tcg/cputlb.c)."
>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg561345.html

What about catching it in memory_region_dispatch_write:

if (mr->ops->write) {
return access_with_adjusted_size(addr, , size,
 mr->ops->impl.min_access_size,
 mr->ops->impl.max_access_size,
 memory_region_write_accessor, mr,
 attrs);
} else if (mr->ops->write_with_attrs) {
return
access_with_adjusted_size(addr, , size,
  mr->ops->impl.min_access_size,
  mr->ops->impl.max_access_size,
  memory_region_write_with_attrs_accessor,
  mr, attrs);
} else {
qemu_log_mask(LOG_UNIMP|LOG_GUEST_ERROR, "%s: %s un-handled write\n",
  __func__, mr->name);
}


-- 
Alex Bennée

Re: [PATCH 0/6] Add various undefined MMIO r/w functions

2020-06-17 Thread Philippe Mathieu-Daudé 
On 6/17/20 3:06 PM, Alex Williamson wrote:
> On Wed, 17 Jun 2020 16:39:56 +1000
> David Gibson  wrote:
> 
>> On Wed, Jun 17, 2020 at 11:09:27AM +0530, P J P wrote:
>>> From: Prasad J Pandit 
>>>
>>> Hello,
>>>
>>> This series adds various undefined MMIO read/write functions
>>> to avoid potential guest crash via a NULL pointer dereference.  
>>
>> Hrm.  If this is such a common problem, maybe we should just add a
>> NULL check in the common paths.
> 
> +1, clearly the behavior is already expected.  Thanks,

20 months ago Peter suggested:

"assert that every MemoryRegionOps has pointers to callbacks
 in it, when it is registered in memory_region_init_io() and
 memory_region_init_rom_device_nomigrate()."

https://www.mail-archive.com/qemu-devel@nongnu.org/msg573310.html

Li Qiang refers to this post from Paolo:

>  static const MemoryRegionOps notdirty_mem_ops = {
> +.read = notdirty_mem_read,
>  .write = notdirty_mem_write,
>  .valid.accepts = notdirty_mem_accepts,
>  .endianness = DEVICE_NATIVE_ENDIAN,

"This cannot happen, since TLB_NOTDIRTY is only added
 to the addr_write member (see accel/tcg/cputlb.c)."

https://www.mail-archive.com/qemu-devel@nongnu.org/msg561345.html

Re: [PATCH 0/6] Add various undefined MMIO r/w functions

2020-06-17 Thread Alex Williamson 
On Wed, 17 Jun 2020 16:39:56 +1000
David Gibson  wrote:

> On Wed, Jun 17, 2020 at 11:09:27AM +0530, P J P wrote:
> > From: Prasad J Pandit 
> > 
> > Hello,
> > 
> > This series adds various undefined MMIO read/write functions
> > to avoid potential guest crash via a NULL pointer dereference.  
> 
> Hrm.  If this is such a common problem, maybe we should just add a
> NULL check in the common paths.

+1, clearly the behavior is already expected.  Thanks,

Alex

Re: [PATCH 0/6] Add various undefined MMIO r/w functions

2020-06-17 Thread David Gibson 
On Wed, Jun 17, 2020 at 11:09:27AM +0530, P J P wrote:
> From: Prasad J Pandit 
> 
> Hello,
> 
> This series adds various undefined MMIO read/write functions
> to avoid potential guest crash via a NULL pointer dereference.

Hrm.  If this is such a common problem, maybe we should just add a
NULL check in the common paths.

> 
> ex. -> 
> https://git.qemu.org/?p=qemu.git;a=commit;h=bb15013ef34617eb1344f5276292cadd326c21b2
> 
> Thank you.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH 0/6] Add various undefined MMIO r/w functions

2020-06-17 Thread no-reply 
Patchew URL: 
https://patchew.org/QEMU/20200617053934.122642-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/guest-agent-command-state.o
  CC  qga/main.o
  CC  qga/commands-posix.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)
  CC  qga/channel-posix.o
  CC  qga/qapi-generated/qga-qapi-types.o
  CC  qga/qapi-generated/qga-qapi-visit.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)
  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
  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
/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/multiboot.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-img
  AS  pc-bios/optionrom/linuxboot.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)
  CC  pc-bios/optionrom/linuxboot_dma.o
  AS  pc-bios/optionrom/kvmvapic.o
  AS  pc-bios/optionrom/pvh.o
  CC  pc-bios/optionrom/pvh_main.o
  BUILD   pc-bios/optionrom/multiboot.img
  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)
  BUILD   pc-bios/optionrom/linuxboot.img
  BUILD   pc-bios/optionrom/linuxboot_dma.img
  BUILD   pc-bios/optionrom/kvmvapic.img
---
  LINKfsdev/virtfs-proxy-helper
  BUILD   pc-bios/optionrom/linuxboot.raw
  BUILD   pc-bios/optionrom/multiboot.raw
/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)
  BUILD   pc-bios/optionrom/kvmvapic.raw
  SIGNpc-bios/optionrom/linuxboot.bin
  LINKscsi/qemu-pr-helper
  SIGNpc-bios/optionrom/linuxboot_dma.bin
  SIGNpc-bios/optionrom/kvmvapic.bin
  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)
  SIGNpc-bios/optionrom/multiboot.bin
  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