[RFC PATCH v2 0/5] physmem: Have flaview API check bus permission from MemTxAttrs argument

[RFC PATCH v2 0/5] physmem: Have flaview API check bus permission from MemTxAttrs argument

[Philippe Mathieu-Daudé]

This series aim to kill a recent class of bug, the infamous
"DMA reentrancy" issues found by Alexander while fuzzing.

Introduce the 'bus_perm' field in MemTxAttrs, defining 3 bits:

- MEMTXPERM_UNSPECIFIED (current default, unchanged behavior)
- MEMTXPERM_UNRESTRICTED (allow list approach)
- MEMTXPERM_RAM_DEVICE (example of deny list approach)

If a transaction permission is not allowed (for example access
to non-RAM device), we return the specific MEMTX_BUS_ERROR.

Permissions are checked in after the flatview is resolved, and
before the access is done, in a new function: flatview_access_allowed().

I'll post another series on top as example, fixing the SD card
bugs.

Since v1 ("hw: Forbid DMA write accesses to MMIO regions") [1]:
- rewrite based on Peter / Stefan feedbacks

Based on "hw: Let the DMA API take a MemTxAttrs argument" [2].

Supersedes: <20200903110831.353476-1-philmd@redhat.com>
Based-on: <20210702092439.989969-1-philmd@redhat.com>

[1] https://www.mail-archive.com/qemu-block@nongnu.org/msg72924.html
[2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg820359.html

Philippe Mathieu-Daudé (5):
  softmmu/physmem: Simplify flatview_write and
    address_space_access_valid
  hw/intc/arm_gicv3: Check for !MEMTX_OK instead of MEMTX_ERROR
  exec/memattrs: Introduce MemTxAttrs::bus_perm field
  softmmu/physmem: Introduce flatview_access_allowed() to check bus
    perms
  softmmu/physmem: Have flaview API check MemTxAttrs::bus_perm field

 include/exec/memattrs.h    | 21 +++++++++++++
 hw/intc/arm_gicv3_dist.c   |  4 +--
 hw/intc/arm_gicv3_redist.c |  4 +--
 softmmu/physmem.c          | 61 ++++++++++++++++++++++++++++++++------
 4 files changed, 77 insertions(+), 13 deletions(-)

-- 
2.31.1

[RFC PATCH v2 1/5] softmmu/physmem: Simplify flatview_write and address_space_access_valid

[Philippe Mathieu-Daudé]

Remove unuseful local 'result' variables.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 softmmu/physmem.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 2e18947598e..e534dc69918 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2810,14 +2810,11 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
     hwaddr l;
     hwaddr addr1;
     MemoryRegion *mr;
-    MemTxResult result = MEMTX_OK;
 
     l = len;
     mr = flatview_translate(fv, addr, &addr1, &l, true, attrs);
-    result = flatview_write_continue(fv, addr, attrs, buf, len,
-                                     addr1, l, mr);
-
-    return result;
+    return flatview_write_continue(fv, addr, attrs, buf, len,
+                                   addr1, l, mr);
 }
 
 /* Called within RCU critical section.  */
@@ -3114,12 +3111,10 @@ bool address_space_access_valid(AddressSpace *as, hwaddr addr,
                                 MemTxAttrs attrs)
 {
     FlatView *fv;
-    bool result;
 
     RCU_READ_LOCK_GUARD();
     fv = address_space_to_flatview(as);
-    result = flatview_access_valid(fv, addr, len, is_write, attrs);
-    return result;
+    return flatview_access_valid(fv, addr, len, is_write, attrs);
 }
 
 static hwaddr
-- 
2.31.1

Re: [RFC PATCH v2 1/5] softmmu/physmem: Simplify flatview_write and address_space_access_valid

[Peter Xu]

On Mon, Aug 23, 2021 at 06:41:53PM +0200, Philippe Mathieu-Daudé wrote:
> Remove unuseful local 'result' variables.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [RFC PATCH v2 1/5] softmmu/physmem: Simplify flatview_write and address_space_access_valid

[David Hildenbrand]

On 23.08.21 18:41, Philippe Mathieu-Daudé wrote:
> Remove unuseful local 'result' variables.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [RFC PATCH v2 1/5] softmmu/physmem: Simplify flatview_write and address_space_access_valid

[Alexander Bulekov]

On 210823 1841, Philippe Mathieu-Daudé wrote:
> Remove unuseful local 'result' variables.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  softmmu/physmem.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 

Reviewed-by: Alexander Bulekov <alxndr@bu.edu>

Re: [RFC PATCH v2 1/5] softmmu/physmem: Simplify flatview_write and address_space_access_valid

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]

On Mon, Aug 23, 2021 at 06:41:53PM +0200, Philippe Mathieu-Daudé wrote:
> Remove unuseful local 'result' variables.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  softmmu/physmem.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH v2 2/5] hw/intc/arm_gicv3: Check for !MEMTX_OK instead of MEMTX_ERROR

[Philippe Mathieu-Daudé]

We are going to introduce more MemTxResult bits, so it is
safer to check for !MEMTX_OK rather than MEMTX_ERROR.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/intc/arm_gicv3_dist.c   | 4 ++--
 hw/intc/arm_gicv3_redist.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c
index b65f56f9035..e62a9cdfa8d 100644
--- a/hw/intc/arm_gicv3_dist.c
+++ b/hw/intc/arm_gicv3_dist.c
@@ -819,7 +819,7 @@ MemTxResult gicv3_dist_read(void *opaque, hwaddr offset, uint64_t *data,
         break;
     }
 
-    if (r == MEMTX_ERROR) {
+    if (r != MEMTX_OK) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid guest read at offset " TARGET_FMT_plx
                       "size %u\n", __func__, offset, size);
@@ -861,7 +861,7 @@ MemTxResult gicv3_dist_write(void *opaque, hwaddr offset, uint64_t data,
         break;
     }
 
-    if (r == MEMTX_ERROR) {
+    if (r != MEMTX_OK) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid guest write at offset " TARGET_FMT_plx
                       "size %u\n", __func__, offset, size);
diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
index 53da703ed84..547281a8961 100644
--- a/hw/intc/arm_gicv3_redist.c
+++ b/hw/intc/arm_gicv3_redist.c
@@ -450,7 +450,7 @@ MemTxResult gicv3_redist_read(void *opaque, hwaddr offset, uint64_t *data,
         break;
     }
 
-    if (r == MEMTX_ERROR) {
+    if (r != MEMTX_OK) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid guest read at offset " TARGET_FMT_plx
                       " size %u\n", __func__, offset, size);
@@ -507,7 +507,7 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data,
         break;
     }
 
-    if (r == MEMTX_ERROR) {
+    if (r != MEMTX_OK) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid guest write at offset " TARGET_FMT_plx
                       " size %u\n", __func__, offset, size);
-- 
2.31.1

Re: [RFC PATCH v2 2/5] hw/intc/arm_gicv3: Check for !MEMTX_OK instead of MEMTX_ERROR

[Peter Xu]

On Mon, Aug 23, 2021 at 06:41:54PM +0200, Philippe Mathieu-Daudé wrote:
> We are going to introduce more MemTxResult bits, so it is
> safer to check for !MEMTX_OK rather than MEMTX_ERROR.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [RFC PATCH v2 2/5] hw/intc/arm_gicv3: Check for !MEMTX_OK instead of MEMTX_ERROR

