[for-3.0,1/2] hw/intc/arm_gic: Check interrupt number in gic_deactivate_irq()

Message ID 20180712154152.32183-2-peter.maydell@linaro.org
State New
Headers show
Series
  • a couple of GICv2 bug fixes
Related show

Commit Message

Peter Maydell July 12, 2018, 3:41 p.m.
In gic_deactivate_irq() the interrupt number comes from the guest
(on a write to the GICC_DIR register), so we need to sanity check
that it isn't out of range before we use it as an array index.
Handle this in a similar manner to the check we do in
gic_complete_irq() for the GICC_EOI register.

The array overrun is not disastrous because the calling code
uses (value & 0x3ff) to extract the interrupt field, so the
only out-of-range values possible are 1020..1023, which allow
overrunning only from irq_state[] into the following
irq_target[] array which the guest can already manipulate.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Richard Henderson July 12, 2018, 5:57 p.m. | #1
On 07/12/2018 10:41 AM, Peter Maydell wrote:
> In gic_deactivate_irq() the interrupt number comes from the guest
> (on a write to the GICC_DIR register), so we need to sanity check
> that it isn't out of range before we use it as an array index.
> Handle this in a similar manner to the check we do in
> gic_complete_irq() for the GICC_EOI register.
> 
> The array overrun is not disastrous because the calling code
> uses (value & 0x3ff) to extract the interrupt field, so the
> only out-of-range values possible are 1020..1023, which allow
> overrunning only from irq_state[] into the following
> irq_target[] array which the guest can already manipulate.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/arm_gic.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Luc Michel July 13, 2018, 1:23 p.m. | #2
On 07/12/2018 05:41 PM, Peter Maydell wrote:
> In gic_deactivate_irq() the interrupt number comes from the guest
> (on a write to the GICC_DIR register), so we need to sanity check
> that it isn't out of range before we use it as an array index.
> Handle this in a similar manner to the check we do in
> gic_complete_irq() for the GICC_EOI register.
> 
> The array overrun is not disastrous because the calling code
> uses (value & 0x3ff) to extract the interrupt field, so the
> only out-of-range values possible are 1020..1023, which allow
> overrunning only from irq_state[] into the following
> irq_target[] array which the guest can already manipulate.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/arm_gic.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)

Reviewed-by: Luc Michel <luc.michel@greensocs.com>

Patch

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index ea0323f9691..b0a69d6386e 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -543,7 +543,21 @@  static bool gic_eoi_split(GICState *s, int cpu, MemTxAttrs attrs)
 static void gic_deactivate_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
 {
     int cm = 1 << cpu;
-    int group = gic_has_groups(s) && GIC_TEST_GROUP(irq, cm);
+    int group;
+
+    if (irq >= s->num_irq) {
+        /*
+         * This handles two cases:
+         * 1. If software writes the ID of a spurious interrupt [ie 1023]
+         * to the GICC_DIR, the GIC ignores that write.
+         * 2. If software writes the number of a non-existent interrupt
+         * this must be a subcase of "value written is not an active interrupt"
+         * and so this is UNPREDICTABLE. We choose to ignore it.
+         */
+        return;
+    }
+
+    group = gic_has_groups(s) && GIC_TEST_GROUP(irq, cm);
 
     if (!gic_eoi_split(s, cpu, attrs)) {
         /* This is UNPREDICTABLE; we choose to ignore it */