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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.