diff mbox

highbank: validate register offset before access

Message ID 20171109115814.28232-1-ppandit@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Prasad Pandit Nov. 9, 2017, 11:58 a.m. UTC 
From: Prasad J Pandit <pjp@fedoraproject.org>

An 'offset' parameter sent to highbank register r/w functions
could be greater than number(NUM_REGS=0x200) of hb registers,
leading to an OOB access issue. Add check to avoid it.

Reported-by: Moguofang (Dennis mo) <moguofang@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/arm/highbank.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Philippe Mathieu-Daudé Nov. 10, 2017, 1:35 p.m. UTC | #1 
Hi Prasad, Moguofang.

On 11/09/2017 08:58 AM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> An 'offset' parameter sent to highbank register r/w functions
> could be greater than number(NUM_REGS=0x200) of hb registers,
> leading to an OOB access issue. Add check to avoid it.
> 
> Reported-by: Moguofang (Dennis mo) <moguofang@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/arm/highbank.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index 354c6b25a8..94df151454 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -117,6 +117,9 @@ static void hb_regs_write(void *opaque, hwaddr offset,
>          }
>      }
>  
> +    if (offset / 4 >= NUM_REGS) {

I'd report that:

           qemu_log_mask(LOG_UNIMP, ...

Cc'ing Shawn & Rob since this might also be a LOG_GUEST_ERROR.

> +        return;
> +    }
>      regs[offset/4] = value;
>  }
>  
> @@ -124,6 +127,10 @@ static uint64_t hb_regs_read(void *opaque, hwaddr offset,
>                               unsigned size)
>  {
>      uint32_t *regs = opaque;
> +
> +    if (offset / 4 >= NUM_REGS) {

Ditto.

> +        return 0;
> +    }

From CODING_STYLE:

Mixed declarations (interleaving statements and declarations within
blocks) are generally not allowed; declarations should be at the
beginning of blocks.

>      uint32_t value = regs[offset/4];
>  
>      if ((offset == 0x100) || (offset == 0x108) || (offset == 0x10C)) {

Regards,

Phil.
Prasad Pandit Nov. 11, 2017, 8:13 a.m. UTC | #2 
Hello Philippe,

+-- On Fri, 10 Nov 2017, Philippe Mathieu-Daudé wrote --+
| I'd report that:
| 
|            qemu_log_mask(LOG_UNIMP, ...
| 
| Cc'ing Shawn & Rob since this might also be a LOG_GUEST_ERROR.
| 
| Mixed declarations (interleaving statements and declarations within
| blocks) are generally not allowed; declarations should be at the
| beginning of blocks.

Done. Sent a revised patch v1.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
diff mbox

Patch

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index 354c6b25a8..94df151454 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -117,6 +117,9 @@  static void hb_regs_write(void *opaque, hwaddr offset,
         }
     }
 
+    if (offset / 4 >= NUM_REGS) {
+        return;
+    }
     regs[offset/4] = value;
 }
 
@@ -124,6 +127,10 @@  static uint64_t hb_regs_read(void *opaque, hwaddr offset,
                              unsigned size)
 {
     uint32_t *regs = opaque;
+
+    if (offset / 4 >= NUM_REGS) {
+        return 0;
+    }
     uint32_t value = regs[offset/4];
 
     if ((offset == 0x100) || (offset == 0x108) || (offset == 0x10C)) {