diff mbox

hw/arm_gic.c: Ignore attempts to complete nonexistent IRQs

Message ID 1322764637-15818-1-git-send-email-peter.maydell@linaro.org
State Accepted
Commit 217bfb445b54db618a30f3a39170bebd9fd9dbf2
Headers show

Commit Message

Peter Maydell Dec. 1, 2011, 6:37 p.m. UTC 
Ignore attempts to complete non-existent IRQs; this fixes a buffer
overrun if the guest writes a bad value to the GICC_EOIR register.
(This case is UNPREDICTABLE so ignoring it is a valid choice.)
Note that doing nothing if the guest writes 1023 to this register
is not in fact a change in behaviour: the old code would also
always do nothing in this case but in a non-obvious way.
(The buffer overrun was noted by Coverity, see bug 887883.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
It looks to me as if Coverity is being alarmist about the other alleged
static overruns in this file...

 hw/arm_gic.c |   27 ++++++++++++++++++---------
 1 files changed, 18 insertions(+), 9 deletions(-)

Comments

andrzej zaborowski Dec. 5, 2011, 8:25 p.m. UTC | #1 
On 1 December 2011 19:37, Peter Maydell <peter.maydell@linaro.org> wrote:
> Ignore attempts to complete non-existent IRQs; this fixes a buffer
> overrun if the guest writes a bad value to the GICC_EOIR register.
> (This case is UNPREDICTABLE so ignoring it is a valid choice.)
> Note that doing nothing if the guest writes 1023 to this register
> is not in fact a change in behaviour: the old code would also
> always do nothing in this case but in a non-obvious way.
> (The buffer overrun was noted by Coverity, see bug 887883.)

Thanks, applied this patch also.

Cheers
diff mbox

Patch

diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index f3f3516..527c9ce 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -215,17 +215,26 @@  static void gic_complete_irq(gic_state * s, int cpu, int irq)
     int update = 0;
     int cm = 1 << cpu;
     DPRINTF("EOI %d\n", irq);
+    if (irq >= GIC_NIRQ) {
+        /* This handles two cases:
+         * 1. If software writes the ID of a spurious interrupt [ie 1023]
+         * to the GICC_EOIR, the GIC ignores that write.
+         * 2. If software writes the number of a non-existent interrupt
+         * this must be a subcase of "value written does not match the last
+         * valid interrupt value read from the Interrupt Acknowledge
+         * register" and so this is UNPREDICTABLE. We choose to ignore it.
+         */
+        return;
+    }
     if (s->running_irq[cpu] == 1023)
         return; /* No active IRQ.  */
-    if (irq != 1023) {
-        /* Mark level triggered interrupts as pending if they are still
-           raised.  */
-        if (!GIC_TEST_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)
-                && GIC_TEST_LEVEL(irq, cm) && (GIC_TARGET(irq) & cm) != 0) {
-            DPRINTF("Set %d pending mask %x\n", irq, cm);
-            GIC_SET_PENDING(irq, cm);
-            update = 1;
-        }
+    /* Mark level triggered interrupts as pending if they are still
+       raised.  */
+    if (!GIC_TEST_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)
+        && GIC_TEST_LEVEL(irq, cm) && (GIC_TARGET(irq) & cm) != 0) {
+        DPRINTF("Set %d pending mask %x\n", irq, cm);
+        GIC_SET_PENDING(irq, cm);
+        update = 1;
     }
     if (irq != s->running_irq[cpu]) {
         /* Complete an IRQ that is not currently running.  */