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