Re: [PATCH] hw/intc: fix heap-buffer-overflow in rxicu_realize()

[Peter Maydell]

On Thu, 5 Nov 2020 at 07:08, Chen Qun <kuhn.chenqun@huawei.com> wrote:
>
> When 'j = icu->nr_sense – 1', the 'j < icu->nr_sense' condition is true,
> then 'j = icu->nr_sense', the'icu->init_sense[j]' has out-of-bounds access.

Yes, this is a bug...

> Maybe this could lead to some security problems.

...but it's not a security bug, because this device can't
be used with KVM, so it's not on the QEMU security boundary.


>  hw/intc/rx_icu.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/intc/rx_icu.c b/hw/intc/rx_icu.c
> index 94e17a9dea..692a4c78e0 100644
> --- a/hw/intc/rx_icu.c
> +++ b/hw/intc/rx_icu.c
> @@ -308,11 +308,9 @@ static void rxicu_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>      for (i = j = 0; i < NR_IRQS; i++) {
> -        if (icu->init_sense[j] == i) {
> +        if (j < icu->nr_sense && icu->init_sense[j] == i) {
>              icu->src[i].sense = TRG_LEVEL;
> -            if (j < icu->nr_sense) {
> -                j++;
> -            }
> +            j++;
>          } else {
>              icu->src[i].sense = TRG_PEDGE;
>          }

This works, so:

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

but to be honest I think this would be more readable:

    for (i = 0; i < NR_IRQS; i++) {
        ice->src[i].sense = TRG_PEDGE;
    }
    for (i = 0; i < icu->nr_sense; i++) {
        uint8_t irqno = icu->init_sense[i];
        if (irqno < NR_IRQS) {
            icu->src[irqno].sense = TRG_LEVEL;
        }
    }

so we first initialize everything to the default before
processing the init_sense array to identify which irqs
should be level-triggered. (It also means that the caller
doesn't have to ensure the input property is in sorted
order.)

thanks
-- PMM