[David Hildenbrand]

On 23.08.21 18:41, Philippe Mathieu-Daudé wrote:
> We are going to introduce more MemTxResult bits, so it is
> safer to check for !MEMTX_OK rather than MEMTX_ERROR.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [RFC PATCH v2 2/5] hw/intc/arm_gicv3: Check for !MEMTX_OK instead of MEMTX_ERROR

[Peter Maydell]

On Mon, 23 Aug 2021 at 17:42, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> We are going to introduce more MemTxResult bits, so it is
> safer to check for !MEMTX_OK rather than MEMTX_ERROR.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

but note that these MEMTX_* aren't from the memory transaction
API functions; they're just being used by gicd_readl() and
friends as a way to indicate a success/failure so that the
actual MemoryRegionOps read/write fns like gicv3_dist_read()
can log a guest error. Arguably this is a bit of a misuse of
the MEMTX_* constants and perhaps we should have gicd_readl etc
return a bool instead.

thanks
-- PMM

Re: [RFC PATCH v2 2/5] hw/intc/arm_gicv3: Check for !MEMTX_OK instead of MEMTX_ERROR

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 450 bytes --]

On Mon, Aug 23, 2021 at 06:41:54PM +0200, Philippe Mathieu-Daudé wrote:
> We are going to introduce more MemTxResult bits, so it is
> safer to check for !MEMTX_OK rather than MEMTX_ERROR.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/intc/arm_gicv3_dist.c   | 4 ++--
>  hw/intc/arm_gicv3_redist.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH v2 3/5] exec/memattrs: Introduce MemTxAttrs::bus_perm field

[Philippe Mathieu-Daudé]

Add the 'direct_access' bit to the memory attributes to restrict
bus master access to ROM/RAM.
Have read/write accessors return MEMTX_BUS_ERROR if an access is
restricted and the region is not ROM/RAM ('direct').
Add corresponding trace events.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memattrs.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 95f2d20d55b..7a94ee75a88 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -14,6 +14,13 @@
 #ifndef MEMATTRS_H
 #define MEMATTRS_H
 
+/* Permission to restrict bus memory accesses. See MemTxAttrs::bus_perm */
+enum {
+    MEMTXPERM_UNSPECIFIED   = 0,
+    MEMTXPERM_UNRESTRICTED  = 1,
+    MEMTXPERM_RAM_DEVICE    = 2,
+};
+
 /* Every memory transaction has associated with it a set of
  * attributes. Some of these are generic (such as the ID of
  * the bus master); some are specific to a particular kind of
@@ -35,6 +42,19 @@ typedef struct MemTxAttrs {
     unsigned int secure:1;
     /* Memory access is usermode (unprivileged) */
     unsigned int user:1;
+    /*
+     * Bus memory access permission.
+     *
+     * Some devices (such DMA) might be restricted to only access
+     * some type of device, such RAM devices. By default memory
+     * accesses are unspecified (MEMTXPERM_UNSPECIFIED), but could be
+     * unrestricted (MEMTXPERM_UNRESTRICTED, similar to an allow list)
+     * or restricted to a type of devices (similar to a deny list).
+     * Currently only RAM devices can be restricted (MEMTXPERM_RAM_DEVICE).
+     *
+     * Memory accesses to restricted addresses return MEMTX_BUS_ERROR.
+     */
+    unsigned int bus_perm:2;
     /* Requester ID (for MSI for example) */
     unsigned int requester_id:16;
     /* Invert endianness for this page */
@@ -66,6 +86,7 @@ typedef struct MemTxAttrs {
 #define MEMTX_OK 0
 #define MEMTX_ERROR             (1U << 0) /* device returned an error */
 #define MEMTX_DECODE_ERROR      (1U << 1) /* nothing at that address */
+#define MEMTX_BUS_ERROR         (1U << 2) /* bus returned an error */
 typedef uint32_t MemTxResult;
 
 #endif
-- 
2.31.1

Re: [RFC PATCH v2 3/5] exec/memattrs: Introduce MemTxAttrs::bus_perm field

[Peter Xu]

On Mon, Aug 23, 2021 at 06:41:55PM +0200, Philippe Mathieu-Daudé wrote:
> +/* Permission to restrict bus memory accesses. See MemTxAttrs::bus_perm */
> +enum {
> +    MEMTXPERM_UNSPECIFIED   = 0,
> +    MEMTXPERM_UNRESTRICTED  = 1,
> +    MEMTXPERM_RAM_DEVICE    = 2,
> +};

Is there a difference between UNSPECIFIED and UNRESTRICTED?

If no, should we merge them?

-- 
Peter Xu

Re: [RFC PATCH v2 3/5] exec/memattrs: Introduce MemTxAttrs::bus_perm field

[David Hildenbrand]

On 23.08.21 20:41, Peter Xu wrote:
> On Mon, Aug 23, 2021 at 06:41:55PM +0200, Philippe Mathieu-Daudé wrote:
>> +/* Permission to restrict bus memory accesses. See MemTxAttrs::bus_perm */
>> +enum {
>> +    MEMTXPERM_UNSPECIFIED   = 0,
>> +    MEMTXPERM_UNRESTRICTED  = 1,
>> +    MEMTXPERM_RAM_DEVICE    = 2,
>> +};
> 
> Is there a difference between UNSPECIFIED and UNRESTRICTED?
> 
> If no, should we merge them?
> 

I'd assume MEMTXPERM_UNSPECIFIED has to be treated like 
MEMTXPERM_UNRESTRICTED, so I'd also think we should just squash them.

-- 
Thanks,

David / dhildenb

Re: [RFC PATCH v2 3/5] exec/memattrs: Introduce MemTxAttrs::bus_perm field

[Philippe Mathieu-Daudé]

On 8/23/21 21:04, David Hildenbrand wrote:
> On 23.08.21 20:41, Peter Xu wrote:
>> On Mon, Aug 23, 2021 at 06:41:55PM +0200, Philippe Mathieu-Daudé wrote:
>>> +/* Permission to restrict bus memory accesses. See
>>> MemTxAttrs::bus_perm */
>>> +enum {
>>> +    MEMTXPERM_UNSPECIFIED   = 0,
>>> +    MEMTXPERM_UNRESTRICTED  = 1,
>>> +    MEMTXPERM_RAM_DEVICE    = 2,
>>> +};
>>
>> Is there a difference between UNSPECIFIED and UNRESTRICTED?
>>
>> If no, should we merge them?
>>
> 
> I'd assume MEMTXPERM_UNSPECIFIED has to be treated like
> MEMTXPERM_UNRESTRICTED, so I'd also think we should just squash them.

For now they are treated the same way, but ideally we should
explicitly classify bus accesses and remove the MEMTXPERM_UNSPECIFIED.

While we can use the same definition with comments, I think having
different definitions ease maintainance (thinking of git-grep), but
if we know we will never classify/convert the devices, then indeed
having MEMTXPERM_UNSPECIFIED is pointless and confusing.

Re: [RFC PATCH v2 3/5] exec/memattrs: Introduce MemTxAttrs::bus_perm field

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2153 bytes --]

On Mon, Aug 23, 2021 at 06:41:55PM +0200, Philippe Mathieu-Daudé wrote:
> Add the 'direct_access' bit to the memory attributes to restrict
> bus master access to ROM/RAM.
> Have read/write accessors return MEMTX_BUS_ERROR if an access is
> restricted and the region is not ROM/RAM ('direct').
> Add corresponding trace events.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/exec/memattrs.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index 95f2d20d55b..7a94ee75a88 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -14,6 +14,13 @@
>  #ifndef MEMATTRS_H
>  #define MEMATTRS_H
>  
> +/* Permission to restrict bus memory accesses. See MemTxAttrs::bus_perm */
> +enum {
> +    MEMTXPERM_UNSPECIFIED   = 0,
> +    MEMTXPERM_UNRESTRICTED  = 1,
> +    MEMTXPERM_RAM_DEVICE    = 2,
> +};
> +
>  /* Every memory transaction has associated with it a set of
>   * attributes. Some of these are generic (such as the ID of
>   * the bus master); some are specific to a particular kind of
> @@ -35,6 +42,19 @@ typedef struct MemTxAttrs {
>      unsigned int secure:1;
>      /* Memory access is usermode (unprivileged) */
>      unsigned int user:1;
> +    /*
> +     * Bus memory access permission.
> +     *
> +     * Some devices (such DMA) might be restricted to only access
> +     * some type of device, such RAM devices. By default memory
> +     * accesses are unspecified (MEMTXPERM_UNSPECIFIED), but could be
> +     * unrestricted (MEMTXPERM_UNRESTRICTED, similar to an allow list)
> +     * or restricted to a type of devices (similar to a deny list).
> +     * Currently only RAM devices can be restricted (MEMTXPERM_RAM_DEVICE).

I don't understand these 3 categories.

MEMTXPERM_UNSPECIFIED means any MemoryRegion can be accessed?

What does MEMTXPERM_UNRESTRICTED mean? How does this differ from
MEMTXPERM_UNSPECIFIED?

What exactly does MEMTXPERM_RAM_DEVICE mean? Maybe that only
MemoryRegions where memory_region_is_ram() is true can be accessed?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH v2 3/5] exec/memattrs: Introduce MemTxAttrs::bus_perm field

[Philippe Mathieu-Daudé]

On 8/24/21 15:08, Stefan Hajnoczi wrote:
> On Mon, Aug 23, 2021 at 06:41:55PM +0200, Philippe Mathieu-Daudé wrote:
>> Add the 'direct_access' bit to the memory attributes to restrict
>> bus master access to ROM/RAM.
>> Have read/write accessors return MEMTX_BUS_ERROR if an access is
>> restricted and the region is not ROM/RAM ('direct').
>> Add corresponding trace events.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/exec/memattrs.h | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
>> index 95f2d20d55b..7a94ee75a88 100644
>> --- a/include/exec/memattrs.h
>> +++ b/include/exec/memattrs.h
>> @@ -14,6 +14,13 @@
>>  #ifndef MEMATTRS_H
>>  #define MEMATTRS_H
>>  
>> +/* Permission to restrict bus memory accesses. See MemTxAttrs::bus_perm */
>> +enum {
>> +    MEMTXPERM_UNSPECIFIED   = 0,
>> +    MEMTXPERM_UNRESTRICTED  = 1,
>> +    MEMTXPERM_RAM_DEVICE    = 2,
>> +};
>> +
>>  /* Every memory transaction has associated with it a set of
>>   * attributes. Some of these are generic (such as the ID of
>>   * the bus master); some are specific to a particular kind of
>> @@ -35,6 +42,19 @@ typedef struct MemTxAttrs {
>>      unsigned int secure:1;
>>      /* Memory access is usermode (unprivileged) */
>>      unsigned int user:1;
>> +    /*
>> +     * Bus memory access permission.
>> +     *
>> +     * Some devices (such DMA) might be restricted to only access
>> +     * some type of device, such RAM devices. By default memory
>> +     * accesses are unspecified (MEMTXPERM_UNSPECIFIED), but could be
>> +     * unrestricted (MEMTXPERM_UNRESTRICTED, similar to an allow list)
>> +     * or restricted to a type of devices (similar to a deny list).
>> +     * Currently only RAM devices can be restricted (MEMTXPERM_RAM_DEVICE).
> 
> I don't understand these 3 categories.
> 
> MEMTXPERM_UNSPECIFIED means any MemoryRegion can be accessed?

MEMTXPERM_UNSPECIFIED means no change in the current behavior.
IOW we haven't reviewed the device, and don't know whether it has
to be restricted or not.

> What does MEMTXPERM_UNRESTRICTED mean? How does this differ from
> MEMTXPERM_UNSPECIFIED?

We set MEMTXPERM_UNRESTRICTED when we reviewed a device and are sure
it can be accessed any region (even being re-entrant on itself).
I understand it like connected via a dual-port on the bus, and
allowing read *and* write accesses at the same time... So the device
is allowed to access itself, i.e. it can re-program itself in loop
and so on. IIUC this is a requested feature for this API.

> What exactly does MEMTXPERM_RAM_DEVICE mean? Maybe that only
> MemoryRegions where memory_region_is_ram() is true can be accessed?

No, it means, while we don't know which bus owner will access the
device, the device itself can only access RAMd regions on the bus.

I added MEMTXPERM_RAM_DEVICE as the first example, thinking it is
the more generic use case. But maybe it is too generic and unuseful,
and we need a real bus permission matrix?

[RFC PATCH v2 4/5] softmmu/physmem: Introduce flatview_access_allowed() to check bus perms

[Philippe Mathieu-Daudé]

Introduce flatview_access_allowed() to check bus permission
before running any bus transaction. For now this is a simple
stub.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 softmmu/physmem.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index e534dc69918..0d31a2f4199 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2754,6 +2754,27 @@ static bool prepare_mmio_access(MemoryRegion *mr)
     return release_lock;
 }
 
+/**
+ * flatview_access_allowed
+ * @mr: #MemoryRegion to be accessed
+ * @attrs: memory transaction attributes
+ * @addr: address within that memory region
+ * @len: the number of bytes to access
+ * @result: optional pointer to a MemTxResult or NULL
+ *
+ * Check if a memory transaction is allowed. If an error occurs this
+ * method will return false to indicate denial, as well as setting
+ * @result to contain corresponding #MemTxResult.
+ *
+ * Returns: true if transaction is allowed, false if denied.
+ */
+static inline bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs,
+                                           hwaddr addr, hwaddr len,
+                                           MemTxResult *result)
+{
+    return true;
+}
+
 /* Called within RCU critical section.  */
 static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
                                            MemTxAttrs attrs,
