[Qemu-devel] [PATCH] hw/pci-host/bonito: Avoid buffer overrun for bad LDMA/COP accesses

[Qemu-devel] [PATCH] hw/pci-host/bonito: Avoid buffer overrun for bad LDMA/COP accesses

[Peter Maydell]

The LDMA and COP memory regions represent four 32 bit registers
each, but the memory regions themselves are 0x100 bytes large.
Add guards to the read and write accessors so that bogus accesses
beyond the four defined registers don't just run off the end of
the bonldma and boncop structs and into whatever lies beyond.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I don't have a fulong2e image, so this is compile tested only...

 hw/pci-host/bonito.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
index 3a731fe..4139a2c 100644
--- a/hw/pci-host/bonito.c
+++ b/hw/pci-host/bonito.c
@@ -355,6 +355,10 @@ static uint64_t bonito_ldma_readl(void *opaque, hwaddr addr,
     uint32_t val;
     PCIBonitoState *s = opaque;
 
+    if (addr >= sizeof(s->bonldma)) {
+        return 0;
+    }
+
     val = ((uint32_t *)(&s->bonldma))[addr/sizeof(uint32_t)];
 
     return val;
@@ -365,6 +369,10 @@ static void bonito_ldma_writel(void *opaque, hwaddr addr,
 {
     PCIBonitoState *s = opaque;
 
+    if (addr >= sizeof(s->bonldma)) {
+        return;
+    }
+
     ((uint32_t *)(&s->bonldma))[addr/sizeof(uint32_t)] = val & 0xffffffff;
 }
 
@@ -384,6 +392,10 @@ static uint64_t bonito_cop_readl(void *opaque, hwaddr addr,
     uint32_t val;
     PCIBonitoState *s = opaque;
 
+    if (addr >= sizeof(s->boncop)) {
+        return 0;
+    }
+
     val = ((uint32_t *)(&s->boncop))[addr/sizeof(uint32_t)];
 
     return val;
@@ -394,6 +406,10 @@ static void bonito_cop_writel(void *opaque, hwaddr addr,
 {
     PCIBonitoState *s = opaque;
 
+    if (addr >= sizeof(s->boncop)) {
+        return;
+    }
+
     ((uint32_t *)(&s->boncop))[addr/sizeof(uint32_t)] = val & 0xffffffff;
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH] hw/pci-host/bonito: Avoid buffer overrun for bad LDMA/COP accesses

[Aurelien Jarno]

On 2015-07-30 16:33, Peter Maydell wrote:
> The LDMA and COP memory regions represent four 32 bit registers
> each, but the memory regions themselves are 0x100 bytes large.
> Add guards to the read and write accessors so that bogus accesses
> beyond the four defined registers don't just run off the end of
> the bonldma and boncop structs and into whatever lies beyond.

Thanks for finding that. I don't know if it is better to reduce the
memory region or just ignore the access as in your patch. I haven't
found any documentation about the bonito northbridge, so I think it's
safer to go like in your patch.

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I don't have a fulong2e image, so this is compile tested only...

I have just tested, it still boots fine with the change.

>  hw/pci-host/bonito.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)

Acked-by: Aurelien Jarno <aurelien@aurel32.net>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] hw/pci-host/bonito: Avoid buffer overrun for bad LDMA/COP accesses

[Peter Maydell]

On 30 July 2015 at 23:02, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On 2015-07-30 16:33, Peter Maydell wrote:
>> The LDMA and COP memory regions represent four 32 bit registers
>> each, but the memory regions themselves are 0x100 bytes large.
>> Add guards to the read and write accessors so that bogus accesses
>> beyond the four defined registers don't just run off the end of
>> the bonldma and boncop structs and into whatever lies beyond.
>
> Thanks for finding that. I don't know if it is better to reduce the
> memory region or just ignore the access as in your patch. I haven't
> found any documentation about the bonito northbridge, so I think it's
> safer to go like in your patch.

I did find some documentation by random googling -- but it just
defines that there are four valid registers in each region,
and doesn't say anything about what happens in the gaps
in between...

> I have just tested, it still boots fine with the change.
>
>>  hw/pci-host/bonito.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>
> Acked-by: Aurelien Jarno <aurelien@aurel32.net>

Thanks. (I haven't marked this as for-2.4 because
it's been like this since forever, and fulong2e isn't a
KVM board we care about security on; this is just a random
cleanup I happened to remember about. I could be persuaded
that it ought to go in, though.)

-- PMM