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.