@@ -2768,7 +2789,9 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
     const uint8_t *buf = ptr;
 
     for (;;) {
-        if (!memory_access_is_direct(mr, true)) {
+        if (!flatview_access_allowed(mr, attrs, addr1, l, &result)) {
+            /* 'result' contains the error, keep going. */
+        } else if (!memory_access_is_direct(mr, true)) {
             release_lock |= prepare_mmio_access(mr);
             l = memory_access_size(mr, l, addr1);
             /* XXX: could force current_cpu to NULL to avoid
@@ -2810,9 +2833,13 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
     hwaddr l;
     hwaddr addr1;
     MemoryRegion *mr;
+    MemTxResult result = MEMTX_OK;
 
     l = len;
     mr = flatview_translate(fv, addr, &addr1, &l, true, attrs);
+    if (!flatview_access_allowed(mr, attrs, addr, len, &result)) {
+        return result;
+    }
     return flatview_write_continue(fv, addr, attrs, buf, len,
                                    addr1, l, mr);
 }
@@ -2831,7 +2858,9 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
 
     fuzz_dma_read_cb(addr, len, mr);
     for (;;) {
-        if (!memory_access_is_direct(mr, false)) {
+        if (!flatview_access_allowed(mr, attrs, addr1, l, &result)) {
+            /* 'result' contains the error, keep going. */
+        } else if (!memory_access_is_direct(mr, false)) {
             /* I/O case */
             release_lock |= prepare_mmio_access(mr);
             l = memory_access_size(mr, l, addr1);
@@ -2871,9 +2900,13 @@ static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
     hwaddr l;
     hwaddr addr1;
     MemoryRegion *mr;
+    MemTxResult result = MEMTX_OK;
 
     l = len;
     mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
+    if (!flatview_access_allowed(mr, attrs, addr, len, &result)) {
+        return result;
+    }
     return flatview_read_continue(fv, addr, attrs, buf, len,
                                   addr1, l, mr);
 }
-- 
2.31.1

Re: [RFC PATCH v2 4/5] softmmu/physmem: Introduce flatview_access_allowed() to check bus perms

[Peter Xu]

On Mon, Aug 23, 2021 at 06:41:56PM +0200, Philippe Mathieu-Daudé wrote:
> Introduce flatview_access_allowed() to check bus permission
> before running any bus transaction. For now this is a simple
> stub.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Shall we squash this patch into the next one?  It helps explain better on why
it's needed.  Thanks,

-- 
Peter Xu

Re: [RFC PATCH v2 4/5] softmmu/physmem: Introduce flatview_access_allowed() to check bus perms

[David Hildenbrand]

On 23.08.21 20:43, Peter Xu wrote:
> On Mon, Aug 23, 2021 at 06:41:56PM +0200, Philippe Mathieu-Daudé wrote:
>> Introduce flatview_access_allowed() to check bus permission
>> before running any bus transaction. For now this is a simple
>> stub.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Shall we squash this patch into the next one?  It helps explain better on why
> it's needed.  Thanks,
> 

I'd even go one step further and squash 3-5 into a single one.

-- 
Thanks,

David / dhildenb

Re: [RFC PATCH v2 4/5] softmmu/physmem: Introduce flatview_access_allowed() to check bus perms

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4061 bytes --]

On Mon, Aug 23, 2021 at 06:41:56PM +0200, Philippe Mathieu-Daudé wrote:
> Introduce flatview_access_allowed() to check bus permission
> before running any bus transaction. For now this is a simple
> stub.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  softmmu/physmem.c | 37 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index e534dc69918..0d31a2f4199 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -2754,6 +2754,27 @@ static bool prepare_mmio_access(MemoryRegion *mr)
>      return release_lock;
>  }
>  
> +/**
> + * flatview_access_allowed
> + * @mr: #MemoryRegion to be accessed
> + * @attrs: memory transaction attributes
> + * @addr: address within that memory region
> + * @len: the number of bytes to access
> + * @result: optional pointer to a MemTxResult or NULL
> + *
> + * Check if a memory transaction is allowed. If an error occurs this
> + * method will return false to indicate denial, as well as setting
> + * @result to contain corresponding #MemTxResult.
> + *
> + * Returns: true if transaction is allowed, false if denied.
> + */
> +static inline bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs,
> +                                           hwaddr addr, hwaddr len,
> +                                           MemTxResult *result)
> +{
> +    return true;
> +}

The callers below don't benefit from the result pointer argument. The
code would be clearer if they did:

  if (!flatview_access_allowed(...)) {
      return MEMTX_BUS_ERROR;
  }

> +
>  /* Called within RCU critical section.  */
>  static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
>                                             MemTxAttrs attrs,
> @@ -2768,7 +2789,9 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
>      const uint8_t *buf = ptr;
>  
>      for (;;) {
> -        if (!memory_access_is_direct(mr, true)) {
> +        if (!flatview_access_allowed(mr, attrs, addr1, l, &result)) {
> +            /* 'result' contains the error, keep going. */
> +        } else if (!memory_access_is_direct(mr, true)) {
>              release_lock |= prepare_mmio_access(mr);
>              l = memory_access_size(mr, l, addr1);
>              /* XXX: could force current_cpu to NULL to avoid
> @@ -2810,9 +2833,13 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
>      hwaddr l;
>      hwaddr addr1;
>      MemoryRegion *mr;
> +    MemTxResult result = MEMTX_OK;
>  
>      l = len;
>      mr = flatview_translate(fv, addr, &addr1, &l, true, attrs);
> +    if (!flatview_access_allowed(mr, attrs, addr, len, &result)) {
> +        return result;
> +    }
>      return flatview_write_continue(fv, addr, attrs, buf, len,
>                                     addr1, l, mr);
>  }
> @@ -2831,7 +2858,9 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
>  
>      fuzz_dma_read_cb(addr, len, mr);
>      for (;;) {
> -        if (!memory_access_is_direct(mr, false)) {
> +        if (!flatview_access_allowed(mr, attrs, addr1, l, &result)) {
> +            /* 'result' contains the error, keep going. */
> +        } else if (!memory_access_is_direct(mr, false)) {
>              /* I/O case */
>              release_lock |= prepare_mmio_access(mr);
>              l = memory_access_size(mr, l, addr1);
> @@ -2871,9 +2900,13 @@ static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
>      hwaddr l;
>      hwaddr addr1;
>      MemoryRegion *mr;
> +    MemTxResult result = MEMTX_OK;
>  
>      l = len;
>      mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
> +    if (!flatview_access_allowed(mr, attrs, addr, len, &result)) {
> +        return result;
> +    }
>      return flatview_read_continue(fv, addr, attrs, buf, len,
>                                    addr1, l, mr);
>  }
> -- 
> 2.31.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH v2 5/5] softmmu/physmem: Have flaview API check MemTxAttrs::bus_perm field

[Philippe Mathieu-Daudé]

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

Re: [RFC PATCH v2 5/5] softmmu/physmem: Have flaview API check MemTxAttrs::bus_perm field

[Peter Xu]

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

Re: [RFC PATCH v2 5/5] softmmu/physmem: Have flaview API check MemTxAttrs::bus_perm field

[David Hildenbrand]

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

Re: [RFC PATCH v2 5/5] softmmu/physmem: Have flaview API check MemTxAttrs::bus_perm field

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1917 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH v2 5/5] softmmu/physmem: Have flaview API check MemTxAttrs::bus_perm field

[Philippe Mathieu-Daudé]

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;
}

Re: [RFC PATCH v2 5/5] softmmu/physmem: Have flaview API check MemTxAttrs::bus_perm field

[Peter Maydell]

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

Re: [RFC PATCH v2 5/5] softmmu/physmem: Have flaview API check MemTxAttrs::bus_perm field

[Philippe Mathieu-Daudé]

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.

Re: [RFC PATCH v2 0/5] physmem: Have flaview API check bus permission from MemTxAttrs argument

[Peter Maydell]

