hw/arm/highbank.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
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
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
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; >
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
© 2016 - 2024 Red Hat, Inc.