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

[RFC PATCH v3 0/3] 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 'memory' field in MemTxAttrs, allowing to restrict
a controller to memories (and not devices).

If a transaction permission is not allowed (for example access
to 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().

Since v2 [1]:
- Addressed review comments:
  - reword arm_gicv3 description (pm215)
  - merged patches 3/4/5 (peterx & dhildenb)
  - simplify flatview_access_allowed() logic (stefanha)
  - drop MEMTXPERM enum and reword following AMBA terminology (edgar)

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

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

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

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

Philippe Mathieu-Daudé (3):
  hw/intc/arm_gicv3: Check for !MEMTX_OK instead of MEMTX_ERROR
  softmmu/physmem: Simplify flatview_write and
    address_space_access_valid
  softmmu/physmem: Introduce MemTxAttrs::memory field and
    MEMTX_BUS_ERROR

 include/exec/memattrs.h    |  9 +++++++
 hw/intc/arm_gicv3_redist.c |  4 +--
 softmmu/physmem.c          | 54 +++++++++++++++++++++++++++++++-------
 3 files changed, 55 insertions(+), 12 deletions(-)

-- 
2.33.1

[RFC PATCH v3 1/3] hw/intc/arm_gicv3: Check for !MEMTX_OK instead of MEMTX_ERROR

[Philippe Mathieu-Daudé]

Quoting Peter Maydell:

 "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."

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

Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/intc/arm_gicv3_redist.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
index c8ff3eca085..99b11ca5eee 100644
--- a/hw/intc/arm_gicv3_redist.c
+++ b/hw/intc/arm_gicv3_redist.c
@@ -462,7 +462,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);
@@ -521,7 +521,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.33.1

[RFC PATCH v3 2/3] softmmu/physmem: Simplify flatview_write and address_space_access_valid

[Philippe Mathieu-Daudé]

Remove unuseful local 'result' variables.

Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Alexander Bulekov <alxndr@bu.edu>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
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 007e7526f0a..6c97a20107a 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2815,14 +2815,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.  */
@@ -3119,12 +3116,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.33.1

[RFC PATCH v3 3/3] softmmu/physmem: Introduce MemTxAttrs::memory field and MEMTX_BUS_ERROR

[Philippe Mathieu-Daudé]

Add the 'memory' bit to the memory attributes to restrict bus
controller accesses to memories.

Introduce flatview_access_allowed() to check bus permission
before running any bus transaction.

Have read/write accessors return MEMTX_BUS_ERROR if an access is
restricted.

There is no change for the default case where 'memory' is not set.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memattrs.h |  9 +++++++++
 softmmu/physmem.c       | 43 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 95f2d20d55b..f0063583ee2 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -35,6 +35,14 @@ typedef struct MemTxAttrs {
     unsigned int secure:1;
     /* Memory access is usermode (unprivileged) */
     unsigned int user:1;
+    /*
+     * Bus interconnect and peripherals can access anything (memories,
+     * devices) by default. By setting the 'memory' bit, bus transaction
+     * are restricted to "normal" memories (per the AMBA documentation)
+     * versus devices. Access to devices will be logged and rejected
+     * (see MEMTX_BUS_ERROR).
+     */
+    unsigned int memory:1;
     /* Requester ID (for MSI for example) */
     unsigned int requester_id:16;
     /* Invert endianness for this page */
@@ -66,6 +74,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
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 6c97a20107a..c03abcc0362 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2759,6 +2759,33 @@ 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
+ *
+ * Check if a memory transaction is allowed.
+ *
+ * Returns: true if transaction is allowed, false if denied.
+ */
+static inline bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs,
+                                           hwaddr addr, hwaddr len)
+{
+    if (likely(!attrs.memory)) {
+        return true;
+    }
+    if (memory_region_is_ram(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));
+    return false;
+}
+
 /* Called within RCU critical section.  */
 static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
                                            MemTxAttrs attrs,
@@ -2773,7 +2800,10 @@ 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 |= MEMTX_BUS_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
@@ -2818,6 +2848,9 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
 
     l = len;
     mr = flatview_translate(fv, addr, &addr1, &l, true, attrs);
+    if (!flatview_access_allowed(mr, attrs, addr, len)) {
+        return MEMTX_BUS_ERROR;
+    }
     return flatview_write_continue(fv, addr, attrs, buf, len,
                                    addr1, l, mr);
 }
@@ -2836,7 +2869,10 @@ 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 |= MEMTX_BUS_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);
@@ -2879,6 +2915,9 @@ static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
 
     l = len;
     mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