On Mon, 23 Aug 2021 at 17:42, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> This series aim to kill a recent class of bug, the infamous
> "DMA reentrancy" issues found by Alexander while fuzzing.
>
> Introduce the 'bus_perm' field in MemTxAttrs, defining 3 bits:
>
> - MEMTXPERM_UNSPECIFIED (current default, unchanged behavior)
> - MEMTXPERM_UNRESTRICTED (allow list approach)
> - MEMTXPERM_RAM_DEVICE (example of deny list approach)
>
> If a transaction permission is not allowed (for example access
> to non-RAM device), we return the specific MEMTX_BUS_ERROR.
>
> Permissions are checked in after the flatview is resolved, and
> before the access is done, in a new function: flatview_access_allowed().

So I'm not going to say 'no' to this, because we have a real
recursive-device-handling problem and I don't have a better
idea to hand, but the thing about this is that we end up with
behaviour which is not what the real hardware does. I'm not
aware of any DMA device which has this kind of "can only DMA
to/from RAM, and aborts on access to a device" behaviour...

-- PMM

Re: [RFC PATCH v2 0/5] physmem: Have flaview API check bus permission from MemTxAttrs argument

[Peter Xu]

On Mon, Aug 23, 2021 at 08:10:50PM +0100, Peter Maydell wrote:
> On Mon, 23 Aug 2021 at 17:42, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >
> > This series aim to kill a recent class of bug, the infamous
> > "DMA reentrancy" issues found by Alexander while fuzzing.
> >
> > Introduce the 'bus_perm' field in MemTxAttrs, defining 3 bits:
> >
> > - MEMTXPERM_UNSPECIFIED (current default, unchanged behavior)
> > - MEMTXPERM_UNRESTRICTED (allow list approach)
> > - MEMTXPERM_RAM_DEVICE (example of deny list approach)
> >
> > If a transaction permission is not allowed (for example access
> > to non-RAM device), we return the specific MEMTX_BUS_ERROR.
> >
> > Permissions are checked in after the flatview is resolved, and
> > before the access is done, in a new function: flatview_access_allowed().
> 
> So I'm not going to say 'no' to this, because we have a real
> recursive-device-handling problem and I don't have a better
> idea to hand, but the thing about this is that we end up with
> behaviour which is not what the real hardware does. I'm not
> aware of any DMA device which has this kind of "can only DMA
> to/from RAM, and aborts on access to a device" behaviour...

Sorry for not being familiar with the context - is there more info regarding
the problem to fix?  I'm looking at the links mentioned in the old series:

https://lore.kernel.org/qemu-devel/20200903110831.353476-12-philmd@redhat.com/
https://bugs.launchpad.net/qemu/+bug/1886362
https://bugs.launchpad.net/qemu/+bug/1888606

They seem all marked as fixed now.

Thanks,

-- 
Peter Xu

Re: [RFC PATCH v2 0/5] physmem: Have flaview API check bus permission from MemTxAttrs argument

[Alexander Bulekov]

On 210823 1650, Peter Xu wrote:
> On Mon, Aug 23, 2021 at 08:10:50PM +0100, Peter Maydell wrote:
> > On Mon, 23 Aug 2021 at 17:42, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > >
> > > This series aim to kill a recent class of bug, the infamous
> > > "DMA reentrancy" issues found by Alexander while fuzzing.
> > >
> > > Introduce the 'bus_perm' field in MemTxAttrs, defining 3 bits:
> > >
> > > - MEMTXPERM_UNSPECIFIED (current default, unchanged behavior)
> > > - MEMTXPERM_UNRESTRICTED (allow list approach)
> > > - MEMTXPERM_RAM_DEVICE (example of deny list approach)
> > >
> > > If a transaction permission is not allowed (for example access
> > > to non-RAM device), we return the specific MEMTX_BUS_ERROR.
> > >
> > > Permissions are checked in after the flatview is resolved, and
> > > before the access is done, in a new function: flatview_access_allowed().
> > 
> > So I'm not going to say 'no' to this, because we have a real
> > recursive-device-handling problem and I don't have a better
> > idea to hand, but the thing about this is that we end up with
> > behaviour which is not what the real hardware does. I'm not
> > aware of any DMA device which has this kind of "can only DMA
> > to/from RAM, and aborts on access to a device" behaviour...
> 
> Sorry for not being familiar with the context - is there more info regarding
> the problem to fix?  I'm looking at the links mentioned in the old series:
> 
> https://lore.kernel.org/qemu-devel/20200903110831.353476-12-philmd@redhat.com/
> https://bugs.launchpad.net/qemu/+bug/1886362
> https://bugs.launchpad.net/qemu/+bug/1888606
> 
> They seem all marked as fixed now.

Here are some that should still reproduce:
https://gitlab.com/qemu-project/qemu/-/issues/542
https://gitlab.com/qemu-project/qemu/-/issues/540
https://gitlab.com/qemu-project/qemu/-/issues/541
https://gitlab.com/qemu-project/qemu/-/issues/62
https://lore.kernel.org/qemu-devel/20210218140629.373646-1-ppandit@redhat.com/ (CVE-2021-20255)

There's also this one, that I don't think I ever created a bug report
for (working on it now):
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=33247
-Alex

> 
> Thanks,
> 
> -- 
> Peter Xu
> 

Re: [RFC PATCH v2 0/5] physmem: Have flaview API check bus permission from MemTxAttrs argument

[Philippe Mathieu-Daudé]

On 8/24/21 12:26 AM, Alexander Bulekov wrote:
> On 210823 1650, Peter Xu wrote:
>> On Mon, Aug 23, 2021 at 08:10:50PM +0100, Peter Maydell wrote:
>>> On Mon, 23 Aug 2021 at 17:42, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>>
>>>> This series aim to kill a recent class of bug, the infamous
>>>> "DMA reentrancy" issues found by Alexander while fuzzing.
>>>>
>>>> Introduce the 'bus_perm' field in MemTxAttrs, defining 3 bits:
>>>>
>>>> - MEMTXPERM_UNSPECIFIED (current default, unchanged behavior)
>>>> - MEMTXPERM_UNRESTRICTED (allow list approach)
>>>> - MEMTXPERM_RAM_DEVICE (example of deny list approach)
>>>>
>>>> If a transaction permission is not allowed (for example access
>>>> to non-RAM device), we return the specific MEMTX_BUS_ERROR.
>>>>
>>>> Permissions are checked in after the flatview is resolved, and
>>>> before the access is done, in a new function: flatview_access_allowed().
>>>
>>> So I'm not going to say 'no' to this, because we have a real
>>> recursive-device-handling problem and I don't have a better
>>> idea to hand, but the thing about this is that we end up with
>>> behaviour which is not what the real hardware does. I'm not
>>> aware of any DMA device which has this kind of "can only DMA
>>> to/from RAM, and aborts on access to a device" behaviour...
>>
>> Sorry for not being familiar with the context - is there more info regarding
>> the problem to fix?  I'm looking at the links mentioned in the old series:
>>
>> https://lore.kernel.org/qemu-devel/20200903110831.353476-12-philmd@redhat.com/
>> https://bugs.launchpad.net/qemu/+bug/1886362
>> https://bugs.launchpad.net/qemu/+bug/1888606
>>
>> They seem all marked as fixed now.
> 
> Here are some that should still reproduce:
> https://gitlab.com/qemu-project/qemu/-/issues/542
> https://gitlab.com/qemu-project/qemu/-/issues/540
> https://gitlab.com/qemu-project/qemu/-/issues/541
> https://gitlab.com/qemu-project/qemu/-/issues/62
> https://lore.kernel.org/qemu-devel/20210218140629.373646-1-ppandit@redhat.com/ (CVE-2021-20255)

