[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH] hw/intc: fix heap-buffer-overflow in rxicu_realize()
From: |
Chenqun (kuhn) |
Subject: |
RE: [PATCH] hw/intc: fix heap-buffer-overflow in rxicu_realize() |
Date: |
Wed, 11 Nov 2020 14:03:31 +0000 |
> -----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