| Message ID | 20170905112152.8851-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 5 September 2017 at 12:21, P J P <ppandit@redhat.com> wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > When reading or writing to GICR ipriority array, index 'irq' > could go beyond its bounds; Restrict it within array limits. > > Reported-by: Guoxiang Niu <niuguoxiang@huawei.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/intc/arm_gicv3_redist.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c > index 77e5cfa327..7683c4cc7f 100644 > --- a/hw/intc/arm_gicv3_redist.c > +++ b/hw/intc/arm_gicv3_redist.c > @@ -187,7 +187,7 @@ static MemTxResult gicr_readl(GICv3CPUState *cs, hwaddr offset, > case GICR_ICACTIVER0: > *data = gicr_read_bitmap_reg(cs, attrs, cs->gicr_iactiver0); > return MEMTX_OK; > - case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1f: > + case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1c: > { > int i, irq = offset - GICR_IPRIORITYR; > uint32_t value = 0; > @@ -310,7 +310,7 @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset, > case GICR_ICACTIVER0: > gicr_write_clear_bitmap_reg(cs, attrs, &cs->gicr_iactiver0, value); > return MEMTX_OK; > - case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1f: > + case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1c: > { > int i, irq = offset - GICR_IPRIORITYR; Why do you think the buffer can be overrun? These functions are the word (4 byte) access functions, and they cannot be called with a non-4-aligned offset (see the asserts in gicv3_redist_read() and gicv3_redist_write()). thanks -- PMM
On 09/05/2017 08:58 AM, Peter Maydell wrote: > On 5 September 2017 at 12:21, P J P <ppandit@redhat.com> wrote: >> From: Prasad J Pandit <pjp@fedoraproject.org> >> >> When reading or writing to GICR ipriority array, index 'irq' >> could go beyond its bounds; Restrict it within array limits. >> >> Reported-by: Guoxiang Niu <niuguoxiang@huawei.com> >> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> >> --- >> hw/intc/arm_gicv3_redist.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c >> index 77e5cfa327..7683c4cc7f 100644 >> --- a/hw/intc/arm_gicv3_redist.c >> +++ b/hw/intc/arm_gicv3_redist.c >> @@ -187,7 +187,7 @@ static MemTxResult gicr_readl(GICv3CPUState *cs, hwaddr offset, >> case GICR_ICACTIVER0: >> *data = gicr_read_bitmap_reg(cs, attrs, cs->gicr_iactiver0); >> return MEMTX_OK; >> - case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1f: 0x1f are only the cpu (private) irqs, then the range is valid up-to: (extract64(cs->gicr_typer, 0, 5) + 1) * 32 - 1 supported irqs >> + case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1c: >> { >> int i, irq = offset - GICR_IPRIORITYR; >> uint32_t value = 0; >> @@ -310,7 +310,7 @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset, >> case GICR_ICACTIVER0: >> gicr_write_clear_bitmap_reg(cs, attrs, &cs->gicr_iactiver0, value); >> return MEMTX_OK; >> - case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1f: >> + case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1c: >> { >> int i, irq = offset - GICR_IPRIORITYR; > > Why do you think the buffer can be overrun? These functions > are the word (4 byte) access functions, and they cannot > be called with a non-4-aligned offset (see the asserts in > gicv3_redist_read() and gicv3_redist_write()). > > thanks > -- PMM >
I think only assert is not enough, because assert() depends on NDEBUG preprocessing, please check : /usr/include/assert.h 37#if defined __cplusplus && __GNUC_PREREQ (2,95) 38# define __ASSERT_VOID_CAST static_cast<void> 39#else 40# define __ASSERT_VOID_CAST (void) 41#endif 48#ifdef NDEBUG 49 50# define assert(expr) (__ASSERT_VOID_CAST (0)) 62#else /* Not NDEBUG. */ 68/* This prints an "Assertion failed" message and aborts. */ 69extern void __assert_fail (const char *__assertion, const char *__file, 70 unsigned int __line, const char *__function) 71 __THROW __attribute__ ((__noreturn__)); 86#endif /* Not _ASSERT_H_DECLS */ 88# define assert(expr) \ 89 ((expr) \ 90 ? __ASSERT_VOID_CAST (0) \ 91 : __assert_fail (__STRING(expr), __FILE__, __LINE__, __ASSERT_FUNCTION)) 92 From above, for NDEBUG, it may do nothing when assert fail, for Not NDEBUG, it will print and abort when assert fail. So, for NDEBUG, assert will not work when offset is 0x10000d. How do you think? Br， Guoxiang Niu 华为技术有限公司 Huawei Technologies Co., Ltd. 本邮件及其附件含有华为公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁 止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中 的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from HUAWEI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it! -----邮件原件----- 发件人: Peter Maydell [mailto:peter.maydell@linaro.org] 发送时间: 2017年9月5日 19:59 收件人: P J P 抄送: QEMU Developers; qemu-arm; niuguoxiang; Prasad J Pandit 主题: Re: [PATCH] intc: arm_gicv3: limit GICR ipriority index On 5 September 2017 at 12:21, P J P <ppandit@redhat.com<mailto:ppandit@redhat.com>> wrote: > From: Prasad J Pandit <pjp@fedoraproject.org<mailto:pjp@fedoraproject.org>> > > When reading or writing to GICR ipriority array, index 'irq' > could go beyond its bounds; Restrict it within array limits. > > Reported-by: Guoxiang Niu <niuguoxiang@huawei.com<mailto:niuguoxiang@huawei.com>> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org<mailto:pjp@fedoraproject.org>> > --- > hw/intc/arm_gicv3_redist.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c > index 77e5cfa327..7683c4cc7f 100644 > --- a/hw/intc/arm_gicv3_redist.c > +++ b/hw/intc/arm_gicv3_redist.c > @@ -187,7 +187,7 @@ static MemTxResult gicr_readl(GICv3CPUState *cs, hwaddr offset, > case GICR_ICACTIVER0: > *data = gicr_read_bitmap_reg(cs, attrs, cs->gicr_iactiver0); > return MEMTX_OK; > - case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1f: > + case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1c: > { > int i, irq = offset - GICR_IPRIORITYR; > uint32_t value = 0; > @@ -310,7 +310,7 @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset, > case GICR_ICACTIVER0: > gicr_write_clear_bitmap_reg(cs, attrs, &cs->gicr_iactiver0, value); > return MEMTX_OK; > - case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1f: > + case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1c: > { > int i, irq = offset - GICR_IPRIORITYR; Why do you think the buffer can be overrun? These functions are the word (4 byte) access functions, and they cannot be called with a non-4-aligned offset (see the asserts in gicv3_redist_read() and gicv3_redist_write()). thanks -- PMM
On 5 September 2017 at 13:30, niuguoxiang <niuguoxiang@huawei.com> wrote: > I think only assert is not enough, because assert() depends on NDEBUG > preprocessing The code cannot be reached with a non-aligned value, because we register these functions via the gic_ops[] MemoryRegionops in hw/intc/arm_gicv3.c, and since we do not specify .valid.unaligned=true there, the memory.c code will throw out attempts at unaligned accesses. The assert is just checking at runtime that this never becomes false accidentally (and also for the benefit of people reading the code). Incidentally, QEMU can never be compiled with NDEBUG not set -- we will #error in the compilation if it is not set. (It's not good practice to depend on the assert() actually doing anything though, and indeed in this case we don't.) thanks -- PMM
On 09/05/2017 07:35 AM, Peter Maydell wrote: > On 5 September 2017 at 13:30, niuguoxiang <niuguoxiang@huawei.com> wrote: >> I think only assert is not enough, because assert() depends on NDEBUG >> preprocessing > > Incidentally, QEMU can never be compiled with NDEBUG not > set -- we will #error in the compilation if it is not set. Well, right now, we only #error if you happen to compile certain devices, although the proposal has been made to #error for ALL builds: https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03608.html
+-- On Tue, 5 Sep 2017, Peter Maydell wrote --+ | The code cannot be reached with a non-aligned value, | because we register these functions via the gic_ops[] | MemoryRegionops in hw/intc/arm_gicv3.c, and since we | do not specify .valid.unaligned=true there, the memory.c | code will throw out attempts at unaligned accesses. I see, thank you for the clarification; ie. within the range GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1f: 'offset' would be GICR_IPRIORITYR +0, +4, +8 ... 0x1c? Maybe the patch could still be considered, not as bug fix though. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c index 77e5cfa327..7683c4cc7f 100644 --- a/hw/intc/arm_gicv3_redist.c +++ b/hw/intc/arm_gicv3_redist.c @@ -187,7 +187,7 @@ static MemTxResult gicr_readl(GICv3CPUState *cs, hwaddr offset, case GICR_ICACTIVER0: *data = gicr_read_bitmap_reg(cs, attrs, cs->gicr_iactiver0); return MEMTX_OK; - case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1f: + case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1c: { int i, irq = offset - GICR_IPRIORITYR; uint32_t value = 0; @@ -310,7 +310,7 @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset, case GICR_ICACTIVER0: gicr_write_clear_bitmap_reg(cs, attrs, &cs->gicr_iactiver0, value); return MEMTX_OK; - case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1f: + case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1c: { int i, irq = offset - GICR_IPRIORITYR;