Also 305, 451, 557.

Issues list tracked here:
https://gitlab.com/qemu-project/qemu/-/issues/556
(Thanks Alex for updating it!)

> 
> There's also this one, that I don't think I ever created a bug report
> for (working on it now):
> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=33247
> -Alex
> 
>>
>> Thanks,
>>
>> -- 
>> Peter Xu
>>
> 

Re: [RFC PATCH v2 0/5] physmem: Have flaview API check bus permission from MemTxAttrs argument

[Peter Maydell]

On Mon, 23 Aug 2021 at 21:50, Peter Xu <peterx@redhat.com> wrote:
>
> On Mon, Aug 23, 2021 at 08:10:50PM +0100, Peter Maydell wrote:
> > On Mon, 23 Aug 2021 at 17:42, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > >
> > > This series aim to kill a recent class of bug, the infamous
> > > "DMA reentrancy" issues found by Alexander while fuzzing.
> > >
> > > Introduce the 'bus_perm' field in MemTxAttrs, defining 3 bits:
> > >
> > > - MEMTXPERM_UNSPECIFIED (current default, unchanged behavior)
> > > - MEMTXPERM_UNRESTRICTED (allow list approach)
> > > - MEMTXPERM_RAM_DEVICE (example of deny list approach)
> > >
> > > If a transaction permission is not allowed (for example access
> > > to non-RAM device), we return the specific MEMTX_BUS_ERROR.
> > >
> > > Permissions are checked in after the flatview is resolved, and
> > > before the access is done, in a new function: flatview_access_allowed().
> >
> > So I'm not going to say 'no' to this, because we have a real
> > recursive-device-handling problem and I don't have a better
> > idea to hand, but the thing about this is that we end up with
> > behaviour which is not what the real hardware does. I'm not
> > aware of any DMA device which has this kind of "can only DMA
> > to/from RAM, and aborts on access to a device" behaviour...
>
> Sorry for not being familiar with the context - is there more info regarding
> the problem to fix?

So, the general problem is that we have a whole class of bugs
that look like this:

 * Device A is DMA-capable. It also has a set of memory
   mapped registers which can be used to control it.
 * Malicious guest code (or the fuzzer) programs A's DMA
   engine to do a DMA read or write to the address where
   A's own registers are mapped.
 * Typically, the MemoryRegionOps write function for the
   register block will handle the "write to start-dma
   register" by doing the DMA, ie calling address_space_write(),
   pci_dma_write(), or equivalent. Because of the target address
   the guest code has set, that will result in the memory
   subsystem calling back into the same MemoryRegionOps
   write function, recursively.
 * Our code implementing the model of device A is not at
   all expecting this re-entrancy, and might crash, access
   freed memory, or otherwise misbehave.

You can elaborate on that basic scenario, for instance with
a loop of multiple devices where you program device A to
do a DMA write to device B's registers which starts device B doing
a DMA write to A's registers. Nor is it inherently limited to
DMA -- device A could be made to assert a qemu_irq that is
connected to device B in a way that causes device B to
do something that results in code calling back into device A;
or maybe device A DMAs to a register in device B that implements
a device-reset on device A. DMA is just the easiest for guest
code to set up and has the least restrictions on how it's
connected up.

In some specific cases we have "fixed" individual instances
of this bug by putting in checks or changes to whatever device
model the fuzzer happened to find a problem with, or put in
slightly wider-scale attempts to catch this (eg commit 22dc8663d9f
which prevents re-entering a NIC device's packet-rx callback
function if the guest has set it up so that a received packet
results in DMA that triggers another received packet). But
we don't have a coherent model of how we ought to structure
device models that can avoid this problem in a general way,
and I think that until we do we're liable to keep running into
specific bugs, some of which will be (or at least be labelled
as) "security issues".

Philippe's series here tries to fix this for at least any
variety of this bug where there is a DMA access in the
loop, by forbidding DMA accesses to MMIO regions not backed
by RAM. That works, in that it breaks the loop; as I
mentioned in my other email, it does it in a way that's
not the way real h/w behaves. Unfortunately "what does real
h/w do?" is not in this situation necessarily a helpful
guide for QEMU design: I think that real hardware:
(a) often doesn't see the same kind of problem because
   the design will usually decouple the DMA engine from
   the register-access logic in a way that means it
   naturally doesn't literally lock up
(b) often won't have been designed to deal with "software
   programs a DMA-to-self" either, but the threat model
   for real hw is different, in that software has many ways
   of making the overall system crash, hang or misbehave;
   it often doesn't have the "need to allow untrusted
   software to touch this device" situation.

One could have QEMU work somewhat like (a) by mandating
that all DMA of any kind was done in separate bottom-half
routines and not directly from the register-write code.
That would probably reduce performance and be a lot of
code to restructure. It would deal also with another class
of "guest code can make QEMU hang by programming it to do
an enormous DMA all at once or by setting up an infinitely
looping chain of DMA commands" bugs, though.

I was vaguely tossing an idea around in the back of my mind
about whether you could have a flag on devices that marked
them as "this device is currently involved in IO", such that
you could then just fail the last DMA (or qemu_irq_set, or
whatever) that would complete the loop back to a device that
was already doing IO. But that would need a lot of thinking
through to figure out if it's feasible, and it's probably
a lot of code change.

thanks
-- PMM

Re: [RFC PATCH v2 0/5] physmem: Have flaview API check bus permission from MemTxAttrs argument

[Gerd Hoffmann]

  Hi,

> I was vaguely tossing an idea around in the back of my mind
> about whether you could have a flag on devices that marked
> them as "this device is currently involved in IO", such that
> you could then just fail the last DMA (or qemu_irq_set, or
> whatever) that would complete the loop back to a device that
> was already doing IO. But that would need a lot of thinking
> through to figure out if it's feasible, and it's probably
> a lot of code change.

Quick & dirty hack trying the above.  Not much code, it is opt-in per
MemoryRegion (so less overhead for devices which already handle all DMA
in a BH), tracks state in DeviceState.  Adds a check to a rather hot
code path though.  Not tested yet (stopped investigating when I noticed
Philippe tries to fix the same thing with another approach).  Not
benchmarked.

Maybe it helps ...

take care,
  Gerd

From 80e58a2cd2c630f0bddd9d0eaee71abb7eeb9440 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 17 Aug 2021 07:35:37 +0200
Subject: [PATCH] allow track active mmio handlers

---
 include/exec/memory.h  |  1 +
 include/hw/qdev-core.h |  1 +
 softmmu/memory.c       | 24 ++++++++++++++++++++++--
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index c3d417d317f0..b1883d45e817 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -265,6 +265,7 @@ struct MemoryRegionOps {
          */
         bool unaligned;
     } impl;
