* [Qemu-devel] [PATCH for-3.0 0/2] a couple of GICv2 bug fixes
@ 2018-07-12 15:41 Peter Maydell
2018-07-12 15:41 ` [Qemu-devel] [PATCH for-3.0 1/2] hw/intc/arm_gic: Check interrupt number in gic_deactivate_irq() Peter Maydell
2018-07-12 15:41 ` [Qemu-devel] [PATCH for-3.0 2/2] hw/intc/arm_gic: Fix handling of GICD_ITARGETSR Peter Maydell
0 siblings, 2 replies; 7+ messages in thread
From: Peter Maydell @ 2018-07-12 15:41 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: patches, Jan Kiszka, Luc Michel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH for-3.0 1/2] hw/intc/arm_gic: Check interrupt number in gic_deactivate_irq()
2018-07-12 15:41 [Qemu-devel] [PATCH for-3.0 0/2] a couple of GICv2 bug fixes Peter Maydell
@ 2018-07-12 15:41 ` Peter Maydell
2018-07-12 17:57 ` Richard Henderson
2018-07-13 13:23 ` Luc Michel
2018-07-12 15:41 ` [Qemu-devel] [PATCH for-3.0 2/2] hw/intc/arm_gic: Fix handling of GICD_ITARGETSR Peter Maydell
1 sibling, 2 replies; 7+ messages in thread
From: Peter Maydell @ 2018-07-12 15:41 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: patches, Jan Kiszka, Luc Michel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH for-3.0 2/2] hw/intc/arm_gic: Fix handling of GICD_ITARGETSR
2018-07-12 15:41 [Qemu-devel] [PATCH for-3.0 0/2] a couple of GICv2 bug fixes Peter Maydell
2018-07-12 15:41 ` [Qemu-devel] [PATCH for-3.0 1/2] hw/intc/arm_gic: Check interrupt number in gic_deactivate_irq() Peter Maydell
@ 2018-07-12 15:41 ` Peter Maydell
2018-07-12 18:01 ` Richard Henderson
2018-07-13 13:24 ` Luc Michel
1 sibling, 2 replies; 7+ messages in thread
From: Peter Maydell @ 2018-07-12 15:41 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: patches, Jan Kiszka, Luc Michel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.0 1/2] hw/intc/arm_gic: Check interrupt number in gic_deactivate_irq()
2018-07-12 15:41 ` [Qemu-devel] [PATCH for-3.0 1/2] hw/intc/arm_gic: Check interrupt number in gic_deactivate_irq() Peter Maydell
@ 2018-07-12 17:57 ` Richard Henderson
2018-07-13 13:23 ` Luc Michel
1 sibling, 0 replies; 7+ messages in thread
From: Richard Henderson @ 2018-07-12 17:57 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Jan Kiszka, Luc Michel, patches
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~
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.0 2/2] hw/intc/arm_gic: Fix handling of GICD_ITARGETSR
2018-07-12 15:41 ` [Qemu-devel] [PATCH for-3.0 2/2] hw/intc/arm_gic: Fix handling of GICD_ITARGETSR Peter Maydell
@ 2018-07-12 18:01 ` Richard Henderson
2018-07-13 13:24 ` Luc Michel
1 sibling, 0 replies; 7+ messages in thread
From: Richard Henderson @ 2018-07-12 18:01 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Jan Kiszka, Luc Michel, patches
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~
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.0 1/2] hw/intc/arm_gic: Check interrupt number in gic_deactivate_irq()
2018-07-12 15:41 ` [Qemu-devel] [PATCH for-3.0 1/2] hw/intc/arm_gic: Check interrupt number in gic_deactivate_irq() Peter Maydell
2018-07-12 17:57 ` Richard Henderson
@ 2018-07-13 13:23 ` Luc Michel
1 sibling, 0 replies; 7+ messages in thread
From: Luc Michel @ 2018-07-13 13:23 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: patches, Jan Kiszka
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.0 2/2] hw/intc/arm_gic: Fix handling of GICD_ITARGETSR
2018-07-12 15:41 ` [Qemu-devel] [PATCH for-3.0 2/2] hw/intc/arm_gic: Fix handling of GICD_ITARGETSR Peter Maydell
2018-07-12 18:01 ` Richard Henderson
@ 2018-07-13 13:24 ` Luc Michel
1 sibling, 0 replies; 7+ messages in thread
From: Luc Michel @ 2018-07-13 13:24 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: patches, Jan Kiszka
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-07-13 13:24 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-12 15:41 [Qemu-devel] [PATCH for-3.0 0/2] a couple of GICv2 bug fixes Peter Maydell
2018-07-12 15:41 ` [Qemu-devel] [PATCH for-3.0 1/2] hw/intc/arm_gic: Check interrupt number in gic_deactivate_irq() Peter Maydell
2018-07-12 17:57 ` Richard Henderson
2018-07-13 13:23 ` Luc Michel
2018-07-12 15:41 ` [Qemu-devel] [PATCH for-3.0 2/2] hw/intc/arm_gic: Fix handling of GICD_ITARGETSR Peter Maydell
2018-07-12 18:01 ` Richard Henderson
2018-07-13 13:24 ` Luc Michel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.