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

[Chenqun (kuhn)]

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Tuesday, November 10, 2020 11:30 PM
> To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>
> Cc: QEMU Developers <qemu-devel@nongnu.org>; QEMU Trivial
> <qemu-trivial@nongnu.org>; Yoshinori Sato <ysato@users.sourceforge.jp>;
> Zhanghailiang <zhang.zhanghailiang@huawei.com>; ganqixin
> <ganqixin@huawei.com>; Euler Robot <euler.robot@huawei.com>
> Subject: Re: [PATCH] hw/intc: fix heap-buffer-overflow in rxicu_realize()
> 
> 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;
>         }
>     }
> 
It is a good point!  
I tried to modify and compile it, and the test results are exactly the same.
 
Only GCC9 reports a warning：
../hw/intc/rx_icu.c: In function ‘rxicu_realize’:
../hw/intc/rx_icu.c:317:19: warning: comparison is always true due to limited 
range of data type [-Wtype-limits]
  317 |         if (irqno < NR_IRQS) {
     |                   ^

The 'NR_IRQS = 256' ,the ' if (irqno < NR_IRQS)' condition is always true. 
So,maybe we should remove this condition. I'll modify it later.

Thanks,
Chen Qun