| Message ID | 1453732190-13416-1-git-send-email-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25/01/2016 15:29, P J P wrote: > diff --git a/exec.c b/exec.c > index 0a4a0c5..98d97d3 100644 > --- a/exec.c > +++ b/exec.c > @@ -375,7 +375,7 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x > static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write) > { > if (memory_region_is_ram(mr)) { > - return !(is_write && mr->readonly); > + return (is_write && !mr->readonly); Putting the various cases in a table: Read or write? Readonly? Old New Read Yes T F Read No T F Write Yes F F Write No T T This patch changes behavior for reads (is_write=false). For address_space_read, this makes them go through a path that is at least 100 times slower (memory_region_dispatch_read instead of just a memcpy). For address_space_map, it probably breaks everything that expects a single block of RAM to be mapped in a single step, for example virtio. So, how was this tested, and how can the bug be triggered? Paolo > } > if (memory_region_is_romd(mr)) { > return !is_write; >
+-- On Mon, 25 Jan 2016, Paolo Bonzini wrote --+
| > static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
| > {
| > if (memory_region_is_ram(mr)) {
| > - return !(is_write && mr->readonly);
| > + return (is_write && !mr->readonly);
|
| Read or write? Readonly? Old New
| Read Yes T F
| Read No T F
| Write Yes F F
| Write No T T
|
| This patch changes behavior for reads (is_write=false). For
| address_space_read, this makes them go through a path that is at least
| 100 times slower (memory_region_dispatch_read instead of just a memcpy).
| For address_space_map, it probably breaks everything that expects a
| single block of RAM to be mapped in a single step, for example virtio.
|
| So, how was this tested, and how can the bug be triggered?
The bug was triggered if 'addr' in 'read_dword()' is set by user(ex.
0xffffffff). The MemoryRegion section(*mr) could point to host memory area,
which is then copied by memcpy(2) call. This leads to the said issue. The
patch was tested using gdb(1).
read_dword
-> pci_dma_read
-> pci_dma_rw
-> dma_memory_rw
-> dma_memory_rw_relaxed
-> address_space_rw
-> memcpy
Thank you.
--
Prasad J Pandit / Red Hat Product Security
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
On 25/01/2016 19:19, P J P wrote: > +-- On Mon, 25 Jan 2016, Paolo Bonzini wrote --+ > | > static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write) > | > { > | > if (memory_region_is_ram(mr)) { > | > - return !(is_write && mr->readonly); > | > + return (is_write && !mr->readonly); > | > | Read or write? Readonly? Old New > | Read Yes T F > | Read No T F > | Write Yes F F > | Write No T T > | > | This patch changes behavior for reads (is_write=false). For > | address_space_read, this makes them go through a path that is at least > | 100 times slower (memory_region_dispatch_read instead of just a memcpy). > | For address_space_map, it probably breaks everything that expects a > | single block of RAM to be mapped in a single step, for example virtio. > | > | So, how was this tested, and how can the bug be triggered? > > The bug was triggered if 'addr' in 'read_dword()' is set by user(ex. > 0xffffffff). The MemoryRegion section(*mr) could point to host memory area, > which is then copied by memcpy(2) call. This should be handled correctly by address_space_translate_internal: if (memory_region_is_ram(mr)) { diff = int128_sub(section->size, int128_make64(addr)); *plen = int128_get64(int128_min(diff, int128_make64(*plen))); } ... then, on return from address_space_translate, l will be 1: e.g. section->size = 0x100000000, addr = 0xffffffff; diff = 1; *plen = min(diff, *plen) = min(1, 4) = 1 > The patch was tested using gdb(1). You also have to test that the patch doesn't break other code. It's not enough to test that it solves your problem. Paolo This bit hasn't changed since 2.4.1.
Hello Paolo,
+-- On Mon, 25 Jan 2016, Paolo Bonzini wrote --+
| This should be handled correctly by address_space_translate_internal:
|
| if (memory_region_is_ram(mr)) {
| diff = int128_sub(section->size, int128_make64(addr));
| *plen = int128_get64(int128_min(diff, int128_make64(*plen)));
| }
|
| ... then, on return from address_space_translate, l will be 1:
|
| e.g. section->size = 0x100000000, addr = 0xffffffff;
| diff = 1;
| *plen = min(diff, *plen) = min(1, 4) = 1
I see. Sorry, I think the issue affects versions <= v2.3.1 and not v2.4.x.
v2.3.x series seems to be missing this patch
-> http://git.qemu.org/?p=qemu.git;a=commit;h=23820dbfc79d1c9dce090b4c555994f2bb6a69b3
which avoids setting '*plen' to its earlier value. I'll send it to the -stable
list.
| You also have to test that the patch doesn't break other code. It's not
| enough to test that it solves your problem.
Right, I'll run the tests/* going forward.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
diff --git a/exec.c b/exec.c index 0a4a0c5..98d97d3 100644 --- a/exec.c +++ b/exec.c @@ -375,7 +375,7 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write) { if (memory_region_is_ram(mr)) { - return !(is_write && mr->readonly); + return (is_write && !mr->readonly); } if (memory_region_is_romd(mr)) { return !is_write;