Re: [Qemu-devel] [PATCH] exec: Fix section_covers_addr() for sections with non-zero offset

[Paolo Bonzini]

On 21/10/2017 13:24, BALATON Zoltan wrote:
> When a section with non-0 offset_within_region field is tested to
> cover an address the offset should be taken into account as well.
> 
> This fixes a crash caused by picking the wrong memory region in
> address_space_lookup_region seen with client code accessing a device
> model that uses alias memory regions.
> 
> Signed-off-by: BALATON Zoltan <address@hidden>
> ---
> This seems to fix the problem described in
> http://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03356.html
> but I'm not completely sure about it. This seems to be introduced in
> 729633c exec: Introduce AddressSpaceDispatch.mru_section and the patch
> before that which split off section_covers_addr from phys_page_find so
> this patch also changes that caller. Is that OK to do? It appears to
> work but I don't know this part of QEMU.
> 
> Also the bug seems to be caused by section_covers_addr accepting
> sii3112.bar5 when that's the mru_section instead of picking
> sii3112.bar0 (which it picks when going through phys_page_find) when
> client code is accessing 0xc08001006 from this address map (full
> address map is at above URL):
> 
> address-space: memory
>     0000000c08000000-0000000c0800ffff (prio 0, i/o): alias isa_mmio @io 
> 0000000000000000-000000000000ffff
> 
> address-space: I/O
>   0000000000000000-000000000000ffff (prio 0, i/o): io
>     0000000000001000-0000000000001007 (prio 1, i/o): alias sii3112.bar0 
> @sii3112.bar5 0000000000000080-0000000000000087
>     0000000000001008-000000000000100b (prio 1, i/o): alias sii3112.bar1 
> @sii3112.bar5 0000000000000088-000000000000008b
>     0000000000001010-0000000000001017 (prio 1, i/o): alias sii3112.bar2 
> @sii3112.bar5 00000000000000c0-00000000000000c7
>     0000000000001018-000000000000101b (prio 1, i/o): alias sii3112.bar3 
> @sii3112.bar5 00000000000000c8-00000000000000cb
>     0000000000001020-000000000000102f (prio 1, i/o): alias sii3112.bar4 
> @sii3112.bar5 0000000000000000-000000000000000f
> 
> which this patch fixes but would the same problem happen if the
> mru_section is bar5 but bar4 is accessed? I could not reproduce that
> case but then the offset is 0 but in this case the address would be
> above 0xc08001020 and size is 0xf so they probably won't match. But
> this is only because of the size of the region. Could that mean the
> bug is caused by something else and should be fixed elsewhere?
> 
> ---
>  exec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/exec.c b/exec.c
> index db5ae23..a915817 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -370,7 +370,8 @@ static inline bool section_covers_addr(const 
> MemoryRegionSection *section,
>       * the section must cover the entire address space.
>       */
>      return int128_gethi(section->size) ||
> -           range_covers_byte(section->offset_within_address_space,
> +           range_covers_byte(section->offset_within_address_space +
> +                             section->offset_within_region,
>                               int128_getlo(section->size), addr);
>  }

Sorry, this is incorrect.  addr is an address in the address space, and
range_covers_byte checks if it is between
section->offset_within_address_space and
section->offset_within_address_space + section->size.  I am not sure how
things don't explode completely by adding section->offset_within_region
(probably it's just because section->offset_within_region is usually 0).

Paolo