[Qemu-devel] [PATCH for v2.4.1] exec: fix a glitch in checking dma r/w access

[Qemu-devel] [PATCH for v2.4.1] exec: fix a glitch in checking dma r/w access

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

While checking r/w access in 'memory_access_is_direct' routine
a glitch in the expression leads to segmentation fault while
performing dma read operation.

Reported-by: Donghai Zdh <donghai.zdh@alibaba-inc.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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;
-- 
2.5.0

Re: [Qemu-devel] [PATCH for v2.4.1] exec: fix a glitch in checking dma r/w access

[Paolo Bonzini]

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;
> 

Re: [Qemu-devel] [PATCH for v2.4.1] exec: fix a glitch in checking dma r/w access

[P J P]

+-- 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

Re: [Qemu-devel] [PATCH for v2.4.1] exec: fix a glitch in checking dma r/w access

[Paolo Bonzini]

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.

Re: [Qemu-devel] [PATCH for v2.4.1] exec: fix a glitch in checking dma r/w access

[P J P]

  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