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

[Philippe Mathieu-Daudé]

+Prasad

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.
> 
> 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")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  softmmu/memory.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index af25987518..2fce3fef2d 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1509,6 +1509,8 @@ void 
> memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
>      Error *err = NULL;
>      memory_region_init(mr, owner, name, size);
>      mr->ram = true;
> +    mr->ops = &ram_device_mem_ops;
> +    mr->opaque = mr;
>      mr->terminates = true;
>      mr->destructor = memory_region_destructor_ram;
>      mr->ram_block = qemu_ram_alloc(size, share, mr, &err);
> @@ -1533,6 +1535,8 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
>      Error *err = NULL;
>      memory_region_init(mr, owner, name, size);
>      mr->ram = true;
> +    mr->ops = &ram_device_mem_ops;
> +    mr->opaque = mr;
>      mr->terminates = true;
>      mr->destructor = memory_region_destructor_ram;
>      mr->ram_block = qemu_ram_alloc_resizeable(size, max_size, resized,
> @@ -1558,6 +1562,8 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
>      Error *err = NULL;
>      memory_region_init(mr, owner, name, size);
>      mr->ram = true;
> +    mr->ops = &ram_device_mem_ops;
> +    mr->opaque = mr;
>      mr->terminates = true;
>      mr->destructor = memory_region_destructor_ram;
>      mr->align = align;
> @@ -1581,6 +1587,8 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
>      Error *err = NULL;
>      memory_region_init(mr, owner, name, size);
>      mr->ram = true;
> +    mr->ops = &ram_device_mem_ops;
> +    mr->opaque = mr;
>      mr->terminates = true;
>      mr->destructor = memory_region_destructor_ram;
>      mr->ram_block = qemu_ram_alloc_from_fd(size, mr,
> @@ -1603,6 +1611,8 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
>  {
>      memory_region_init(mr, owner, name, size);
>      mr->ram = true;
> +    mr->ops = &ram_device_mem_ops;
> +    mr->opaque = mr;
>      mr->terminates = true;
>      mr->destructor = memory_region_destructor_ram;
>      mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>