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

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

[Chen Qun]

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.

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)

Add the 'ice->src[i].sense' initialize to the default value, and then
process init_sense array to identify which irqs should be level-triggered.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>

v1->v2:
Modify the code logic based on Peter's suggestions.
We first initialize everything to the default before processing the init_sense array to identify which irqs should be level-triggered.
---
 hw/intc/rx_icu.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/intc/rx_icu.c b/hw/intc/rx_icu.c
index 94e17a9dea..e5c01807b9 100644
--- a/hw/intc/rx_icu.c
+++ b/hw/intc/rx_icu.c
@@ -300,22 +300,20 @@ static const MemoryRegionOps icu_ops = {
 static void rxicu_realize(DeviceState *dev, Error **errp)
 {
     RXICUState *icu = RX_ICU(dev);
-    int i, j;
+    int i;
 
     if (icu->init_sense == NULL) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "rx_icu: trigger-level property must be set.");
         return;
     }
-    for (i = j = 0; i < NR_IRQS; i++) {
-        if (icu->init_sense[j] == i) {
-            icu->src[i].sense = TRG_LEVEL;
-            if (j < icu->nr_sense) {
-                j++;
-            }
-        } else {
-            icu->src[i].sense = TRG_PEDGE;
-        }
+
+    for (i = 0; i < NR_IRQS; i++) {
+        icu->src[i].sense = TRG_PEDGE;
+    }
+    for (i = 0; i < icu->nr_sense; i++) {
+        uint8_t irqno = icu->init_sense[i];
+        icu->src[irqno].sense = TRG_LEVEL;
     }
     icu->req_irq = -1;
 }
-- 
2.27.0

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

[Chenqun (kuhn)]

Kindly ping！

Maybe it should be need for version 5.2.

> -----Original Message-----
> From: Chenqun (kuhn)
> Sent: Wednesday, November 11, 2020 10:18 PM
> To: qemu-devel@nongnu.org; qemu-trivial@nongnu.org
> Cc: Zhanghailiang <zhang.zhanghailiang@huawei.com>; ganqixin
> <ganqixin@huawei.com>; f4bug@amsat.org; Chenqun (kuhn)
> <kuhn.chenqun@huawei.com>; Peter Maydell <peter.maydell@linaro.org>;
> Euler Robot <euler.robot@huawei.com>; Yoshinori Sato
> <ysato@users.sourceforge.jp>
> Subject: [PATCH-for-5.2 v2] 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.
> 
> 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)
> 
> Add the 'ice->src[i].sense' initialize to the default value, and then process
> init_sense array to identify which irqs should be level-triggered.
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
> 
> v1->v2:
> Modify the code logic based on Peter's suggestions.
> We first initialize everything to the default before processing the init_sense
> array to identify which irqs should be level-triggered.
> ---
>  hw/intc/rx_icu.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/intc/rx_icu.c b/hw/intc/rx_icu.c index 94e17a9dea..e5c01807b9
> 100644
> --- a/hw/intc/rx_icu.c
> +++ b/hw/intc/rx_icu.c
> @@ -300,22 +300,20 @@ static const MemoryRegionOps icu_ops = {  static
> void rxicu_realize(DeviceState *dev, Error **errp)  {
>      RXICUState *icu = RX_ICU(dev);
> -    int i, j;
> +    int i;
> 
>      if (icu->init_sense == NULL) {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "rx_icu: trigger-level property must be set.");
>          return;
>      }
> -    for (i = j = 0; i < NR_IRQS; i++) {
> -        if (icu->init_sense[j] == i) {
> -            icu->src[i].sense = TRG_LEVEL;
> -            if (j < icu->nr_sense) {
> -                j++;
> -            }
> -        } else {
> -            icu->src[i].sense = TRG_PEDGE;
> -        }
> +
> +    for (i = 0; i < NR_IRQS; i++) {
> +        icu->src[i].sense = TRG_PEDGE;
> +    }
> +    for (i = 0; i < icu->nr_sense; i++) {
> +        uint8_t irqno = icu->init_sense[i];
> +        icu->src[irqno].sense = TRG_LEVEL;
>      }
>      icu->req_irq = -1;
>  }
> --
> 2.27.0

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

[Peter Maydell]

On Wed, 11 Nov 2020 at 14:18, 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.
>
> 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)
>
> Add the 'ice->src[i].sense' initialize to the default value, and then
> process init_sense array to identify which irqs should be level-triggered.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>


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

thanks
-- PMM

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

[Peter Maydell]

On Fri, 20 Nov 2020 at 13:44, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 11 Nov 2020 at 14:18, 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.

> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Reported-by: Euler Robot <euler.robot@huawei.com>
> > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I'll take this via target-arm.next, I guess.

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

Hi Peter,

On 11/20/20 5:41 PM, Peter Maydell wrote:
> On Fri, 20 Nov 2020 at 13:44, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Wed, 11 Nov 2020 at 14:18, 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.
> 
>>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>>
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> I'll take this via target-arm.next, I guess.

Sorry I missed this patch while preparing the latest Renesas
pull request. I filter for hw/rx/ and didn't notice this
(also I was not Cc'ed in v1).

BTW to make things clear, I'm not maintaining this subsystem,
I simply try to cover for Yoshinori who is not very active.

As there are no other patch, I appreciate you taking this via
your ARM queue.

Thanks,

Phil.