intc: arm_gicv3: limit GICR ipriority index

[Qemu-devel] [PATCH] intc: arm_gicv3: limit GICR ipriority index

Posted by P J P 6 years, 7 months ago

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;
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH] intc: arm_gicv3: limit GICR ipriority index

Posted by Peter Maydell 6 years, 7 months ago

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

Re: [Qemu-devel] [Qemu-arm] [PATCH] intc: arm_gicv3: limit GICR ipriority index

Posted by Philippe Mathieu-Daudé 6 years, 7 months ago

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
>

[Qemu-devel] 答复: [PATCH] intc: arm_gicv3: limit GICR ipriority index

Posted by niuguoxiang 6 years, 7 months ago

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

Re: [Qemu-devel]答复: [PATCH] intc: arm_gicv3: limit GICR ipriority index

Posted by Peter Maydell 6 years, 7 months ago

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

Re: [Qemu-devel]答复: [PATCH] intc: arm_gicv3: limit GICR ipriority index

Posted by Eric Blake 6 years, 7 months ago

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

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

Re: [Qemu-devel]答复: [PATCH] intc: arm_gicv3: limit GICR ipriority index

Posted by P J P 6 years, 7 months ago

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