+    if (!flatview_access_allowed(mr, attrs, addr, len)) {
+        return MEMTX_BUS_ERROR;
+    }
     return flatview_read_continue(fv, addr, attrs, buf, len,
                                   addr1, l, mr);
 }
-- 
2.33.1

Re: [RFC PATCH v3 3/3] softmmu/physmem: Introduce MemTxAttrs::memory field and MEMTX_BUS_ERROR

[Richard Henderson]

On 12/15/21 10:24 AM, Philippe Mathieu-Daudé wrote:
> +static inline bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs,
> +                                           hwaddr addr, hwaddr len)

There's no need to mark this inline. Otherwise,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [RFC PATCH v3 3/3] softmmu/physmem: Introduce MemTxAttrs::memory field and MEMTX_BUS_ERROR

[Peter Maydell]

On Wed, 15 Dec 2021 at 18:24, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Add the 'memory' bit to the memory attributes to restrict bus
> controller accesses to memories.
>
> Introduce flatview_access_allowed() to check bus permission
> before running any bus transaction.
>
> Have read/write accessors return MEMTX_BUS_ERROR if an access is
> restricted.
>
> There is no change for the default case where 'memory' is not set.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/exec/memattrs.h |  9 +++++++++
>  softmmu/physmem.c       | 43 +++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index 95f2d20d55b..f0063583ee2 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -35,6 +35,14 @@ typedef struct MemTxAttrs {
>      unsigned int secure:1;
>      /* Memory access is usermode (unprivileged) */
>      unsigned int user:1;
> +    /*
> +     * Bus interconnect and peripherals can access anything (memories,
> +     * devices) by default. By setting the 'memory' bit, bus transaction
> +     * are restricted to "normal" memories (per the AMBA documentation)
> +     * versus devices. Access to devices will be logged and rejected
> +     * (see MEMTX_BUS_ERROR).
> +     */
> +    unsigned int memory:1;
>      /* Requester ID (for MSI for example) */
>      unsigned int requester_id:16;
>      /* Invert endianness for this page */
> @@ -66,6 +74,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 */

This is kind of odd naming, because MEMTX_DECODE_ERROR already means
"bus/interconnect returned an error" and it generally translates
into what at the OS level gets called a "bus error"...

-- PMM

Re: [RFC PATCH v3 3/3] softmmu/physmem: Introduce MemTxAttrs::memory field and MEMTX_BUS_ERROR

[Philippe Mathieu-Daudé]

On 12/17/21 23:34, Peter Maydell wrote:
> On Wed, 15 Dec 2021 at 18:24, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Add the 'memory' bit to the memory attributes to restrict bus
>> controller accesses to memories.
>>
>> Introduce flatview_access_allowed() to check bus permission
>> before running any bus transaction.
>>
>> Have read/write accessors return MEMTX_BUS_ERROR if an access is
>> restricted.
>>
>> There is no change for the default case where 'memory' is not set.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/exec/memattrs.h |  9 +++++++++
>>  softmmu/physmem.c       | 43 +++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 50 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
>> index 95f2d20d55b..f0063583ee2 100644
>> --- a/include/exec/memattrs.h
>> +++ b/include/exec/memattrs.h
>> @@ -35,6 +35,14 @@ typedef struct MemTxAttrs {
>>      unsigned int secure:1;
>>      /* Memory access is usermode (unprivileged) */
>>      unsigned int user:1;
>> +    /*
>> +     * Bus interconnect and peripherals can access anything (memories,
>> +     * devices) by default. By setting the 'memory' bit, bus transaction
>> +     * are restricted to "normal" memories (per the AMBA documentation)
>> +     * versus devices. Access to devices will be logged and rejected
>> +     * (see MEMTX_BUS_ERROR).
>> +     */
>> +    unsigned int memory:1;
>>      /* Requester ID (for MSI for example) */
>>      unsigned int requester_id:16;
>>      /* Invert endianness for this page */
>> @@ -66,6 +74,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 */
> 
> This is kind of odd naming, because MEMTX_DECODE_ERROR already means
> "bus/interconnect returned an error" and it generally translates
> into what at the OS level gets called a "bus error"...

MEMTX_DECODE_ERROR is "nothing at that address". We want a name
for "there is something, but you don't have access to it".
Maybe MEMTX_ILLEGAL_ERROR?

Re: [RFC PATCH v3 3/3] softmmu/physmem: Introduce MemTxAttrs::memory field and MEMTX_BUS_ERROR

[Richard Henderson]

On 12/17/21 3:18 PM, Philippe Mathieu-Daudé wrote:
>>>   #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 */
>>
>> This is kind of odd naming, because MEMTX_DECODE_ERROR already means
>> "bus/interconnect returned an error" and it generally translates
>> into what at the OS level gets called a "bus error"...
> 
> MEMTX_DECODE_ERROR is "nothing at that address". We want a name
> for "there is something, but you don't have access to it".
> Maybe MEMTX_ILLEGAL_ERROR?

ILLEGAL doesn't convey much.  MEMTX_ACCESS_ERROR?

r~

Re: [RFC PATCH v3 3/3] softmmu/physmem: Introduce MemTxAttrs::memory field and MEMTX_BUS_ERROR

[Philippe Mathieu-Daudé]

On 12/18/21 03:07, Richard Henderson wrote:
> On 12/17/21 3:18 PM, Philippe Mathieu-Daudé wrote:
>>>>   #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 */
>>>
>>> This is kind of odd naming, because MEMTX_DECODE_ERROR already means
>>> "bus/interconnect returned an error" and it generally translates
>>> into what at the OS level gets called a "bus error"...
>>
>> MEMTX_DECODE_ERROR is "nothing at that address". We want a name
>> for "there is something, but you don't have access to it".
>> Maybe MEMTX_ILLEGAL_ERROR?
> 
> ILLEGAL doesn't convey much.  MEMTX_ACCESS_ERROR?

OK, such:

  #define MEMTX_ACCESS_ERROR      (1U << 2) /* access denied */

alternatively MEMTX_PERM_ERROR.

Re: [RFC PATCH v3 1/3] hw/intc/arm_gicv3: Check for !MEMTX_OK instead of MEMTX_ERROR

[Philippe Mathieu-Daudé via]

Hi Peter,

Can you take this single patch via your arm tree?

Thanks,

Phil.

On 12/15/21 19:24, Philippe Mathieu-Daudé wrote:
> Quoting Peter Maydell:
> 
>  "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."
> 
> We are going to introduce more MemTxResult bits, so it is
> safer to check for !MEMTX_OK rather than MEMTX_ERROR.
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/intc/arm_gicv3_redist.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
> index c8ff3eca085..99b11ca5eee 100644
> --- a/hw/intc/arm_gicv3_redist.c
> +++ b/hw/intc/arm_gicv3_redist.c
> @@ -462,7 +462,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);
> @@ -521,7 +521,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);

