[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH] physmem: Do not allow unprivileged device map privileged
From: |
Peter Xu |
Subject: |
Re: [RFC PATCH] physmem: Do not allow unprivileged device map privileged memory |
Date: |
Fri, 3 Sep 2021 17:02:29 -0400 |
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