| Message ID | 20210903153820.686913-1-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] physmem: Do not allow unprivileged device map privileged memory | expand |
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 >
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
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;
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(-)