Patchwork [13/15] openpic: add some bounds checking for IRQ numbers

login
register
mail settings
Submitter Scott Wood
Date Dec. 22, 2012, 2:15 a.m.
Message ID <1356142552-13453-14-git-send-email-scottwood@freescale.com>
Download mbox | patch
Permalink /patch/207917/
State New
Headers show

Comments

Scott Wood - Dec. 22, 2012, 2:15 a.m.
The two checks with abort() guard against potential QEMU-internal
problems, but the EOI check stops the guest from causing updates to queue
position -1 and other havoc if it writes EOI with no interrupt in
service.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 hw/openpic.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)
Alexander Graf - Jan. 3, 2013, 6:55 p.m.
On 22.12.2012, at 03:15, Scott Wood wrote:

> The two checks with abort() guard against potential QEMU-internal
> problems, but the EOI check stops the guest from causing updates to queue
> position -1 and other havoc if it writes EOI with no interrupt in
> service.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>

Did you ever actually experience this? MAX_IRQ should match the memory region size, so we shouldn't be able to receive any interrupt above it.

I might be inclined to accept an assert() there for internal sanity checking though. The last hunk looks fine.


Alex

> ---
> hw/openpic.c |   15 +++++++++++++++
> 1 file changed, 15 insertions(+)
> 
> diff --git a/hw/openpic.c b/hw/openpic.c
> index 5accff5..a3fcefd 100644
> --- a/hw/openpic.c
> +++ b/hw/openpic.c
> @@ -289,6 +289,10 @@ static void IRQ_check(OpenPICState *opp, IRQ_queue_t *q)
>             irq = base + offset;
>             map &= ~(1UL << offset);
> 
> +            if (irq >= MAX_IRQ) {
> +                abort();
> +            }
> +
>             DPRINTF("IRQ_check: irq %d set ivpr_pr=%d pr=%d\n",
>                     irq, IVPR_PRIORITY(opp->src[irq].ivpr), priority);
> 
> @@ -428,6 +432,11 @@ static void openpic_set_irq(void *opaque, int n_IRQ, int level)
>     OpenPICState *opp = opaque;
>     IRQ_src_t *src;
> 
> +    if (n_IRQ >= MAX_IRQ) {
> +        fprintf(stderr, "%s: IRQ %d out of range\n", __func__, n_IRQ);
> +        abort();
> +    }
> +
>     src = &opp->src[n_IRQ];
>     DPRINTF("openpic: set irq %d = %d ivpr=0x%08x\n",
>             n_IRQ, level, src->ivpr);
> @@ -923,6 +932,12 @@ static void openpic_cpu_write_internal(void *opaque, hwaddr addr,
>     case 0xB0: /* EOI */
>         DPRINTF("EOI\n");
>         s_IRQ = IRQ_get_next(opp, &dst->servicing);
> +
> +        if (s_IRQ < 0) {
> +            DPRINTF("%s: EOI with no interrupt in service\n", __func__);
> +            break;
> +        }
> +
>         IRQ_resetbit(&dst->servicing, s_IRQ);
>         /* Set up next servicing IRQ */
>         s_IRQ = IRQ_get_next(opp, &dst->servicing);
> -- 
> 1.7.9.5
> 
>

Patch

diff --git a/hw/openpic.c b/hw/openpic.c
index 5accff5..a3fcefd 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -289,6 +289,10 @@  static void IRQ_check(OpenPICState *opp, IRQ_queue_t *q)
             irq = base + offset;
             map &= ~(1UL << offset);
 
+            if (irq >= MAX_IRQ) {
+                abort();
+            }
+
             DPRINTF("IRQ_check: irq %d set ivpr_pr=%d pr=%d\n",
                     irq, IVPR_PRIORITY(opp->src[irq].ivpr), priority);
 
@@ -428,6 +432,11 @@  static void openpic_set_irq(void *opaque, int n_IRQ, int level)
     OpenPICState *opp = opaque;
     IRQ_src_t *src;
 
+    if (n_IRQ >= MAX_IRQ) {
+        fprintf(stderr, "%s: IRQ %d out of range\n", __func__, n_IRQ);
+        abort();
+    }
+
     src = &opp->src[n_IRQ];
     DPRINTF("openpic: set irq %d = %d ivpr=0x%08x\n",
             n_IRQ, level, src->ivpr);
@@ -923,6 +932,12 @@  static void openpic_cpu_write_internal(void *opaque, hwaddr addr,
     case 0xB0: /* EOI */
         DPRINTF("EOI\n");
         s_IRQ = IRQ_get_next(opp, &dst->servicing);
+
+        if (s_IRQ < 0) {
+            DPRINTF("%s: EOI with no interrupt in service\n", __func__);
+            break;
+        }
+
         IRQ_resetbit(&dst->servicing, s_IRQ);
         /* Set up next servicing IRQ */
         s_IRQ = IRQ_get_next(opp, &dst->servicing);