[Qemu-devel] [PATCH for-3.0 0/2] a couple of GICv2 bug fixes

[Qemu-devel] [PATCH for-3.0 0/2] a couple of GICv2 bug fixes

[Peter Maydell]

These patches fix bugs in our GICv2 implementation that we noticed
in the course of reviewing Luc's patchset to add virtualization
support to it.

Patch 1 is a missing bounds check, effectively. Luckily there
was a mask operation in place that means it's not actually
possible to do anything nasty as a result.

Patch 2 fixes GICD_ITARGETSR for non-11MPCore GICs, which is
necessary to run Jailhouse as a guest.

thanks
-- PMM

Peter Maydell (2):
  hw/intc/arm_gic: Check interrupt number in gic_deactivate_irq()
  hw/intc/arm_gic: Fix handling of GICD_ITARGETSR

 hw/intc/arm_gic.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

-- 
2.17.1

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

[Peter Maydell]

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(-)

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 */
-- 
2.17.1

Re: [Qemu-devel] [PATCH for-3.0 1/2] hw/intc/arm_gic: Check interrupt number in gic_deactivate_irq()

[Richard Henderson]

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~

Re: [Qemu-devel] [PATCH for-3.0 1/2] hw/intc/arm_gic: Check interrupt number in gic_deactivate_irq()

[Luc Michel]

[-- Attachment #1: Type: text/plain, Size: 904 bytes --]

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>

-- 
Luc


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH for-3.0 2/2] hw/intc/arm_gic: Fix handling of GICD_ITARGETSR

[Peter Maydell]

The GICD_ITARGETSR implementation still has some 11MPCore behaviour
that we were incorrectly using in our GICv1 and GICv2 implementations
for the case where the interrupt number is less than GIC_INTERNAL.
The desired behaviour here is:
 * for 11MPCore: RAZ/WI for irqs 0..28; read a number matching the
   CPU doing the read for irqs 29..31
 * for GICv1 and v2: RAZ/WI if uniprocessor; otherwise read a
   number matching the CPU doing the read for all irqs < 32

Stop squashing GICD_ITARGETSR to 0 for IRQs 0..28 unless this
is an 11MPCore GIC.

Reported-by: Jan Kiszka <jan.kiszka@web.de>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index b0a69d6386e..34dc84ae813 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -751,7 +751,9 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
             if (irq >= s->num_irq) {
                 goto bad_reg;
             }
-            if (irq >= 29 && irq <= 31) {
+            if (irq < 29 && s->revision == REV_11MPCORE) {
+                res = 0;
+            } else if (irq < GIC_INTERNAL) {
                 res = cm;
             } else {
                 res = GIC_TARGET(irq);
@@ -1014,7 +1016,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
             if (irq >= s->num_irq) {
                 goto bad_reg;
             }
-            if (irq < 29) {
+            if (irq < 29 && s->revision == REV_11MPCORE) {
                 value = 0;
             } else if (irq < GIC_INTERNAL) {
                 value = ALL_CPU_MASK;
-- 
2.17.1

Re: [Qemu-devel] [PATCH for-3.0 2/2] hw/intc/arm_gic: Fix handling of GICD_ITARGETSR

[Richard Henderson]

On 07/12/2018 10:41 AM, Peter Maydell wrote:
> The GICD_ITARGETSR implementation still has some 11MPCore behaviour
> that we were incorrectly using in our GICv1 and GICv2 implementations
> for the case where the interrupt number is less than GIC_INTERNAL.
> The desired behaviour here is:
>  * for 11MPCore: RAZ/WI for irqs 0..28; read a number matching the
>    CPU doing the read for irqs 29..31
>  * for GICv1 and v2: RAZ/WI if uniprocessor; otherwise read a
>    number matching the CPU doing the read for all irqs < 32
> 
> Stop squashing GICD_ITARGETSR to 0 for IRQs 0..28 unless this
> is an 11MPCore GIC.
> 
> Reported-by: Jan Kiszka <jan.kiszka@web.de>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/arm_gic.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

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


r~

Re: [Qemu-devel] [PATCH for-3.0 2/2] hw/intc/arm_gic: Fix handling of GICD_ITARGETSR

[Luc Michel]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]



On 07/12/2018 05:41 PM, Peter Maydell wrote:
> The GICD_ITARGETSR implementation still has some 11MPCore behaviour
> that we were incorrectly using in our GICv1 and GICv2 implementations
> for the case where the interrupt number is less than GIC_INTERNAL.
> The desired behaviour here is:
>  * for 11MPCore: RAZ/WI for irqs 0..28; read a number matching the
>    CPU doing the read for irqs 29..31
>  * for GICv1 and v2: RAZ/WI if uniprocessor; otherwise read a
>    number matching the CPU doing the read for all irqs < 32
> 
> Stop squashing GICD_ITARGETSR to 0 for IRQs 0..28 unless this
> is an 11MPCore GIC.
> 
> Reported-by: Jan Kiszka <jan.kiszka@web.de>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/arm_gic.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

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

-- 
Luc



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]