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;
+ }
+ return false;
+ } else {
+ /* MEMTXPERM_UNRESTRICTED and MEMTXPERM_UNSPECIFIED cases */
+ return true;
+ }
}
/* Called within RCU critical section. */
--
2.31.1
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,
--
Peter Xu
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?
--
Thanks,
David / dhildenb
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.
© 2016 - 2026 Red Hat, Inc.