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