intc: arm_gicv3: limit GICR ipriority index

From: Prasad J Pandit <[email protected]>

When reading or writing to GICR ipriority array, index 'irq' could go beyond its bounds; Restrict it within array limits.

---
 hw/intc/arm_gicv3_redist.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 77e5cfa327..7683c4cc7f 100644
@@ -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;
 

2.13.5

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

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:[email protected]] 发送时间: 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

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

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

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

+-- 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