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.
Maybe this could lead to some security problems.

The asan showed stack:
ERROR: AddressSanitizer: heap-buffer-overflow on address 0x604000004d7d at pc 0x55852cd26a76 bp 0x7ffe39f26200 sp 0x7ffe39f261f0
READ of size 1 at 0x604000004d7d thread T0
    #0 0x55852cd26a75 in rxicu_realize ../hw/intc/rx_icu.c:311
    #1 0x55852cf075f7 in device_set_realized ../hw/core/qdev.c:886
    #2 0x55852cd4a32f in property_set_bool ../qom/object.c:2251
    #3 0x55852cd4f9bb in object_property_set ../qom/object.c:1398
    #4 0x55852cd54f3f in object_property_set_qobject ../qom/qom-qobject.c:28
    #5 0x55852cd4fc3f in object_property_set_bool ../qom/object.c:1465
    #6 0x55852cbf0b27 in register_icu ../hw/rx/rx62n.c:156
    #7 0x55852cbf12a6 in rx62n_realize ../hw/rx/rx62n.c:261
    #8 0x55852cf075f7 in device_set_realized ../hw/core/qdev.c:886
    #9 0x55852cd4a32f in property_set_bool ../qom/object.c:2251
    #10 0x55852cd4f9bb in object_property_set ../qom/object.c:1398
    #11 0x55852cd54f3f in object_property_set_qobject ../qom/qom-qobject.c:28
    #12 0x55852cd4fc3f in object_property_set_bool ../qom/object.c:1465
    #13 0x55852cbf1a85 in rx_gdbsim_init ../hw/rx/rx-gdbsim.c:109
    #14 0x55852cd22de0 in qemu_init ../softmmu/vl.c:4380
    #15 0x55852ca57088 in main ../softmmu/main.c:49
    #16 0x7feefafa5d42 in __libc_start_main (/lib64/libc.so.6+0x26d42)

Change the 'j < icu->nr_sense' condition place to fix it.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
---
 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;
         }
-- 
2.27.0

Ping,

Fix: e78597cc457ff7611 
Maybe this bug needs to qemu-5.2 version.

The "icu->nr_sense" is array length.  It's a typical out-of-bounds array bug.


Thanks,
Chen Qun

> -----Original Message-----
> From: Chenqun (kuhn)
> Sent: Thursday, November 5, 2020 3:06 PM
> To: qemu-devel@nongnu.org; qemu-trivial@nongnu.org
> Cc: Zhanghailiang <zhang.zhanghailiang@huawei.com>; ganqixin
> <ganqixin@huawei.com>; Chenqun (kuhn) <kuhn.chenqun@huawei.com>;
> Euler Robot <euler.robot@huawei.com>; Yoshinori Sato
> <ysato@users.sourceforge.jp>
> Subject: [PATCH] hw/intc: fix heap-buffer-overflow in rxicu_realize()
> 
> 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.
> Maybe this could lead to some security problems.
> 
> The asan showed stack:
> ERROR: AddressSanitizer: heap-buffer-overflow on address 0x604000004d7d at
> pc 0x55852cd26a76 bp 0x7ffe39f26200 sp 0x7ffe39f261f0 READ of size 1 at
> 0x604000004d7d thread T0
>     #0 0x55852cd26a75 in rxicu_realize ../hw/intc/rx_icu.c:311
>     #1 0x55852cf075f7 in device_set_realized ../hw/core/qdev.c:886
>     #2 0x55852cd4a32f in property_set_bool ../qom/object.c:2251
>     #3 0x55852cd4f9bb in object_property_set ../qom/object.c:1398
>     #4 0x55852cd54f3f in
> object_property_set_qobject ../qom/qom-qobject.c:28
>     #5 0x55852cd4fc3f in object_property_set_bool ../qom/object.c:1465
>     #6 0x55852cbf0b27 in register_icu ../hw/rx/rx62n.c:156
>     #7 0x55852cbf12a6 in rx62n_realize ../hw/rx/rx62n.c:261
>     #8 0x55852cf075f7 in device_set_realized ../hw/core/qdev.c:886
>     #9 0x55852cd4a32f in property_set_bool ../qom/object.c:2251
>     #10 0x55852cd4f9bb in object_property_set ../qom/object.c:1398
>     #11 0x55852cd54f3f in
> object_property_set_qobject ../qom/qom-qobject.c:28
>     #12 0x55852cd4fc3f in object_property_set_bool ../qom/object.c:1465
>     #13 0x55852cbf1a85 in rx_gdbsim_init ../hw/rx/rx-gdbsim.c:109
>     #14 0x55852cd22de0 in qemu_init ../softmmu/vl.c:4380
>     #15 0x55852ca57088 in main ../softmmu/main.c:49
>     #16 0x7feefafa5d42 in __libc_start_main (/lib64/libc.so.6+0x26d42)
> 
> Change the 'j < icu->nr_sense' condition place to fix it.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
> ---
>  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;
>          }
> --
> 2.27.0

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

> -----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