| Message ID | 20201111141733.2358800-1-kuhn.chenqun@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH-for-5.2,v2] hw/intc: fix heap-buffer-overflow in rxicu_realize() | expand |
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
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
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
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.
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; }
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(-)