> -----Original Message----- > From: Peter Maydell [mailto:peter.mayd...@linaro.org] > Sent: Tuesday, November 10, 2020 11:30 PM > To: Chenqun (kuhn) <kuhn.chen...@huawei.com> > Cc: QEMU Developers <qemu-devel@nongnu.org>; QEMU Trivial > <qemu-triv...@nongnu.org>; Yoshinori Sato <ys...@users.sourceforge.jp>; > Zhanghailiang <zhang.zhanghaili...@huawei.com>; ganqixin > <ganqi...@huawei.com>; Euler Robot <euler.ro...@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.chen...@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.mayd...@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