+    bool block_reenter;
 };
 
 typedef struct MemoryRegionClass {
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bafc311bfa1b..4cf281a81fa9 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -191,6 +191,7 @@ struct DeviceState {
     int instance_id_alias;
     int alias_required_for_version;
     ResettableState reset;
+    bool io_handler_active;
 };
 
 struct DeviceListener {
diff --git a/softmmu/memory.c b/softmmu/memory.c
index bfedaf9c4dfc..5eb5dd465dd2 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -437,7 +437,18 @@ static MemTxResult  memory_region_read_accessor(MemoryRegion *mr,
 {
     uint64_t tmp;
 
-    tmp = mr->ops->read(mr->opaque, addr, size);
+    if (mr->ops->block_reenter) {
+        DeviceState *dev = DEVICE(mr->owner);
+        if (!dev->io_handler_active) {
+            dev->io_handler_active = true;
+            tmp = mr->ops->read(mr->opaque, addr, size);
+            dev->io_handler_active = false;
+        } else {
+            tmp = MEMTX_OK;
+        }
+    } else {
+        tmp = mr->ops->read(mr->opaque, addr, size);
+    }
     if (mr->subpage) {
         trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size);
     } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) {
@@ -489,7 +500,16 @@ static MemTxResult memory_region_write_accessor(MemoryRegion *mr,
         trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size,
                                       memory_region_name(mr));
     }
-    mr->ops->write(mr->opaque, addr, tmp, size);
+    if (mr->ops->block_reenter) {
+        DeviceState *dev = DEVICE(mr->owner);
+        if (!dev->io_handler_active) {
+            dev->io_handler_active = true;
+            mr->ops->write(mr->opaque, addr, tmp, size);
+            dev->io_handler_active = false;
+        }
+    } else {
+        mr->ops->write(mr->opaque, addr, tmp, size);
+    }
     return MEMTX_OK;
 }
 
-- 
2.31.1

Re: [RFC PATCH v2 0/5] physmem: Have flaview API check bus permission from MemTxAttrs argument

[Li Qiang]

Gerd Hoffmann <kraxel@redhat.com> 于2021年8月24日周二 下午8:02写道：
>
>   Hi,
>
> > I was vaguely tossing an idea around in the back of my mind
> > about whether you could have a flag on devices that marked
> > them as "this device is currently involved in IO", such that
> > you could then just fail the last DMA (or qemu_irq_set, or
> > whatever) that would complete the loop back to a device that
> > was already doing IO. But that would need a lot of thinking
> > through to figure out if it's feasible, and it's probably
> > a lot of code change.
>
> Quick & dirty hack trying the above.  Not much code, it is opt-in per
> MemoryRegion (so less overhead for devices which already handle all DMA
> in a BH), tracks state in DeviceState.  Adds a check to a rather hot
> code path though.  Not tested yet (stopped investigating when I noticed
> Philippe tries to fix the same thing with another approach).  Not
> benchmarked.
>
> Maybe it helps ...
>

Gerd's patch just remind my approach here, Just add here:

https://mail.gnu.org/archive/html/qemu-devel/2020-09/msg00906.html

But I check and record it in the device MR handler.

Thanks,
Li Qiang


> take care,
>   Gerd
>
> From 80e58a2cd2c630f0bddd9d0eaee71abb7eeb9440 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Tue, 17 Aug 2021 07:35:37 +0200
> Subject: [PATCH] allow track active mmio handlers
>
> ---
>  include/exec/memory.h  |  1 +
>  include/hw/qdev-core.h |  1 +
>  softmmu/memory.c       | 24 ++++++++++++++++++++++--
>  3 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index c3d417d317f0..b1883d45e817 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -265,6 +265,7 @@ struct MemoryRegionOps {
>           */
>          bool unaligned;
>      } impl;
> +    bool block_reenter;
>  };
>
>  typedef struct MemoryRegionClass {
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index bafc311bfa1b..4cf281a81fa9 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -191,6 +191,7 @@ struct DeviceState {
>      int instance_id_alias;
>      int alias_required_for_version;
>      ResettableState reset;
> +    bool io_handler_active;
>  };
>
>  struct DeviceListener {
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bfedaf9c4dfc..5eb5dd465dd2 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -437,7 +437,18 @@ static MemTxResult  memory_region_read_accessor(MemoryRegion *mr,
>  {
>      uint64_t tmp;
>
> -    tmp = mr->ops->read(mr->opaque, addr, size);
> +    if (mr->ops->block_reenter) {
> +        DeviceState *dev = DEVICE(mr->owner);
> +        if (!dev->io_handler_active) {
> +            dev->io_handler_active = true;
> +            tmp = mr->ops->read(mr->opaque, addr, size);
> +            dev->io_handler_active = false;
> +        } else {
> +            tmp = MEMTX_OK;
> +        }
> +    } else {
> +        tmp = mr->ops->read(mr->opaque, addr, size);
> +    }
>      if (mr->subpage) {
>          trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size);
>      } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) {
> @@ -489,7 +500,16 @@ static MemTxResult memory_region_write_accessor(MemoryRegion *mr,
>          trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size,
>                                        memory_region_name(mr));
>      }
> -    mr->ops->write(mr->opaque, addr, tmp, size);
> +    if (mr->ops->block_reenter) {
> +        DeviceState *dev = DEVICE(mr->owner);
> +        if (!dev->io_handler_active) {
> +            dev->io_handler_active = true;
> +            mr->ops->write(mr->opaque, addr, tmp, size);
> +            dev->io_handler_active = false;
> +        }
> +    } else {
> +        mr->ops->write(mr->opaque, addr, tmp, size);
> +    }
>      return MEMTX_OK;
>  }
>
> --
> 2.31.1
>

Re: [RFC PATCH v2 0/5] physmem: Have flaview API check bus permission from MemTxAttrs argument

[Peter Xu]

Hi, Peter, Gerd,

On Tue, Aug 24, 2021 at 02:01:53PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > I was vaguely tossing an idea around in the back of my mind
> > about whether you could have a flag on devices that marked
> > them as "this device is currently involved in IO", such that
> > you could then just fail the last DMA (or qemu_irq_set, or
> > whatever) that would complete the loop back to a device that
> > was already doing IO. But that would need a lot of thinking
> > through to figure out if it's feasible, and it's probably
> > a lot of code change.

(Thanks for the write-up, Peter; it helps a lot)

> 
> Quick & dirty hack trying the above.  Not much code, it is opt-in per
> MemoryRegion (so less overhead for devices which already handle all DMA
> in a BH), tracks state in DeviceState.  Adds a check to a rather hot
> code path though.  Not tested yet (stopped investigating when I noticed
> Philippe tries to fix the same thing with another approach).  Not
> benchmarked.
> 
> Maybe it helps ...
> 
> take care,
>   Gerd
> 
> From 80e58a2cd2c630f0bddd9d0eaee71abb7eeb9440 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Tue, 17 Aug 2021 07:35:37 +0200
> Subject: [PATCH] allow track active mmio handlers
> 
> ---
>  include/exec/memory.h  |  1 +
>  include/hw/qdev-core.h |  1 +
>  softmmu/memory.c       | 24 ++++++++++++++++++++++--
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index c3d417d317f0..b1883d45e817 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -265,6 +265,7 @@ struct MemoryRegionOps {
>           */
>          bool unaligned;
>      } impl;
> +    bool block_reenter;
>  };
>  
>  typedef struct MemoryRegionClass {
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index bafc311bfa1b..4cf281a81fa9 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -191,6 +191,7 @@ struct DeviceState {
>      int instance_id_alias;
>      int alias_required_for_version;
>      ResettableState reset;
> +    bool io_handler_active;
>  };
>  
>  struct DeviceListener {
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bfedaf9c4dfc..5eb5dd465dd2 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -437,7 +437,18 @@ static MemTxResult  memory_region_read_accessor(MemoryRegion *mr,
>  {
>      uint64_t tmp;
>  
> -    tmp = mr->ops->read(mr->opaque, addr, size);
> +    if (mr->ops->block_reenter) {
> +        DeviceState *dev = DEVICE(mr->owner);
> +        if (!dev->io_handler_active) {
> +            dev->io_handler_active = true;
> +            tmp = mr->ops->read(mr->opaque, addr, size);
> +            dev->io_handler_active = false;
> +        } else {
> +            tmp = MEMTX_OK;
> +        }
> +    } else {
> +        tmp = mr->ops->read(mr->opaque, addr, size);
> +    }
>      if (mr->subpage) {
>          trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size);
>      } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) {
> @@ -489,7 +500,16 @@ static MemTxResult memory_region_write_accessor(MemoryRegion *mr,
>          trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size,
>                                        memory_region_name(mr));
>      }
> -    mr->ops->write(mr->opaque, addr, tmp, size);
> +    if (mr->ops->block_reenter) {
> +        DeviceState *dev = DEVICE(mr->owner);
> +        if (!dev->io_handler_active) {
> +            dev->io_handler_active = true;
> +            mr->ops->write(mr->opaque, addr, tmp, size);
> +            dev->io_handler_active = false;
> +        }
> +    } else {
> +        mr->ops->write(mr->opaque, addr, tmp, size);
> +    }
>      return MEMTX_OK;
>  }

Can I read this as a better approach if it still allows P2P so things that
Paolo and Qiang used to mention will still work?

https://lore.kernel.org/qemu-devel/7e4fd726-07e9-dc09-d66b-5692dd51820f@redhat.com/
https://lore.kernel.org/qemu-devel/CAKXe6S+v4z_PYbZ6MMzEZk7Q0Qc+q9tzL+a8918U_-XR=aj7RA@mail.gmail.com/

Can we do that similarly for qemu_set_irq() and friends but based on IRQState?

Thanks,

-- 
Peter Xu

Re: [RFC PATCH v2 0/5] physmem: Have flaview API check bus permission from MemTxAttrs argument

[Edgar E. Iglesias]

On Mon, Aug 23, 2021 at 08:10:50PM +0100, Peter Maydell wrote:
> On Mon, 23 Aug 2021 at 17:42, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >
> > This series aim to kill a recent class of bug, the infamous
> > "DMA reentrancy" issues found by Alexander while fuzzing.
> >
> > Introduce the 'bus_perm' field in MemTxAttrs, defining 3 bits:
> >
> > - MEMTXPERM_UNSPECIFIED (current default, unchanged behavior)
> > - MEMTXPERM_UNRESTRICTED (allow list approach)
> > - MEMTXPERM_RAM_DEVICE (example of deny list approach)
> >
> > If a transaction permission is not allowed (for example access
> > to non-RAM device), we return the specific MEMTX_BUS_ERROR.
> >
> > Permissions are checked in after the flatview is resolved, and
> > before the access is done, in a new function: flatview_access_allowed().
> 
> So I'm not going to say 'no' to this, because we have a real
> recursive-device-handling problem and I don't have a better
> idea to hand, but the thing about this is that we end up with
> behaviour which is not what the real hardware does. I'm not
> aware of any DMA device which has this kind of "can only DMA
> to/from RAM, and aborts on access to a device" behaviour...
>

Yes, I agree.

Having said that, There are DMA devices that do indicate to the
interconnect and peripherals if they are targeting "normal" memory
or device (together with cacheability, buffering and ordering
attributes). Accessing registers of a device with "normal" memory
and cache attributes is not a good idea and may lead to all kinds
of weirdness on real HW.

IMO, it would be better to model something like those attributes
rather than permissions. And it's probably a good idea to not
call it MEMTXPERM_RAM_DEVICE since in the AMBA documentation
it's Normal Memory vs Device (calling it RAM_DEVICE is confusing
to me at least).