Re: [RFC PATCH v3 1/3] hw/intc/arm_gicv3: Check for !MEMTX_OK instead of MEMTX_ERROR

[Peter Maydell]

On Wed, 19 Jan 2022 at 17:34, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Peter,
>
> Can you take this single patch via your arm tree?

Sure.

Applied to target-arm.next, thanks.

-- PMM

Re: [RFC PATCH v3 3/3] softmmu/physmem: Introduce MemTxAttrs::memory field and MEMTX_BUS_ERROR

[Stefan Hajnoczi]

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

On Wed, Dec 15, 2021 at 07:24:21PM +0100, Philippe Mathieu-Daudé wrote:
> Add the 'memory' bit to the memory attributes to restrict bus
> controller accesses to memories.
> 
> Introduce flatview_access_allowed() to check bus permission
> before running any bus transaction.
> 
> Have read/write accessors return MEMTX_BUS_ERROR if an access is
> restricted.
> 
> There is no change for the default case where 'memory' is not set.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/exec/memattrs.h |  9 +++++++++
>  softmmu/physmem.c       | 43 +++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 50 insertions(+), 2 deletions(-)

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

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

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

[Stefan Hajnoczi]

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

On Wed, Dec 15, 2021 at 07:24:18PM +0100, Philippe Mathieu-Daudé wrote:
> This series aim to kill a recent class of bug, the infamous
> "DMA reentrancy" issues found by Alexander while fuzzing.

I took a look at how to protect DMA transactions in VIRTIO devices. It
will require setting the MemTxAttrs for address_space_ld/st_le/be_cached
calls. Errors on write (store) can be ignored. Errors on read (load) are
a bit more questionable since the device performs some operation based
on the loaded value, but at this point the driver has already caused the
device to do something no correct driver does (as of today, it could
change in the future...) so undefined device behavior might be okay.

It would be easier to be confident if there was a single place to
disable DMA re-entrancy for a device. The currently proposed API
requires per-device code audits and fixes. It leaves decisions to the
developer of each device. This will be a lot of work to fix and we
cannot be confident that everything has been covered since this is an
opt-in mechanism.

