Re: [PATCH for 8.0 v2] memory: Prevent recursive memory access

[Alexander Bulekov]

On 230316 2124, Akihiko Odaki wrote:
> A guest may request ask a memory-mapped device to perform DMA. If the
> address specified for DMA is the device performing DMA, it will create
> recursion. It is very unlikely that device implementations are prepared
> for such an abnormal access, which can result in unpredictable behavior.
> 
> In particular, such a recursion breaks e1000e, a network device. If
> the device is configured to write the received packet to the register
> to trigger receiving, it triggers re-entry to the Rx logic of e1000e.
> This causes use-after-free since the Rx logic is not re-entrant.
> 
> As there should be no valid reason to perform recursive memory access,
> check for recursion before accessing memory-mapped device.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1543
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Hi Akihiko,
I think the spirit of this is similar to the fix I proposed here:
20230313082417.827484-1-alxndr@bu.edu/">https://lore.kernel.org/qemu-devel/20230313082417.827484-1-alxndr@bu.edu/

My version also addresses the following case, which we have found
instances of:
Device Foo Bottom Half -> DMA write to Device Foo Memory Region

That said, the patch is held up on some corner cases and it seems it
will not make it into 8.0. I guess we can add #1543 to the list of
issues in https://gitlab.com/qemu-project/qemu/-/issues/556

Thanks
-Alex

> ---
>  softmmu/memory.c | 79 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 62 insertions(+), 17 deletions(-)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 4699ba55ec..19c60ee1f0 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -50,6 +50,10 @@ static QTAILQ_HEAD(, AddressSpace) address_spaces
>  
>  static GHashTable *flat_views;
>  
> +static const Object **accessed_region_owners;
> +static size_t accessed_region_owners_capacity;
> +static size_t accessed_region_owners_num;
> +
>  typedef struct AddrRange AddrRange;
>  
>  /*
> @@ -1394,6 +1398,16 @@ bool memory_region_access_valid(MemoryRegion *mr,
>          return false;
>      }
>  
> +    for (size_t i = 0; i < accessed_region_owners_num; i++) {
> +        if (accessed_region_owners[i] == mr->owner) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "Invalid %s at addr 0x%" 
> HWADDR_PRIX
> +                          ", size %u, region '%s', reason: recursive 
> access\n",
> +                          is_write ? "write" : "read",
> +                          addr, size, memory_region_name(mr));
> +            return false;
> +        }
> +    }
> +
>      /* Treat zero as compatibility all valid */
>      if (!mr->ops->valid.max_access_size) {
>          return true;
> @@ -1413,6 +1427,34 @@ bool memory_region_access_valid(MemoryRegion *mr,
>      return true;
>  }
>  
> +static bool memory_region_access_start(MemoryRegion *mr,
> +                                       hwaddr addr,
> +                                       unsigned size,
> +                                       bool is_write,
> +                                       MemTxAttrs attrs)
> +{
> +    if (!memory_region_access_valid(mr, addr, size, is_write, attrs)) {
> +        return false;
> +    }
> +
> +    accessed_region_owners_num++;
> +    if (accessed_region_owners_num > accessed_region_owners_capacity) {
> +        accessed_region_owners_capacity = accessed_region_owners_num;
> +        accessed_region_owners = g_realloc_n(accessed_region_owners,
> +                                             accessed_region_owners_capacity,
> +                                             
> sizeof(*accessed_region_owners));
> +    }
> +
> +    accessed_region_owners[accessed_region_owners_num - 1] = mr->owner;
> +
> +    return true;
> +}
> +
> +static void memory_region_access_end(void)
> +{
> +    accessed_region_owners_num--;
> +}
> +
>  static MemTxResult memory_region_dispatch_read1(MemoryRegion *mr,
>                                                  hwaddr addr,
>                                                  uint64_t *pval,
> @@ -1450,12 +1492,13 @@ MemTxResult memory_region_dispatch_read(MemoryRegion 
> *mr,
>                                             mr->alias_offset + addr,
>                                             pval, op, attrs);
>      }
> -    if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
> +    if (!memory_region_access_start(mr, addr, size, false, attrs)) {
>          *pval = unassigned_mem_read(mr, addr, size);
>          return MEMTX_DECODE_ERROR;
>      }
>  
>      r = memory_region_dispatch_read1(mr, addr, pval, size, attrs);
> +    memory_region_access_end();
>      adjust_endianness(mr, pval, op);
>      return r;
>  }
> @@ -1493,13 +1536,14 @@ MemTxResult memory_region_dispatch_write(MemoryRegion 
> *mr,
>                                           MemTxAttrs attrs)
>  {
>      unsigned size = memop_size(op);
> +    MemTxResult result;
>  
>      if (mr->alias) {
>          return memory_region_dispatch_write(mr->alias,
>                                              mr->alias_offset + addr,
>                                              data, op, attrs);
>      }
> -    if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
> +    if (!memory_region_access_start(mr, addr, size, true, attrs)) {
>          unassigned_mem_write(mr, addr, data, size);
>          return MEMTX_DECODE_ERROR;
>      }
> @@ -1508,23 +1552,24 @@ MemTxResult memory_region_dispatch_write(MemoryRegion 
> *mr,
>  
>      if ((!kvm_eventfds_enabled()) &&
>          memory_region_dispatch_write_eventfds(mr, addr, data, size, attrs)) {
> -        return MEMTX_OK;
> -    }
> -
> -    if (mr->ops->write) {
> -        return access_with_adjusted_size(addr, &data, size,
> -                                         mr->ops->impl.min_access_size,
> -                                         mr->ops->impl.max_access_size,
> -                                         memory_region_write_accessor, mr,
> -                                         attrs);
> +        result = MEMTX_OK;
> +    } else if (mr->ops->write) {
> +        result = access_with_adjusted_size(addr, &data, size,
> +                                           mr->ops->impl.min_access_size,
> +                                           mr->ops->impl.max_access_size,
> +                                           memory_region_write_accessor, mr,
> +                                           attrs);
>      } else {
> -        return
> -            access_with_adjusted_size(addr, &data, size,
> -                                      mr->ops->impl.min_access_size,
> -                                      mr->ops->impl.max_access_size,
> -                                      
> memory_region_write_with_attrs_accessor,
> -                                      mr, attrs);
> +        result = access_with_adjusted_size(addr, &data, size,
> +                                           mr->ops->impl.min_access_size,
> +                                           mr->ops->impl.max_access_size,
> +                                           
> memory_region_write_with_attrs_accessor,
> +                                           mr, attrs);
>      }
> +
> +    memory_region_access_end();
> +
> +    return result;
>  }
>  
>  void memory_region_init_io(MemoryRegion *mr,
> -- 
> 2.39.2
>