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
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;
>
+-- On Sun, 31 Jan 2021, Philippe Mathieu-Daudé wrote --+
| 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;
| >
* Looks okay.
| > 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
| > Buglink: https://bugs.launchpad.net/qemu/+bug/1913917
|
| > ---
| > Isnt it worth a CVE to help distributions track backports?
| > ---
* Please send such report(s) to 'qemu-security' list to be triaged as
potential security ones.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Forwarding to qemu-security@ to see if this issue is worth a CVE.
On 1/31/21 12:57 PM, P J P wrote:
> +-- On Sun, 31 Jan 2021, Philippe Mathieu-Daudé wrote --+
> | 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;
> | >
>
> * Looks okay.
>
>
> | > 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
> | > Buglink: https://bugs.launchpad.net/qemu/+bug/1913917
> |
> | > ---
> | > Isnt it worth a CVE to help distributions track backports?
> | > ---
>
> * Please send such report(s) to 'qemu-security' list to be triaged as
> potential security ones.
>
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
>
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
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.
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
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
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
© 2016 - 2026 Red Hat, Inc.