| Message ID | 20210823164157.751807-6-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | physmem: Have flaview API check bus permission from MemTxAttrs argument | expand |
On Mon, Aug 23, 2021 at 06:41:57PM +0200, Philippe Mathieu-Daudé wrote: > @@ -2772,7 +2772,22 @@ static inline bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs, > hwaddr addr, hwaddr len, > MemTxResult *result) > { > - return true; > + if (unlikely(attrs.bus_perm == MEMTXPERM_RAM_DEVICE)) { > + if (memory_region_is_ram(mr) || memory_region_is_ram_device(mr)) { memory_region_is_ram() should be enough ("ram_device" is only set if "ram" is set)? Thanks,
On 23.08.21 18:41, Philippe Mathieu-Daudé wrote: > Check bus permission in flatview_access_allowed() before > running any bus transaction. > > There is not change for the default case (MEMTXPERM_UNSPECIFIED). s/not/no/ > > The MEMTXPERM_UNRESTRICTED case works as an allow list. Devices > using it won't be checked by flatview_access_allowed(). Well, and MEMTXPERM_UNSPECIFIED. Another indication that the split should better be avoided. > > The only deny list equivalent is MEMTXPERM_RAM_DEVICE: devices > using this flag will reject transactions and set the optional > MemTxResult to MEMTX_BUS_ERROR. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > softmmu/physmem.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/softmmu/physmem.c b/softmmu/physmem.c > index 0d31a2f4199..329542dba75 100644 > --- a/softmmu/physmem.c > +++ b/softmmu/physmem.c > @@ -2772,7 +2772,22 @@ static inline bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs, > hwaddr addr, hwaddr len, > MemTxResult *result) > { > - return true; > + if (unlikely(attrs.bus_perm == MEMTXPERM_RAM_DEVICE)) { > + if (memory_region_is_ram(mr) || memory_region_is_ram_device(mr)) { > + return true; > + } I'm a bit confused why it's called MEMTXPERM_RAM_DEVICE, yet we also allow access to !memory_region_is_ram_device(mr). Can we find a more expressive name? Also, I wonder if we'd actually want to have "flags" instead of pure values. Like having #define MEMTXPERM_RAM 1 #define MEMTXPERM_RAM_DEVICE 2 and map them cleanly to the two similar, but different types of memory backends. > + qemu_log_mask(LOG_GUEST_ERROR, > + "Invalid access to non-RAM device at " > + "addr 0x%" HWADDR_PRIX ", size %" HWADDR_PRIu ", " > + "region '%s'\n", addr, len, memory_region_name(mr)); > + if (result) { > + *result |= MEMTX_BUS_ERROR; > + } > + return false; > + } else { > + /* MEMTXPERM_UNRESTRICTED and MEMTXPERM_UNSPECIFIED cases */ > + return true; > + } > } > > /* Called within RCU critical section. */ > Do we have any target user examples / prototypes?
On Mon, Aug 23, 2021 at 06:41:57PM +0200, Philippe Mathieu-Daudé wrote: > Check bus permission in flatview_access_allowed() before > running any bus transaction. > > There is not change for the default case (MEMTXPERM_UNSPECIFIED). > > The MEMTXPERM_UNRESTRICTED case works as an allow list. Devices > using it won't be checked by flatview_access_allowed(). > > The only deny list equivalent is MEMTXPERM_RAM_DEVICE: devices > using this flag will reject transactions and set the optional > MemTxResult to MEMTX_BUS_ERROR. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > softmmu/physmem.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/softmmu/physmem.c b/softmmu/physmem.c > index 0d31a2f4199..329542dba75 100644 > --- a/softmmu/physmem.c > +++ b/softmmu/physmem.c > @@ -2772,7 +2772,22 @@ static inline bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs, > hwaddr addr, hwaddr len, > MemTxResult *result) > { > - return true; > + if (unlikely(attrs.bus_perm == MEMTXPERM_RAM_DEVICE)) { > + if (memory_region_is_ram(mr) || memory_region_is_ram_device(mr)) { > + return true; > + } > + qemu_log_mask(LOG_GUEST_ERROR, > + "Invalid access to non-RAM device at " > + "addr 0x%" HWADDR_PRIX ", size %" HWADDR_PRIu ", " > + "region '%s'\n", addr, len, memory_region_name(mr)); > + if (result) { > + *result |= MEMTX_BUS_ERROR; Why bitwise OR? > + } > + return false; > + } else { > + /* MEMTXPERM_UNRESTRICTED and MEMTXPERM_UNSPECIFIED cases */ > + return true; > + } > } > > /* Called within RCU critical section. */ > -- > 2.31.1 >
On 8/24/21 3:15 PM, Stefan Hajnoczi wrote: > On Mon, Aug 23, 2021 at 06:41:57PM +0200, Philippe Mathieu-Daudé wrote: >> Check bus permission in flatview_access_allowed() before >> running any bus transaction. >> >> There is not change for the default case (MEMTXPERM_UNSPECIFIED). >> >> The MEMTXPERM_UNRESTRICTED case works as an allow list. Devices >> using it won't be checked by flatview_access_allowed(). >> >> The only deny list equivalent is MEMTXPERM_RAM_DEVICE: devices >> using this flag will reject transactions and set the optional >> MemTxResult to MEMTX_BUS_ERROR. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> softmmu/physmem.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/softmmu/physmem.c b/softmmu/physmem.c >> index 0d31a2f4199..329542dba75 100644 >> --- a/softmmu/physmem.c >> +++ b/softmmu/physmem.c >> @@ -2772,7 +2772,22 @@ static inline bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs, >> hwaddr addr, hwaddr len, >> MemTxResult *result) >> { >> - return true; >> + if (unlikely(attrs.bus_perm == MEMTXPERM_RAM_DEVICE)) { >> + if (memory_region_is_ram(mr) || memory_region_is_ram_device(mr)) { >> + return true; >> + } >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "Invalid access to non-RAM device at " >> + "addr 0x%" HWADDR_PRIX ", size %" HWADDR_PRIu ", " >> + "region '%s'\n", addr, len, memory_region_name(mr)); >> + if (result) { >> + *result |= MEMTX_BUS_ERROR; > > Why bitwise OR? MemTxResult is not an enum but used as a bitfield. See access_with_adjusted_size(): MemTxResult r = MEMTX_OK; ... if (memory_region_big_endian(mr)) { for (i = 0; i < size; i += access_size) { r |= access_fn(mr, addr + i, value, access_size, (size - access_size - i) * 8, access_mask, attrs); } } else { for (i = 0; i < size; i += access_size) { r |= access_fn(mr, addr + i, value, access_size, i * 8, access_mask, attrs); } } return r; } and flatview_read_continue() / flatview_write_continue(): for (;;) { if (!memory_access_is_direct(mr, true)) { release_lock |= prepare_mmio_access(mr); l = memory_access_size(mr, l, addr1); val = ldn_he_p(buf, l); result |= memory_region_dispatch_write(mr, addr1, val, size_memop(l), attrs); ... return result; }
On Tue, 24 Aug 2021 at 14:50, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 8/24/21 3:15 PM, Stefan Hajnoczi wrote: > > On Mon, Aug 23, 2021 at 06:41:57PM +0200, Philippe Mathieu-Daudé wrote: > >> Check bus permission in flatview_access_allowed() before > >> running any bus transaction. > >> > >> There is not change for the default case (MEMTXPERM_UNSPECIFIED). > >> > >> The MEMTXPERM_UNRESTRICTED case works as an allow list. Devices > >> using it won't be checked by flatview_access_allowed(). > >> > >> The only deny list equivalent is MEMTXPERM_RAM_DEVICE: devices > >> using this flag will reject transactions and set the optional > >> MemTxResult to MEMTX_BUS_ERROR. > >> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> --- > >> softmmu/physmem.c | 17 ++++++++++++++++- > >> 1 file changed, 16 insertions(+), 1 deletion(-) > >> > >> diff --git a/softmmu/physmem.c b/softmmu/physmem.c > >> index 0d31a2f4199..329542dba75 100644 > >> --- a/softmmu/physmem.c > >> +++ b/softmmu/physmem.c > >> @@ -2772,7 +2772,22 @@ static inline bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs, > >> hwaddr addr, hwaddr len, > >> MemTxResult *result) > >> { > >> - return true; > >> + if (unlikely(attrs.bus_perm == MEMTXPERM_RAM_DEVICE)) { > >> + if (memory_region_is_ram(mr) || memory_region_is_ram_device(mr)) { > >> + return true; > >> + } > >> + qemu_log_mask(LOG_GUEST_ERROR, > >> + "Invalid access to non-RAM device at " > >> + "addr 0x%" HWADDR_PRIX ", size %" HWADDR_PRIu ", " > >> + "region '%s'\n", addr, len, memory_region_name(mr)); > >> + if (result) { > >> + *result |= MEMTX_BUS_ERROR; > > > > Why bitwise OR? > > MemTxResult is not an enum but used as a bitfield. > > See access_with_adjusted_size(): > > MemTxResult r = MEMTX_OK; > ... > if (memory_region_big_endian(mr)) { > for (i = 0; i < size; i += access_size) { > r |= access_fn(mr, addr + i, value, access_size, > (size - access_size - i) * 8, > access_mask, attrs); > } > } else { > for (i = 0; i < size; i += access_size) { > r |= access_fn(mr, addr + i, value, access_size, i * 8, > access_mask, attrs); > } > } > return r; > } > > and flatview_read_continue() / flatview_write_continue(): > > for (;;) { > if (!memory_access_is_direct(mr, true)) { > release_lock |= prepare_mmio_access(mr); > l = memory_access_size(mr, l, addr1); > val = ldn_he_p(buf, l); > result |= memory_region_dispatch_write(mr, addr1, val, > size_memop(l), > attrs); > ... > return result; > } In these two examples we OR together the MemTxResults because we are looping over multiple accesses and combining all the results together; we want to return a "not OK" result if any of the individual results failed. Is that the case for flatview_access_allowed() ? -- PMM
On 8/24/21 16:21, Peter Maydell wrote: > On Tue, 24 Aug 2021 at 14:50, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> On 8/24/21 3:15 PM, Stefan Hajnoczi wrote: >>> On Mon, Aug 23, 2021 at 06:41:57PM +0200, Philippe Mathieu-Daudé wrote: >>>> Check bus permission in flatview_access_allowed() before >>>> running any bus transaction. >>>> >>>> There is not change for the default case (MEMTXPERM_UNSPECIFIED). >>>> >>>> The MEMTXPERM_UNRESTRICTED case works as an allow list. Devices >>>> using it won't be checked by flatview_access_allowed(). >>>> >>>> The only deny list equivalent is MEMTXPERM_RAM_DEVICE: devices >>>> using this flag will reject transactions and set the optional >>>> MemTxResult to MEMTX_BUS_ERROR. >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> --- >>>> softmmu/physmem.c | 17 ++++++++++++++++- >>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c >>>> index 0d31a2f4199..329542dba75 100644 >>>> --- a/softmmu/physmem.c >>>> +++ b/softmmu/physmem.c >>>> @@ -2772,7 +2772,22 @@ static inline bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs, >>>> hwaddr addr, hwaddr len, >>>> MemTxResult *result) >>>> { >>>> - return true; >>>> + if (unlikely(attrs.bus_perm == MEMTXPERM_RAM_DEVICE)) { >>>> + if (memory_region_is_ram(mr) || memory_region_is_ram_device(mr)) { >>>> + return true; >>>> + } >>>> + qemu_log_mask(LOG_GUEST_ERROR, >>>> + "Invalid access to non-RAM device at " >>>> + "addr 0x%" HWADDR_PRIX ", size %" HWADDR_PRIu ", " >>>> + "region '%s'\n", addr, len, memory_region_name(mr)); >>>> + if (result) { >>>> + *result |= MEMTX_BUS_ERROR; >>> >>> Why bitwise OR? >> >> MemTxResult is not an enum but used as a bitfield. >> >> See access_with_adjusted_size(): >> >> MemTxResult r = MEMTX_OK; >> ... >> if (memory_region_big_endian(mr)) { >> for (i = 0; i < size; i += access_size) { >> r |= access_fn(mr, addr + i, value, access_size, >> (size - access_size - i) * 8, >> access_mask, attrs); >> } >> } else { >> for (i = 0; i < size; i += access_size) { >> r |= access_fn(mr, addr + i, value, access_size, i * 8, >> access_mask, attrs); >> } >> } >> return r; >> } >> >> and flatview_read_continue() / flatview_write_continue(): >> >> for (;;) { >> if (!memory_access_is_direct(mr, true)) { >> release_lock |= prepare_mmio_access(mr); >> l = memory_access_size(mr, l, addr1); >> val = ldn_he_p(buf, l); >> result |= memory_region_dispatch_write(mr, addr1, val, >> size_memop(l), >> attrs); >> ... >> return result; >> } > > In these two examples we OR together the MemTxResults because > we are looping over multiple accesses and combining all the > results together; we want to return a "not OK" result if any > of the individual results failed. Is that the case for > flatview_access_allowed() ? You are right, this is not the case here, so we can simplify as Stefan suggested. Thanks for clarifying the examples.
diff --git a/softmmu/physmem.c b/softmmu/physmem.c index 0d31a2f4199..329542dba75 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -2772,7 +2772,22 @@ static inline bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs, hwaddr addr, hwaddr len, MemTxResult *result) { - return true; + if (unlikely(attrs.bus_perm == MEMTXPERM_RAM_DEVICE)) { + if (memory_region_is_ram(mr) || memory_region_is_ram_device(mr)) { + return true; + } + qemu_log_mask(LOG_GUEST_ERROR, + "Invalid access to non-RAM device at " + "addr 0x%" HWADDR_PRIX ", size %" HWADDR_PRIu ", " + "region '%s'\n", addr, len, memory_region_name(mr)); + if (result) { + *result |= MEMTX_BUS_ERROR; + } + return false; + } else { + /* MEMTXPERM_UNRESTRICTED and MEMTXPERM_UNSPECIFIED cases */ + return true; + } } /* Called within RCU critical section. */
Check bus permission in flatview_access_allowed() before running any bus transaction. There is not change for the default case (MEMTXPERM_UNSPECIFIED). The MEMTXPERM_UNRESTRICTED case works as an allow list. Devices using it won't be checked by flatview_access_allowed(). The only deny list equivalent is MEMTXPERM_RAM_DEVICE: devices using this flag will reject transactions and set the optional MemTxResult to MEMTX_BUS_ERROR. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- softmmu/physmem.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)