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

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

On 2023/03/18 1:25, Cédric Le Goater wrote:
> Hello,
> 
> On 3/16/23 17:20, 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>
>> ---
>> V1 -> V2: Marked the variable thread-local. Introduced linked list.
>>
>>   softmmu/memory.c | 81 ++++++++++++++++++++++++++++++++++++++----------
>>   1 file changed, 64 insertions(+), 17 deletions(-)
>>
>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>> index 4699ba55ec..6be33a9e3e 100644
>> --- a/softmmu/memory.c
>> +++ b/softmmu/memory.c
>> @@ -61,6 +61,15 @@ struct AddrRange {
>>       Int128 size;
>>   };
>> +typedef struct AccessedRegion AccessedRegion;
>> +
>> +struct AccessedRegion {
>> +    const Object *owner;
>> +    const AccessedRegion *next;
>> +};
>> +
>> +static __thread const AccessedRegion *accessed_region;
>> +
>>   static AddrRange addrrange_make(Int128 start, Int128 size)
>>   {
>>       return (AddrRange) { start, size };
>> @@ -1394,6 +1403,16 @@ bool memory_region_access_valid(MemoryRegion *mr,
>>           return false;
>>       }
>> +    for (const AccessedRegion *ar = accessed_region; ar; ar = 
>> ar->next) {
>> +        if (ar->owner == 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 +1432,29 @@ 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,
>> +                                       AccessedRegion *ar)
>> +{
>> +    if (!memory_region_access_valid(mr, addr, size, is_write, attrs)) {
>> +        return false;
>> +    }
>> +
>> +    ar->owner = mr->owner;
>> +    ar->next = accessed_region;
>> +    accessed_region = ar->next;
> 
> Isn't 'accessed_region' always NULL ?
> 
>> +
>> +    return true;
>> +}
>> +
>> +static void memory_region_access_end(void)
>> +{
>> +    accessed_region = accessed_region->next;
>> +}
> and so, this is a segv.
> 
> Thanks,
> 
> C.

It was intended to be: accessed_region = ar;

Anyway, I'm no longer pushing this forward as there is a better alternative:
https://lore.kernel.org/qemu-devel/20230313082417.827484-1-alxndr@bu.edu/

This can detect a re-entrancy problem involving bottom half API, and 
disable the check where re-entrancy is allowed.

Regards,
Akihiko Odaki