Adding a "memory" attribute to MemTxAttrs may be enough?
If it's set, the access would only target memories. If targeting a device
it could perhaps be logged and dropped rather than aborted?
If not set (the default), accesses would target anything...

Cheers,
Edgar

Re: [RFC PATCH v2 0/5] physmem: Have flaview API check bus permission from MemTxAttrs argument

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2210 bytes --]

On Mon, Aug 23, 2021 at 08:10:50PM +0100, Peter Maydell wrote:
> On Mon, 23 Aug 2021 at 17:42, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >
> > This series aim to kill a recent class of bug, the infamous
> > "DMA reentrancy" issues found by Alexander while fuzzing.
> >
> > Introduce the 'bus_perm' field in MemTxAttrs, defining 3 bits:
> >
> > - MEMTXPERM_UNSPECIFIED (current default, unchanged behavior)
> > - MEMTXPERM_UNRESTRICTED (allow list approach)
> > - MEMTXPERM_RAM_DEVICE (example of deny list approach)
> >
> > If a transaction permission is not allowed (for example access
> > to non-RAM device), we return the specific MEMTX_BUS_ERROR.
> >
> > Permissions are checked in after the flatview is resolved, and
> > before the access is done, in a new function: flatview_access_allowed().
> 
> So I'm not going to say 'no' to this, because we have a real
> recursive-device-handling problem and I don't have a better
> idea to hand, but the thing about this is that we end up with
> behaviour which is not what the real hardware does. I'm not
> aware of any DMA device which has this kind of "can only DMA
> to/from RAM, and aborts on access to a device" behaviour...

Points that have come up in previous discussions on this topic:

- We probably won't be able to find out the actual hardware behavior for
  all device models in QEMU. Strict RAM-only DMA restrictions can be
  merged early in the QEMU 6.2 development cycle so there's plenty of
  time to identify regressions. The benefit of a strict policy is that
  we eliminate this class of bugs for most devices now and in the
  future.

- If the risk of regressions is too high, then this API can be used on a
  case-by-case basis to fix bugs such as those identified by Alexander's
  fuzzer. We'll be plagued with this class of bugs in the future though,
  so I prefer a strict policy.

- DMA capabilities depend on the host bus adapter/controller. In order
  to faithfully emulate real hardware we need to know how it behaves.
  That needs to be done for each host bus adapter (e.g. PCI
  controllers).

- SysBus devices each have their own behavior wrt device-to-device DMA.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH v2 0/5] physmem: Have flaview API check bus permission from MemTxAttrs argument

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

On Mon, Aug 23, 2021 at 06:41:52PM +0200, Philippe Mathieu-Daudé wrote:
> This series aim to kill a recent class of bug, the infamous
> "DMA reentrancy" issues found by Alexander while fuzzing.
> 
> Introduce the 'bus_perm' field in MemTxAttrs, defining 3 bits:
> 
> - MEMTXPERM_UNSPECIFIED (current default, unchanged behavior)
> - MEMTXPERM_UNRESTRICTED (allow list approach)
> - MEMTXPERM_RAM_DEVICE (example of deny list approach)

These names don't hint at their descriptions. I wouldn't be able to tell
what they do based on the name. I'll think more about naming after
reviewing the patches.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]