* [Qemu-devel] [PATCH v1] highbank: validate register offset before access
@ 2017-11-11 8:11 P J P
2017-11-11 8:15 ` no-reply
2017-11-12 17:34 ` Philippe Mathieu-Daudé
0 siblings, 2 replies; 4+ messages in thread
From: P J P @ 2017-11-11 8:11 UTC (permalink / raw)
To: Qemu Developers
Cc: Philippe Mathieu-Daudé,
Moguofang, Peter Maydell, qemu-arm, Shawn Guo, Rob Herring,
Prasad J Pandit
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v1] highbank: validate register offset before access
2017-11-11 8:11 [Qemu-devel] [PATCH v1] highbank: validate register offset before access P J P
@ 2017-11-11 8:15 ` no-reply
2017-11-12 17:34 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 4+ messages in thread
From: no-reply @ 2017-11-11 8:15 UTC (permalink / raw)
To: ppandit
Cc: famz, qemu-devel, robh, pjp, peter.maydell, f4bug, qemu-arm,
shawn.guo, moguofang
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v1] highbank: validate register offset before access
2017-11-11 8:11 [Qemu-devel] [PATCH v1] highbank: validate register offset before access P J P
2017-11-11 8:15 ` no-reply
@ 2017-11-12 17:34 ` Philippe Mathieu-Daudé
2017-11-13 8:10 ` P J P
1 sibling, 1 reply; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-11-12 17:34 UTC (permalink / raw)
To: P J P, Qemu Developers
Cc: Moguofang, Peter Maydell, qemu-arm, Shawn Guo, Rob Herring,
Prasad J Pandit
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;
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v1] highbank: validate register offset before access
2017-11-12 17:34 ` Philippe Mathieu-Daudé
@ 2017-11-13 8:10 ` P J P
0 siblings, 0 replies; 4+ messages in thread
From: P J P @ 2017-11-13 8:10 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Qemu Developers, Moguofang, Peter Maydell, qemu-arm, Shawn Guo,
Rob Herring
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-11-13 8:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-11 8:11 [Qemu-devel] [PATCH v1] highbank: validate register offset before access P J P
2017-11-11 8:15 ` no-reply
2017-11-12 17:34 ` Philippe Mathieu-Daudé
2017-11-13 8:10 ` P J P
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.