Re: [PATCH] memory: Initialize MemoryRegionOps for RAM memory regions

[Philippe Mathieu-Daudé]

On 8/17/20 1:25 PM, P J P wrote:
> +-- On Mon, 17 Aug 2020, Philippe Mathieu-Daudé wrote --+
> | On 8/16/20 8:26 PM, Philippe Mathieu-Daudé wrote:
> | > There is an issue when using memory_region_dispatch_read() or
> | > memory_region_dispatch_write() on RAM memory regions.
> | > 
> | > RAM memory regions are initialized as:
> | > 
> | >   memory_region_init_ram()
> | >   -> memory_region_init_ram_nomigrate()
> | >      -> memory_region_init_ram_shared_nomigrate()
> | >         -> 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_dispatch_read1()
> | >      -> if (mr->ops->read) { ... }
> | >                    ^^^^^^
> | >                    NULL deref as unassigned_mem_ops.read is NULL.

[*]

> | > 
> | >   memory_region_dispatch_write()
> | >   -> if (mr->ops->write) { ... }
> | >                 ^^^^^^^
> | >                 NULL deref as unassigned_mem_ops.read is NULL.
> 
> * This check should pass/fail as normal I think. NULL dereference would 
> happen 
>   on invoking mr->ops->read/write() call.
> 
> * When mr->ops->read/write==NULL, the dispatch_read/write function would go 
> to 
>   the else section to call read/write_with_attrs()

Yes, this is where it crashes. The patch description (code flow)
is incorrect here and in [*].

I'll respin, thanks!

> 
>     const MemoryRegionOps unassigned_mem_ops = {                              
>       
>       .valid.accepts = unassigned_mem_accepts,                                
>     
>       .endianness = DEVICE_NATIVE_ENDIAN,                                     
>     
>     };
> 
>   Maybe we define read/write_with_attrs in 'unassigned_mem_ops' above?
> 
> 
> | > Fix by initializing the MemoryRegionOps to ram_device_mem_ops,
> | > this way the memory accesses are properly dispatched using
> | > memory_region_ram_device_read() / memory_region_ram_device_write().
> | > 
> | > Fixes: 4a2e242bbb ("memory: Don't use memcpy for ram_device regions")
> 
> Yes, this should be fine too.
> 
> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
>