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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2023/03/17 1:15, Alexander Bulekov wrote:
> 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:
> 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

The e1000e bug is certainly covered by your fix. It is nice that it also 
covers the case of DMA from bottom half. I hope it will land soon in the 
next version.

Regards,
Akihiko Odaki

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