For these reasons it seems likely that DMA re-entrancy issues will
continue to creep in. I think the only way to rule out this class of
bugs is to implement a centralized change that doesn't involve fixing
every DMA access in QEMU.

Thoughts?

Stefan

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

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

[Alexander Bulekov]

On 220124 1630, Stefan Hajnoczi wrote:
> On Wed, Dec 15, 2021 at 07:24:18PM +0100, Philippe Mathieu-Daudé wrote:
> > This series aim to kill a recent class of bug, the infamous
> > "DMA reentrancy" issues found by Alexander while fuzzing.
> 
> I took a look at how to protect DMA transactions in VIRTIO devices. It
> will require setting the MemTxAttrs for address_space_ld/st_le/be_cached
> calls. Errors on write (store) can be ignored. Errors on read (load) are
> a bit more questionable since the device performs some operation based
> on the loaded value, but at this point the driver has already caused the
> device to do something no correct driver does (as of today, it could
> change in the future...) so undefined device behavior might be okay.
> 
> It would be easier to be confident if there was a single place to
> disable DMA re-entrancy for a device. The currently proposed API
> requires per-device code audits and fixes. It leaves decisions to the
> developer of each device. This will be a lot of work to fix and we
> cannot be confident that everything has been covered since this is an
> opt-in mechanism.
> 
> For these reasons it seems likely that DMA re-entrancy issues will
> continue to creep in. I think the only way to rule out this class of
> bugs is to implement a centralized change that doesn't involve fixing
> every DMA access in QEMU.
> 
> Thoughts?

Hi Stefan,
Do you have some ideas about how to do this centrally?
There were at least two attempts to do this in a centralized way, but it
seems there is some worry that edge cases will break. However, I'm
not sure there were any concrete examples of such breakages.

[1] https://lore.kernel.org/all/20210824120153.altqys6jjiuxh35p@sirius.home.kraxel.org/
[2] https://lore.kernel.org/all/20211217030858.834822-1-alxndr@bu.edu/
(AFAIK Neither handles the BH->DMA->MMIO case, at the moment)

-Alex

> 
> Stefan

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

[Stefan Hajnoczi]

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

On Mon, Jan 24, 2022 at 11:50:10AM -0500, Alexander Bulekov wrote:
> On 220124 1630, Stefan Hajnoczi wrote:
> > On Wed, Dec 15, 2021 at 07:24:18PM +0100, Philippe Mathieu-Daudé wrote:
> > > This series aim to kill a recent class of bug, the infamous
> > > "DMA reentrancy" issues found by Alexander while fuzzing.
> > 
> > I took a look at how to protect DMA transactions in VIRTIO devices. It
> > will require setting the MemTxAttrs for address_space_ld/st_le/be_cached
> > calls. Errors on write (store) can be ignored. Errors on read (load) are
> > a bit more questionable since the device performs some operation based
> > on the loaded value, but at this point the driver has already caused the
> > device to do something no correct driver does (as of today, it could
> > change in the future...) so undefined device behavior might be okay.
> > 
> > It would be easier to be confident if there was a single place to
> > disable DMA re-entrancy for a device. The currently proposed API
> > requires per-device code audits and fixes. It leaves decisions to the
> > developer of each device. This will be a lot of work to fix and we
> > cannot be confident that everything has been covered since this is an
> > opt-in mechanism.
> > 
> > For these reasons it seems likely that DMA re-entrancy issues will
> > continue to creep in. I think the only way to rule out this class of
> > bugs is to implement a centralized change that doesn't involve fixing
> > every DMA access in QEMU.
> > 
> > Thoughts?
> 
> Hi Stefan,
> Do you have some ideas about how to do this centrally?
> There were at least two attempts to do this in a centralized way, but it
> seems there is some worry that edge cases will break. However, I'm
> not sure there were any concrete examples of such breakages.
> 
> [1] https://lore.kernel.org/all/20210824120153.altqys6jjiuxh35p@sirius.home.kraxel.org/
> [2] https://lore.kernel.org/all/20211217030858.834822-1-alxndr@bu.edu/
> (AFAIK Neither handles the BH->DMA->MMIO case, at the moment)

Regressions are the problem with defaulting to RAM-only DMA. There's no
way to avoid the risk if we change the default. On the other hand, it's
the only way to squash this class of bugs - most existing devices just
aren't written to cope with DMA re-entrancy.

The approach in your patch sounds good to me, but I haven't followed the
discussions so maybe there were valid reasons to look for alternatives.

Stefan

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