[RFC PATCH] physmem: Do not allow unprivileged device map privileged memory

[RFC PATCH] physmem: Do not allow unprivileged device map privileged memory

[Philippe Mathieu-Daudé]

Since commits cc05c43ad94..42874d3a8c6 ("memory: Define API for
MemoryRegionOps to take attrs and return status") the Memory API
returns a zero (MEMTX_OK) response meaning success, anything else
indicating a failure.

In commits c874dc4f5e8..2f7b009c2e7 ("Make address_space_map() take
a MemTxAttrs argument") we updated the AddressSpace and FlatView
APIs but forgot to check the returned value by the FlatView from
the MemoryRegion.

Adjust that now, only returning a non-NULL address if the transaction
succeeded with the requested memory attributes.

Reported-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
RFC because this could become a security issue in a core component,
    however currently all callers pass MEMTXATTRS_UNSPECIFIED.
---
 include/exec/memory.h |  3 ++-
 softmmu/physmem.c     | 16 ++++++++++------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index c3d417d317f..488d2ecdd09 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2706,7 +2706,8 @@ bool address_space_access_valid(AddressSpace *as, hwaddr addr, hwaddr len,
  *
  * May map a subset of the requested range, given by and returned in @plen.
  * May return %NULL and set *@plen to zero(0), if resources needed to perform
- * the mapping are exhausted.
+ * the mapping are exhausted or if the physical memory region is not accessible
+ * for the requested memory attributes.
  * Use only for reads OR writes - not for read-modify-write operations.
  * Use cpu_register_map_client() to know when retrying the map operation is
  * likely to succeed.
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 23e77cb7715..802c339f6d2 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -3188,15 +3188,19 @@ void *address_space_map(AddressSpace *as,
         /* Avoid unbounded allocations */
         l = MIN(l, TARGET_PAGE_SIZE);
         bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, l);
+
+        if (!is_write) {
+            if (flatview_read(fv, addr, attrs, bounce.buffer, l) != MEMTX_OK) {
+                qemu_vfree(bounce.buffer);
+                *plen = 0;
+                return NULL;
+            }
+        }
+
         bounce.addr = addr;
         bounce.len = l;
-
-        memory_region_ref(mr);
         bounce.mr = mr;
-        if (!is_write) {
-            flatview_read(fv, addr, MEMTXATTRS_UNSPECIFIED,
-                               bounce.buffer, l);
-        }
+        memory_region_ref(mr);
 
         *plen = l;
         return bounce.buffer;
-- 
2.31.1

Re: [RFC PATCH] physmem: Do not allow unprivileged device map privileged memory

[Peter Xu]

On Fri, Sep 03, 2021 at 05:38:20PM +0200, Philippe Mathieu-Daudé wrote:
> Since commits cc05c43ad94..42874d3a8c6 ("memory: Define API for
> MemoryRegionOps to take attrs and return status") the Memory API
> returns a zero (MEMTX_OK) response meaning success, anything else
> indicating a failure.
> 
> In commits c874dc4f5e8..2f7b009c2e7 ("Make address_space_map() take
> a MemTxAttrs argument") we updated the AddressSpace and FlatView
> APIs but forgot to check the returned value by the FlatView from
> the MemoryRegion.
> 
> Adjust that now, only returning a non-NULL address if the transaction
> succeeded with the requested memory attributes.
> 
> Reported-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> RFC because this could become a security issue in a core component,
>     however currently all callers pass MEMTXATTRS_UNSPECIFIED.

Could you share more on the implications?

MEMTXATTRS_UNSPECIFIED shouldn't be the only factor to fail flatview_read(), or
is it?  I think the change looks mostly right, but I've no knowledge on the
context of the problems..

> ---
>  include/exec/memory.h |  3 ++-
>  softmmu/physmem.c     | 16 ++++++++++------
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index c3d417d317f..488d2ecdd09 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2706,7 +2706,8 @@ bool address_space_access_valid(AddressSpace *as, hwaddr addr, hwaddr len,
>   *
>   * May map a subset of the requested range, given by and returned in @plen.
>   * May return %NULL and set *@plen to zero(0), if resources needed to perform
> - * the mapping are exhausted.
> + * the mapping are exhausted or if the physical memory region is not accessible
> + * for the requested memory attributes.
>   * Use only for reads OR writes - not for read-modify-write operations.
>   * Use cpu_register_map_client() to know when retrying the map operation is
>   * likely to succeed.

You didn't touch up the comments above address_space_map() in physmem.c, but
instead maybe we can just remove the .c one and only keep the .h one; there's
some duplication and .h should be the most to reference.

> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 23e77cb7715..802c339f6d2 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -3188,15 +3188,19 @@ void *address_space_map(AddressSpace *as,
>          /* Avoid unbounded allocations */
>          l = MIN(l, TARGET_PAGE_SIZE);
>          bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, l);
> +
> +        if (!is_write) {
> +            if (flatview_read(fv, addr, attrs, bounce.buffer, l) != MEMTX_OK) {
> +                qemu_vfree(bounce.buffer);
> +                *plen = 0;

Maybe also:

                   qatomic_mb_set(&bounce.in_use, false);

?

(I'm wondering whether atomic is needed here if we only have one buffer anyway
 and we're with bql; a different matter anyway)

> +                return NULL;
> +            }
> +        }
> +
>          bounce.addr = addr;
>          bounce.len = l;
> -
> -        memory_region_ref(mr);

This line move seems to be unnecessary.

>          bounce.mr = mr;
> -        if (!is_write) {
> -            flatview_read(fv, addr, MEMTXATTRS_UNSPECIFIED,
> -                               bounce.buffer, l);
> -        }
> +        memory_region_ref(mr);
>  
>          *plen = l;
>          return bounce.buffer;
> -- 
> 2.31.1
> 

-- 
Peter Xu

Re: [RFC PATCH] physmem: Do not allow unprivileged device map privileged memory

[Peter Maydell]

On Fri, 3 Sept 2021 at 16:38, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Since commits cc05c43ad94..42874d3a8c6 ("memory: Define API for
> MemoryRegionOps to take attrs and return status") the Memory API
> returns a zero (MEMTX_OK) response meaning success, anything else
> indicating a failure.
>
> In commits c874dc4f5e8..2f7b009c2e7 ("Make address_space_map() take
> a MemTxAttrs argument") we updated the AddressSpace and FlatView
> APIs but forgot to check the returned value by the FlatView from
> the MemoryRegion.
>
> Adjust that now, only returning a non-NULL address if the transaction
> succeeded with the requested memory attributes.
>
> Reported-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> RFC because this could become a security issue in a core component,
>     however currently all callers pass MEMTXATTRS_UNSPECIFIED.

"Wrong set of memattrs" isn't the only reason that a device
could return a failure from the memory transaction. It looks to
me like what this patch is really doing is propagating the
possibly transaction failure up to the callers of address_space_map(),
which seems like the right thing to me.

There are two reasons (now) why address_space_map() could fail:
 (1) QEMU has run out of some internal limited resource
     (ie the bounce buffer is in use elsewhere)
 (2) the emulated guest memory transaction returned a failure;
     this should generally not be fatal, but result in whatever
     the device's reaction to "whoops, DMA failed" is, eg
     setting error registers, stopping processing of DMA, etc

Do we want to make them indistinguishable to callers? It might
be better to have address_space_map() have a way to return the
MemTxResult to callers. (This would bring it into line with other
APIs which both allow passing in MemTxAttrs and getting back a
MemTxResult.) There aren't that many callers of address_space_map()
and dma_memory_map() so it wouldn't be too hard to add an extra
argument or whatever.

Side note: looking at some of the callsites, error handling on
the failure case is not always great. Eg:
 * hw/hyperv/vmbus.c calls dma_memory_unmap() if dma_memory_map()
   fails, which is definitely the wrong thing to do, because it
   will try to unmap NULL
 * hw/misc/aspeed_hace.c just ploughs on using the NULL pointer
   regardless
 * target/ppc/mmu-hash64.c calls hw_error(), ie kills QEMU,
   on failure, which is not strictly wrong but seems a bit harsh.

-- PMM