Re: [PATCH] hw/intc/arm_gic: Allow to use QTest without crashing

[Philippe Mathieu-Daudé]

On 1/28/21 6:18 PM, Alexander Bulekov wrote:
> On 210128 1714, Philippe Mathieu-Daudé wrote:
>> Alexander reported an issue in gic_get_current_cpu() using the
>> fuzzer. Yet another "deref current_cpu with QTest" bug, reproducible
>> doing:
>>
>>   $ echo readb 0xf03ff000 | qemu-system-arm -M npcm750-evb,accel=qtest 
>> -qtest stdio
>>   [I 1611849440.651452] OPENED
>>   [R +0.242498] readb 0xf03ff000
>>   hw/intc/arm_gic.c:63:29: runtime error: member access within null pointer 
>> of type 'CPUState' (aka 'struct CPUState')
>>   SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior 
>> hw/intc/arm_gic.c:63:29 in
>>   AddressSanitizer:DEADLYSIGNAL
>>   =================================================================
>>   ==3719691==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000082a0 
>> (pc 0x5618790ac882 bp 0x7ffca946f4f0 sp 0x7ffca946f4a0 T0)
>>   ==3719691==The signal is caused by a READ memory access.
>>       #0 0x5618790ac882 in gic_get_current_cpu hw/intc/arm_gic.c:63:29
>>       #1 0x5618790a8901 in gic_dist_readb hw/intc/arm_gic.c:955:11
>>       #2 0x5618790a7489 in gic_dist_read hw/intc/arm_gic.c:1158:17
>>       #3 0x56187adc573b in memory_region_read_with_attrs_accessor 
>> softmmu/memory.c:464:9
>>       #4 0x56187ad7903a in access_with_adjusted_size softmmu/memory.c:552:18
>>       #5 0x56187ad766d6 in memory_region_dispatch_read1 
>> softmmu/memory.c:1426:16
>>       #6 0x56187ad758a8 in memory_region_dispatch_read 
>> softmmu/memory.c:1449:9
>>       #7 0x56187b09e84c in flatview_read_continue softmmu/physmem.c:2822:23
>>       #8 0x56187b0a0115 in flatview_read softmmu/physmem.c:2862:12
>>       #9 0x56187b09fc9e in address_space_read_full softmmu/physmem.c:2875:18
>>       #10 0x56187aa88633 in address_space_read include/exec/memory.h:2489:18
>>       #11 0x56187aa88633 in qtest_process_command softmmu/qtest.c:558:13
>>       #12 0x56187aa81881 in qtest_process_inbuf softmmu/qtest.c:797:9
>>       #13 0x56187aa80e02 in qtest_read softmmu/qtest.c:809:5
>>
>> current_cpu is NULL because QTest accelerator does not use CPU.
>>
>> Fix by skipping the check and returning the first CPU index when
>> QTest accelerator is used, similarly to commit c781a2cc423
>> ("hw/i386/vmport: Allow QTest use without crashing").
>>
>> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> 
> Reviewed-by: Alexander Bulekov <alxndr@bu.edu>
> 
> For reference, some older threads about similar issues in the GDB stub
> and monitor:
> https://bugs.launchpad.net/qemu/+bug/1602247

This one is different. I thought this issue was fixed by
the series around commit 7cf48f6752e ("gdbstub: add multiprocess
support to (f|s)ThreadInfo and ThreadExtraInfo").

When using physical addresses with gdbstub, we should be able to
select a particular address space.

Maybe this fixes pmemsave accessing MMIO here:
https://bugs.launchpad.net/qemu/+bug/1751674

> https://bugs.launchpad.net/qemu/+bug/1878645
> https://patchew.org/QEMU/20200701182100.26930-1-philmd@redhat.com/

I'm still procrastinating this thread :>

> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/intc/arm_gic.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index af41e2fb448..c33b1c8c4bc 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -28,6 +28,7 @@
>>  #include "qemu/module.h"
>>  #include "trace.h"
>>  #include "sysemu/kvm.h"
>> +#include "sysemu/qtest.h"
>>  
>>  /* #define DEBUG_GIC */
>>  
>> @@ -57,7 +58,7 @@ static const uint8_t gic_id_gicv2[] = {
>>  
>>  static inline int gic_get_current_cpu(GICState *s)
>>  {
>> -    if (s->num_cpu > 1) {
>> +    if (!qtest_enabled() && s->num_cpu > 1) {
>>          return current_cpu->cpu_index;
>>      }
>>      return 0;
>> -- 
>> 2.26.2
>>
>>
>