highbank: validate register offset before access
PATCH 1/1 highbank: validate register offset before access
From: Prasad J Pandit <[email protected]>
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.
--- 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
hw/arm/highbank.c +14 -1
@@ -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 ;
Signature · 1 line
2.13.6
This series seems to have some coding style problems. See output below for more information:
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: [email protected]
=== 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/[email protected] -> patchew/[email protected] t [tag update] patchew/[email protected] -> patchew/[email protected] t [tag update] patchew/[email protected] -> patchew/[email protected] * [new tag] patchew/[email protected] -> patchew/[email protected]
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 [email protected]
Hi Prasad,
Thanks for updating this patch so quickly :)
Quoted 28 lines · P J P
From: Prasad J Pandit <[email protected]>
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) <[email protected]>Signed-off-by Prasad J Pandit <[email protected]>--- 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.htmlhw/arm/highbank.c +1
-0@@ -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
hw/arm/highbank.c static void hb_regs_write(void *opaque, hwaddr offset, · line 123
Quoted 6 lines
hw/arm/highbank.c +3
-0@@ -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);
Quoted 5 lines
hw/arm/highbank.c +2
-0+return ; + } regs[offset/4 ] = value; }
hw/arm/highbank.c static void hb_regs_write(void *opaque, hwaddr offset, · line 137
Quoted 10 lines
hw/arm/highbank.c +5
-1static 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.
hw/arm/highbank.c static void hb_regs_write(void *opaque, hwaddr offset, · line 140
Quoted 3 lines
hw/arm/highbank.c +3
-0+return 0 ; + } + value = regs[offset/4 ];
+ value = regs[offset / 4];
With checkpatch fixed:
Quoted 4 lines
hw/arm/highbank.c +0
-0if ((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