[Qemu-devel] [PATCH v1] highbank: validate register offset before access

P J P posted 1 patch 6 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20171111081111.32724-1-ppandit@redhat.com
Test checkpatch failed
Test docker passed
Test ppc passed
Test s390x passed
There is a newer version of this series
hw/arm/highbank.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
[Qemu-devel] [PATCH v1] highbank: validate register offset before access
Posted by P J P 6 years, 4 months ago
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 | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Update: log error message(via qemu_log_mask) before returning
  -> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg01914.html

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index 354c6b25a8..8494dc6a48 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -34,6 +34,7 @@
 #include "hw/ide/ahci.h"
 #include "hw/cpu/a9mpcore.h"
 #include "hw/cpu/a15mpcore.h"
+#include "qemu/log.h"
 
 #define SMP_BOOT_ADDR           0x100
 #define SMP_BOOT_REG            0x40
@@ -117,14 +118,26 @@ static void hb_regs_write(void *opaque, hwaddr offset,
         }
     }
 
+    if (offset / 4 >= NUM_REGS) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "highbank: bad write offset 0x%x\n", (uint32_t)offset);
+        return;
+    }
     regs[offset/4] = value;
 }
 
 static uint64_t hb_regs_read(void *opaque, hwaddr offset,
                              unsigned size)
 {
+    uint32_t value;
     uint32_t *regs = opaque;
-    uint32_t value = regs[offset/4];
+
+    if (offset / 4 >= NUM_REGS) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "highbank: bad read offset 0x%x\n", (uint32_t)offset);
+        return 0;
+    }
+    value = regs[offset/4];
 
     if ((offset == 0x100) || (offset == 0x108) || (offset == 0x10C)) {
         value |= 0x30000000;
-- 
2.13.6


Re: [Qemu-devel] [PATCH v1] highbank: validate register offset before access
Posted by no-reply@patchew.org 6 years, 4 months ago
Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH v1] highbank: validate register offset before access
Type: series
Message-id: 20171111081111.32724-1-ppandit@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/1510343626-25861-1-git-send-email-cota@braap.org -> patchew/1510343626-25861-1-git-send-email-cota@braap.org
 t [tag update]            patchew/20171030163309.75770-1-vsementsov@virtuozzo.com -> patchew/20171030163309.75770-1-vsementsov@virtuozzo.com
 t [tag update]            patchew/20171110221329.24176-1-mreitz@redhat.com -> patchew/20171110221329.24176-1-mreitz@redhat.com
 * [new tag]               patchew/20171111081111.32724-1-ppandit@redhat.com -> patchew/20171111081111.32724-1-ppandit@redhat.com
Switched to a new branch 'test'
23df8ad060 highbank: validate register offset before access

=== OUTPUT BEGIN ===
Checking PATCH 1/1: highbank: validate register offset before access...
ERROR: spaces required around that '/' (ctx:VxV)
#50: FILE: hw/arm/highbank.c:140:
+    value = regs[offset/4];
                        ^

total: 1 errors, 0 warnings, 34 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
Re: [Qemu-devel] [PATCH v1] highbank: validate register offset before access
Posted by Philippe Mathieu-Daudé 6 years, 4 months ago
Hi Prasad,

Thanks for updating this patch so quickly :)

On 11/11/2017 05:11 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 | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> Update: log error message(via qemu_log_mask) before returning
>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg01914.html
> 
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index 354c6b25a8..8494dc6a48 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -34,6 +34,7 @@
>  #include "hw/ide/ahci.h"
>  #include "hw/cpu/a9mpcore.h"
>  #include "hw/cpu/a15mpcore.h"
> +#include "qemu/log.h"
>  
>  #define SMP_BOOT_ADDR           0x100
>  #define SMP_BOOT_REG            0x40
> @@ -117,14 +118,26 @@ static void hb_regs_write(void *opaque, hwaddr offset,
>          }
>      }
>  
> +    if (offset / 4 >= NUM_REGS) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "highbank: bad write offset 0x%x\n", (uint32_t)offset);

I'd rather use:

           "highbank: bad write offset 0x%" HWADDR_PRIx "\n", offset);

> +        return;
> +    }
>      regs[offset/4] = value;
>  }
>  
>  static uint64_t hb_regs_read(void *opaque, hwaddr offset,
>                               unsigned size)
>  {
> +    uint32_t value;
>      uint32_t *regs = opaque;
> -    uint32_t value = regs[offset/4];
> +
> +    if (offset / 4 >= NUM_REGS) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "highbank: bad read offset 0x%x\n", (uint32_t)offset);

Ditto HWADDR_PRIx.

> +        return 0;
> +    }
> +    value = regs[offset/4];

  +    value = regs[offset / 4];

With checkpatch fixed:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>  
>      if ((offset == 0x100) || (offset == 0x108) || (offset == 0x10C)) {
>          value |= 0x30000000;
> 

Re: [Qemu-devel] [PATCH v1] highbank: validate register offset before access
Posted by P J P 6 years, 4 months ago
  Hello Philippe,

+-- On Sun, 12 Nov 2017, Philippe Mathieu-Daudé wrote --+
| I'd rather use:
| 
|            "highbank: bad write offset 0x%" HWADDR_PRIx "\n", offset);

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