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

[Richard Henderson <richard.henderson@linaro.org>]

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: