[PATCH] hw/intc/openpic: Avoid taking address of out-of-bounds array index

[PATCH] hw/intc/openpic: Avoid taking address of out-of-bounds array index

[Peter Maydell]

The clang sanitizer complains about the code in the EOI handling
of openpic_cpu_write_internal():

UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1 ./build/clang/qemu-system-ppc -M mac99,graphics=off -display none -kernel day15/invaders.elf
../../hw/intc/openpic.c:1034:16: runtime error: index -1 out of bounds for type 'IRQSource[264]' (aka 'struct IRQSource[264]')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../hw/intc/openpic.c:1034:16 in

This is because we do
  src = &opp->src[n_IRQ];$
when n_IRQ may be -1.  This is in practice harmless because if n_IRQ
is -1 then we don't do anything with the src pointer, but it is
undefined behaviour. (This has been present since this device
was first added to QEMU.)

Rearrange the code so we only do the array index when n_IRQ is not -1.

Cc: qemu-stable@nongnu.org
Fixes: e9df014c0b ("Implement embedded IRQ controller for PowerPC 6xx/740 & 75")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Arguable whether it's worth the stable backport or not...
---
 hw/intc/openpic.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index cd3d87768e0..2ead4b9ba00 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -1031,13 +1031,14 @@ static void openpic_cpu_write_internal(void *opaque, hwaddr addr,
         s_IRQ = IRQ_get_next(opp, &dst->servicing);
         /* Check queued interrupts. */
         n_IRQ = IRQ_get_next(opp, &dst->raised);
-        src = &opp->src[n_IRQ];
-        if (n_IRQ != -1 &&
-            (s_IRQ == -1 ||
-             IVPR_PRIORITY(src->ivpr) > dst->servicing.priority)) {
-            DPRINTF("Raise OpenPIC INT output cpu %d irq %d",
-                    idx, n_IRQ);
-            qemu_irq_raise(opp->dst[idx].irqs[OPENPIC_OUTPUT_INT]);
+        if (n_IRQ != -1) {
+            src = &opp->src[n_IRQ];
+            if (s_IRQ == -1 ||
+                IVPR_PRIORITY(src->ivpr) > dst->servicing.priority) {
+                DPRINTF("Raise OpenPIC INT output cpu %d irq %d",
+                        idx, n_IRQ);
+                qemu_irq_raise(opp->dst[idx].irqs[OPENPIC_OUTPUT_INT]);
+            }
         }
         break;
     default:
-- 
2.34.1

Re: [PATCH] hw/intc/openpic: Avoid taking address of out-of-bounds array index

[Richard Henderson]

On 11/5/24 18:02, Peter Maydell wrote:
> The clang sanitizer complains about the code in the EOI handling
> of openpic_cpu_write_internal():
> 
> UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1 ./build/clang/qemu-system-ppc -M mac99,graphics=off -display none -kernel day15/invaders.elf
> ../../hw/intc/openpic.c:1034:16: runtime error: index -1 out of bounds for type 'IRQSource[264]' (aka 'struct IRQSource[264]')
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../hw/intc/openpic.c:1034:16 in
> 
> This is because we do
>    src = &opp->src[n_IRQ];$
> when n_IRQ may be -1.  This is in practice harmless because if n_IRQ
> is -1 then we don't do anything with the src pointer, but it is
> undefined behaviour. (This has been present since this device
> was first added to QEMU.)
> 
> Rearrange the code so we only do the array index when n_IRQ is not -1.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: e9df014c0b ("Implement embedded IRQ controller for PowerPC 6xx/740 & 75")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Arguable whether it's worth the stable backport or not...
> ---
>   hw/intc/openpic.c | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index cd3d87768e0..2ead4b9ba00 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -1031,13 +1031,14 @@ static void openpic_cpu_write_internal(void *opaque, hwaddr addr,
>           s_IRQ = IRQ_get_next(opp, &dst->servicing);
>           /* Check queued interrupts. */
>           n_IRQ = IRQ_get_next(opp, &dst->raised);
> -        src = &opp->src[n_IRQ];
> -        if (n_IRQ != -1 &&
> -            (s_IRQ == -1 ||
> -             IVPR_PRIORITY(src->ivpr) > dst->servicing.priority)) {
> -            DPRINTF("Raise OpenPIC INT output cpu %d irq %d",
> -                    idx, n_IRQ);
> -            qemu_irq_raise(opp->dst[idx].irqs[OPENPIC_OUTPUT_INT]);
> +        if (n_IRQ != -1) {
> +            src = &opp->src[n_IRQ];

Could move the variable declaration here.  Anyway,

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

r~

> +            if (s_IRQ == -1 ||
> +                IVPR_PRIORITY(src->ivpr) > dst->servicing.priority) {
> +                DPRINTF("Raise OpenPIC INT output cpu %d irq %d",
> +                        idx, n_IRQ);
> +                qemu_irq_raise(opp->dst[idx].irqs[OPENPIC_OUTPUT_INT]);
> +            }
>           }
>           break;
>       default:

Re: [PATCH] hw/intc/openpic: Avoid taking address of out-of-bounds array index

[Mark Cave-Ayland]

On 05/11/2024 18:02, Peter Maydell wrote:

> The clang sanitizer complains about the code in the EOI handling
> of openpic_cpu_write_internal():
> 
> UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1 ./build/clang/qemu-system-ppc -M mac99,graphics=off -display none -kernel day15/invaders.elf
> ../../hw/intc/openpic.c:1034:16: runtime error: index -1 out of bounds for type 'IRQSource[264]' (aka 'struct IRQSource[264]')
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../hw/intc/openpic.c:1034:16 in
> 
> This is because we do
>    src = &opp->src[n_IRQ];$

Extra $ symbol at the end of the line here?

> when n_IRQ may be -1.  This is in practice harmless because if n_IRQ
> is -1 then we don't do anything with the src pointer, but it is
> undefined behaviour. (This has been present since this device
> was first added to QEMU.)
> 
> Rearrange the code so we only do the array index when n_IRQ is not -1.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: e9df014c0b ("Implement embedded IRQ controller for PowerPC 6xx/740 & 75")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Arguable whether it's worth the stable backport or not...
> ---
>   hw/intc/openpic.c | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index cd3d87768e0..2ead4b9ba00 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -1031,13 +1031,14 @@ static void openpic_cpu_write_internal(void *opaque, hwaddr addr,
>           s_IRQ = IRQ_get_next(opp, &dst->servicing);
>           /* Check queued interrupts. */
>           n_IRQ = IRQ_get_next(opp, &dst->raised);
> -        src = &opp->src[n_IRQ];
> -        if (n_IRQ != -1 &&
> -            (s_IRQ == -1 ||
> -             IVPR_PRIORITY(src->ivpr) > dst->servicing.priority)) {
> -            DPRINTF("Raise OpenPIC INT output cpu %d irq %d",
> -                    idx, n_IRQ);
> -            qemu_irq_raise(opp->dst[idx].irqs[OPENPIC_OUTPUT_INT]);
> +        if (n_IRQ != -1) {
> +            src = &opp->src[n_IRQ];
> +            if (s_IRQ == -1 ||
> +                IVPR_PRIORITY(src->ivpr) > dst->servicing.priority) {
> +                DPRINTF("Raise OpenPIC INT output cpu %d irq %d",
> +                        idx, n_IRQ);
> +                qemu_irq_raise(opp->dst[idx].irqs[OPENPIC_OUTPUT_INT]);
> +            }
>           }
>           break;
>       default:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH] hw/intc/openpic: Avoid taking address of out-of-bounds array index

[Peter Maydell]

On Wed, 6 Nov 2024 at 11:58, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 05/11/2024 18:02, Peter Maydell wrote:
>
> > The clang sanitizer complains about the code in the EOI handling
> > of openpic_cpu_write_internal():
> >
> > UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1 ./build/clang/qemu-system-ppc -M mac99,graphics=off -display none -kernel day15/invaders.elf
> > ../../hw/intc/openpic.c:1034:16: runtime error: index -1 out of bounds for type 'IRQSource[264]' (aka 'struct IRQSource[264]')
> > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../hw/intc/openpic.c:1034:16 in
> >
> > This is because we do
> >    src = &opp->src[n_IRQ];$
>
> Extra $ symbol at the end of the line here?

Yep (cut-n-paste from an editor that marks end-of-lines).

> > when n_IRQ may be -1.  This is in practice harmless because if n_IRQ
> > is -1 then we don't do anything with the src pointer, but it is
> > undefined behaviour. (This has been present since this device
> > was first added to QEMU.)
> >
> > Rearrange the code so we only do the array index when n_IRQ is not -1.
> >
> > Cc: qemu-stable@nongnu.org
> > Fixes: e9df014c0b ("Implement embedded IRQ controller for PowerPC 6xx/740 & 75")
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > Arguable whether it's worth the stable backport or not...
> > ---
> >   hw/intc/openpic.c | 15 ++++++++-------
> >   1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> > index cd3d87768e0..2ead4b9ba00 100644
> > --- a/hw/intc/openpic.c
> > +++ b/hw/intc/openpic.c
> > @@ -1031,13 +1031,14 @@ static void openpic_cpu_write_internal(void *opaque, hwaddr addr,
> >           s_IRQ = IRQ_get_next(opp, &dst->servicing);
> >           /* Check queued interrupts. */
> >           n_IRQ = IRQ_get_next(opp, &dst->raised);
> > -        src = &opp->src[n_IRQ];
> > -        if (n_IRQ != -1 &&
> > -            (s_IRQ == -1 ||
> > -             IVPR_PRIORITY(src->ivpr) > dst->servicing.priority)) {
> > -            DPRINTF("Raise OpenPIC INT output cpu %d irq %d",
> > -                    idx, n_IRQ);
> > -            qemu_irq_raise(opp->dst[idx].irqs[OPENPIC_OUTPUT_INT]);
> > +        if (n_IRQ != -1) {
> > +            src = &opp->src[n_IRQ];
> > +            if (s_IRQ == -1 ||
> > +                IVPR_PRIORITY(src->ivpr) > dst->servicing.priority) {
> > +                DPRINTF("Raise OpenPIC INT output cpu %d irq %d",
> > +                        idx, n_IRQ);
> > +                qemu_irq_raise(opp->dst[idx].irqs[OPENPIC_OUTPUT_INT]);
> > +            }
> >           }
> >           break;
> >       default:
>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Thanks. I can take this via target-arm.next, or does anybody
have a different preference?

-- PMM

Re: [PATCH] hw/intc/openpic: Avoid taking address of out-of-bounds array index

[Philippe Mathieu-Daudé]

On 14/11/24 13:22, Peter Maydell wrote:
> On Wed, 6 Nov 2024 at 11:58, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> On 05/11/2024 18:02, Peter Maydell wrote:
>>
>>> The clang sanitizer complains about the code in the EOI handling
>>> of openpic_cpu_write_internal():
>>>
>>> UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1 ./build/clang/qemu-system-ppc -M mac99,graphics=off -display none -kernel day15/invaders.elf
>>> ../../hw/intc/openpic.c:1034:16: runtime error: index -1 out of bounds for type 'IRQSource[264]' (aka 'struct IRQSource[264]')
>>> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../hw/intc/openpic.c:1034:16 in
>>>
>>> This is because we do
>>>     src = &opp->src[n_IRQ];$
>>
>> Extra $ symbol at the end of the line here?
> 
> Yep (cut-n-paste from an editor that marks end-of-lines).
> 
>>> when n_IRQ may be -1.  This is in practice harmless because if n_IRQ
>>> is -1 then we don't do anything with the src pointer, but it is
>>> undefined behaviour. (This has been present since this device
>>> was first added to QEMU.)
>>>
>>> Rearrange the code so we only do the array index when n_IRQ is not -1.
>>>
>>> Cc: qemu-stable@nongnu.org
>>> Fixes: e9df014c0b ("Implement embedded IRQ controller for PowerPC 6xx/740 & 75")
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>> Arguable whether it's worth the stable backport or not...
>>> ---
>>>    hw/intc/openpic.c | 15 ++++++++-------
>>>    1 file changed, 8 insertions(+), 7 deletions(-)


>> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Thanks. I can take this via target-arm.next, or does anybody
> have a different preference?

I had it tagged for my next hw-misc PR but was busy focused on
other things so haven't taken the time for it yet. Better you
take it, thanks!

Phil.