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