Re: [QEMU-SECURITY] [PATCH] hw/intc/arm_gic: Fix interrupt ID in GICD_SGIR register

[Philippe Mathieu-Daudé]

On 2/2/21 7:21 AM, P J P wrote:
> On Sunday, 31 January, 2021, 08:48:26 pm IST, Philippe Mathieu-Daudé 
> <f4bug@amsat.org> wrote: 
>> Forwarding to qemu-security@ to see if this issue is worth a CVE.
>>
>> | On 1/31/21 11:34 AM, Philippe Mathieu-Daudé wrote:
>> | > Per the ARM Generic Interrupt Controller Architecture specification
>> | > (document "ARM IHI 0048B.b (ID072613)"), the SGIINTID field is 4 bit,
>> | > not 10:
>> | > 
>> | >    - Table 4-21 GICD_SGIR bit assignments
>> | > 
>> | >    The Interrupt ID of the SGI to forward to the specified CPU
>> | >    interfaces. The value of this field is the Interrupt ID, in
>> | >    the range 0-15, for example a value of 0b0011 specifies
>> | >    Interrupt ID 3.
>> | > 
>> | > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> | > index af41e2fb448..75316329516 100644
>> | > --- a/hw/intc/arm_gic.c
>> | > +++ b/hw/intc/arm_gic.c
>> | > @@ -1476,7 +1476,7 @@ static void gic_dist_writel(void *opaque, hwaddr 
>> offset,
>> | >          int target_cpu;
>> | >  
>> | >          cpu = gic_get_current_cpu(s);
>> | > -        irq = value & 0x3ff;
>> | > +        irq = value & 0xf;
>> | >          switch ((value >> 24) & 3) {
>> | >          case 0:
>> | >              mask = (value >> 16) & ALL_CPU_MASK;
>> | > 
>> | > Buglink: https://bugs.launchpad.net/qemu/+bug/1913916
>> | > Buglink: https://bugs.launchpad.net/qemu/+bug/1913917
> 
> * Does above patch address both these bugs? For BZ#1913917 'irq' is derived 
> from 'offset' it seems.
> 
>         /* Interrupt Configuration.  */                                       
>   
>         irq = (offset - 0xc00) * 4;

I haven't done a thorough analysis, simply tried to fixed this
bug ASAP as it is public so many users are exposed.

I had the impression the first call (writel 0x8000f00 0x5affaf)
break the heap, so the memory is inconsistent when the second
call (write 0x8000eff 0x1 0x0) is done, but better have developers
familiar with GIC and security auditing this again.

>> | > Correct the irq mask to fix an undefined behavior (which eventually
>> | > lead to a heap-buffer-overflow, see [Buglink]):
>> | > 
>> | >    $ echo 'writel 0x8000f00 0xff4affb0' | qemu-system-aarch64 -M 
>> virt,accel=qtest -qtest stdio
>> | >    [I 1612088147.116987] OPENED
>> | >  [R +0.278293] writel 0x8000f00 0xff4affb0
>> | >  ../hw/intc/arm_gic.c:1498:13: runtime error: index 944 out of bounds 
>> for type 'uint8_t [16][8]'
>> | >  SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior 
>> ../hw/intc/arm_gic.c:1498:13
>> | > 
>> | > Cc: qemu-stable@nongnu.org
>> | > Fixes: 9ee6e8bb853 ("ARMv7 support.")
>> |
>> | > ---
>> | > Isnt it worth a CVE to help distributions track backports?
>> | > ---
> 
> Thank you for reporting this issue. Will process further.

Thank for the quick processing.

Regards,

Phil.