1. Patchew
  2. QEMU
  3. View series

[PATCH] memory: Initialize MemoryRegionOps for RAM memory regions

Philippe Mathieu-Daudé posted 1 patch 3 years, 8 months ago
Test docker-quick@centos7 passed
Test docker-mingw@fedora passed
Test checkpatch passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200816182602.352-1-f4bug@amsat.org
There is a newer version of this series
softmmu/memory.c | 10 ++++++++++
1 file changed, 10 insertions(+)
[PATCH] memory: Initialize MemoryRegionOps for RAM memory regions
Posted by Philippe Mathieu-Daudé 3 years, 8 months ago 
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;
-- 
2.21.3
Re: [PATCH] memory: Initialize MemoryRegionOps for RAM memory regions
Posted by Philippe Mathieu-Daudé 3 years, 8 months ago 
+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;
>
Re: [PATCH] memory: Initialize MemoryRegionOps for RAM memory regions
Posted by P J P 3 years, 8 months ago 
+-- 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()

    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
Re: [PATCH] memory: Initialize MemoryRegionOps for RAM memory regions
Posted by Philippe Mathieu-Daudé 3 years, 8 months ago 
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
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.