There is an issue when accessing an alias memory region via the
memory_region_dispatch_read() / memory_region_dispatch_write()
calls:

The memory_region_init_alias() flow is:

  memory_region_init_alias()
  -> memory_region_init()
     -> object_initialize(TYPE_MEMORY_REGION)
        -> memory_region_initfn()
           -> mr->ops = &unassigned_mem_ops;

Later when accessing the alias, the memory_region_dispatch_read()
flow is:

  memory_region_dispatch_read()
  -> memory_region_access_valid(mr)
     -> mr->ops->valid.accepts()
        -> unassigned_mem_accepts()
        <- false
     <- false
   <- MEMTX_DECODE_ERROR

The caller gets a MEMTX_DECODE_ERROR while the access is OK.

Fix by directly dispatching aliases accesses to its origin region.

Fixes: 2cdfcf272d ("memory: assign MemoryRegionOps to all regions")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 softmmu/memory.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index af25987518..651705b7d1 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1405,6 +1405,10 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
     unsigned size = memop_size(op);
     MemTxResult r;
 
+    if (mr->alias) {
+        addr += mr->alias_offset;
+        mr = mr->alias;
+    }
     if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
         *pval = unassigned_mem_read(mr, addr, size);
         return MEMTX_DECODE_ERROR;
@@ -1449,6 +1453,10 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
 {
     unsigned size = memop_size(op);
 
+    if (mr->alias) {
+        addr += mr->alias_offset;
+        mr = mr->alias;
+    }
     if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
         unassigned_mem_write(mr, addr, data, size);
         return MEMTX_DECODE_ERROR;
-- 
2.21.3

On 16/08/20 19:30, Philippe Mathieu-Daudé wrote:
> There is an issue when accessing an alias memory region via the
> memory_region_dispatch_read() / memory_region_dispatch_write()
> calls:
> 
> The memory_region_init_alias() flow is:
> 
>   memory_region_init_alias()
>   -> memory_region_init()
>      -> object_initialize(TYPE_MEMORY_REGION)
>         -> memory_region_initfn()
>            -> mr->ops = &unassigned_mem_ops;
> 
> Later when accessing the alias, the memory_region_dispatch_read()
> flow is:
> 
>   memory_region_dispatch_read()
>   -> memory_region_access_valid(mr)
>      -> mr->ops->valid.accepts()
>         -> unassigned_mem_accepts()
>         <- false
>      <- false
>    <- MEMTX_DECODE_ERROR
> 
> The caller gets a MEMTX_DECODE_ERROR while the access is OK.

What is the path that leads to this call?

> Fix by directly dispatching aliases accesses to its origin region.
> 
> Fixes: 2cdfcf272d ("memory: assign MemoryRegionOps to all regions")

I don't think the "Fixes" is okay because you'd have gotten a different
bug before.

> +    if (mr->alias) {
> +        addr += mr->alias_offset;
> +        mr = mr->alias;
> +    }

Also, I think this would have to be a while loop.

Paolo

On 8/17/20 6:27 PM, Paolo Bonzini wrote:
> On 16/08/20 19:30, Philippe Mathieu-Daudé wrote:
>> There is an issue when accessing an alias memory region via the
>> memory_region_dispatch_read() / memory_region_dispatch_write()
>> calls:
>>
>> The memory_region_init_alias() flow is:
>>
>>   memory_region_init_alias()
>>   -> memory_region_init()
>>      -> object_initialize(TYPE_MEMORY_REGION)
>>         -> memory_region_initfn()
>>            -> mr->ops = &unassigned_mem_ops;
>>
>> Later when accessing the alias, the memory_region_dispatch_read()
>> flow is:
>>
>>   memory_region_dispatch_read()
>>   -> memory_region_access_valid(mr)
>>      -> mr->ops->valid.accepts()
>>         -> unassigned_mem_accepts()
>>         <- false
>>      <- false
>>    <- MEMTX_DECODE_ERROR
>>
>> The caller gets a MEMTX_DECODE_ERROR while the access is OK.
> 
> What is the path that leads to this call?

Using the interleaver from:
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg03680.html

#2  0x8162f7b6 in unassigned_mem_read (opaque=0x82ac7330, addr=0,
size=1) at softmmu/memory.c:1261
#3  0x8162fc2f in memory_region_dispatch_read (mr=0x82ac7330, addr=0,
pval=0x7ffe24b9cc08, op=MO_8, attrs=...) at softmmu/memory.c:1417
#4  0x8175488b in interleaver_read (opaque=0x82b9e530, offset=0,
data=0x7ffe24b9cc08, size=1, attrs=...) at hw/misc/interleaver.c:76
#5  0x8162cbdc in memory_region_read_with_attrs_accessor (mr=0x82b9e850,
addr=0, value=0x7ffe24b9cd90, size=1, shift=0, mask=255, attrs=...) at
softmmu/memory.c:456
#6  0x8162cfab in access_with_adjusted_size (addr=0,
value=0x7ffe24b9cd90, size=4, access_size_min=1, access_size_max=1,
access_fn=
    0x8162cb7c <memory_region_read_with_attrs_accessor>, mr=0x82b9e850,
attrs=...) at softmmu/memory.c:544
#7  0x8162fb98 in memory_region_dispatch_read1 (mr=0x82b9e850, addr=0,
pval=0x7ffe24b9cd90, size=4, attrs=...) at softmmu/memory.c:1395
#8  0x8162fc5a in memory_region_dispatch_read (mr=0x82b9e850, addr=0,
pval=0x7ffe24b9cd90, op=MO_32, attrs=...) at softmmu/memory.c:1421
#9  0x8153012b in flatview_read_continue (fv=0x82bd0d10, addr=320897024,
attrs=..., ptr=0x7ffe24b9cea0, len=4, addr1=0, l=4, mr=0x82b9e850) at
exec.c:3239
#10 0x8153027e in flatview_read (fv=0x82bd0d10, addr=320897024,
attrs=..., buf=0x7ffe24b9cea0, len=4) at exec.c:3278
#11 0x81530307 in address_space_read_full (as=0x81ec1ac0
<address_space_memory>, addr=320897024, attrs=..., buf=0x7ffe24b9cea0,
len=4) at exec.c:3291
#12 0x8163761e in address_space_read (len=4, buf=0x7ffe24b9cea0,
attrs=..., addr=320897024, as=0x81ec1ac0 <address_space_memory>) at
include/exec/memory.h:2420
#13 qtest_process_command (chr=0x81edcd00 <qtest_chr>, words=0x82be2b30)
at softmmu/qtest.c:495
#14 0x8163877b in qtest_process_inbuf (chr=0x81edcd00 <qtest_chr>,
inbuf=0x82a65220) at softmmu/qtest.c:724
#15 0x8163880c in qtest_read (opaque=0x81edcd00 <qtest_chr>,
buf=0x7ffe24b9d1f0 "readl 0x13208000\n 0x76\n", size=17) at
softmmu/qtest.c:736

> 
>> Fix by directly dispatching aliases accesses to its origin region.
>>
>> Fixes: 2cdfcf272d ("memory: assign MemoryRegionOps to all regions")
> 
> I don't think the "Fixes" is okay because you'd have gotten a different
> bug before.

OK I'll reword.

> 
>> +    if (mr->alias) {
>> +        addr += mr->alias_offset;
>> +        mr = mr->alias;
>> +    }
> 
> Also, I think this would have to be a while loop.

I haven't thought about this case! I'll add a test for it :)

> 
> Paolo
> 
>