[PATCH] hw/intc/arm_gic: Fix interrupt ID in GICD_SGIR register

[PATCH] hw/intc/arm_gic: Fix interrupt ID in GICD_SGIR register

[Philippe Mathieu-Daudé]

Per the ARM Generic Interrupt Controller Architecture specification
(document "ARM IHI 0048B.b (ID072613)"), the SGIINTID field is 4 bit,
not 10:

  - 4.3 Distributor register descriptions
  - 4.3.15 Software Generated Interrupt Register, GICD_SG

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

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.")
Buglink: https://bugs.launchpad.net/qemu/+bug/1913916
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Isnt it worth a CVE to help distributions track backports?
---
 hw/intc/arm_gic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

[Philippe Mathieu-Daudé]

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:
> 
>   - 4.3 Distributor register descriptions
>   - 4.3.15 Software Generated Interrupt Register, GICD_SG
> 
>     - 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.
> 
> 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.")
> Buglink: https://bugs.launchpad.net/qemu/+bug/1913916

Also:
Buglink: https://bugs.launchpad.net/qemu/+bug/1913917

> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Isnt it worth a CVE to help distributions track backports?
> ---
>  hw/intc/arm_gic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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;
> 

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

[P J P]

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;


> | > 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 you.
---
  -P J P
http://feedmug.com

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.

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

[Peter Maydell]

On Tue, 2 Feb 2021 at 09:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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:
> >> | > 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.

Philippe is correct here. In both cases the overrun is on the
first writel to 0x8000f00, but the fuzzer has for some reason not
reported that but instead blundered on until it happens to trigger
some other issue that resulted from the memory corruption it induced
with the first write.

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

On the CVE:

Since this can affect systems using KVM, this is a security bug for
us. However, it only affects an uncommon configuration:
you are only vulnerable if you are using "kernel-irqchip=off"
(the default is 'on', and turning it off is an odd thing to do).

I've applied this patch to target-arm.next.

thanks
-- PMM

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

[Alexander Bulekov]

On 210202 1221, Peter Maydell wrote:
> On Tue, 2 Feb 2021 at 09:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > 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:
> > >> | > 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.
> 
> Philippe is correct here. In both cases the overrun is on the
> first writel to 0x8000f00, but the fuzzer has for some reason not
> reported that but instead blundered on until it happens to trigger
> some other issue that resulted from the memory corruption it induced
> with the first write.
> 

It happens in the standalone reproducer build with
ASAN/--enable-sanitizers, so it seems like an oversight/bug in the
Sanitizers, rather than the fuzzer. It is strange that UBSan seemed to
detect an OOB index, but proceeded as normal. I should start paying
closer attention to those non-fatal UBSan errors.

> > >> | > 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.
> 
> On the CVE:
> 
> Since this can affect systems using KVM, this is a security bug for
> us. However, it only affects an uncommon configuration:
> you are only vulnerable if you are using "kernel-irqchip=off"
> (the default is 'on', and turning it off is an odd thing to do).
> 
> I've applied this patch to target-arm.next.
>

Ah I had a gut feeling there was a GIC in kvm. CVE or no CVE, --n_bugs
-Alex

> thanks
> -- PMM

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

[P J P]

On Tuesday, 2 February, 2021, 08:45:19 pm IST, Peter Maydell <peter.maydell@linaro.org> wrote: 
>On the CVE:
>
>Since this can affect systems using KVM, this is a security bug for
>us. However, it only affects an uncommon configuration:
>you are only vulnerable if you are using "kernel-irqchip=off"
>(the default is 'on', and turning it off is an odd thing to do).
>

'CVE-2021-20221' assigned by Red Hat Inc.
  -> https://bugs.launchpad.net/qemu/+bug/1914353/comments/3

Thank you.
---
  -P J P
http://feedmug.com