lsi53c895a: check script ram address value

[Qemu-devel] [PATCH] lsi53c895a: check script ram address value

Posted by P J P 6 years, 12 months ago

From: Prasad J Pandit <pjp@fedoraproject.org>

While accessing script ram[2048] via 'lsi_ram_read/write' routines,
'addr' could exceed the ram range. Mask high order bits to avoid
OOB access.

Reported-by: Mark Kanda <mark.kanda@oracle.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/scsi/lsi53c895a.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 3f207f607c..0800df416e 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -2035,6 +2035,7 @@ static void lsi_ram_write(void *opaque, hwaddr addr,
     uint32_t mask;
     int shift;
 
+    addr &= 0x01FFF;
     newval = s->script_ram[addr >> 2];
     shift = (addr & 3) * 8;
     mask = ((uint64_t)1 << (size * 8)) - 1;
@@ -2050,6 +2051,7 @@ static uint64_t lsi_ram_read(void *opaque, hwaddr addr,
     uint32_t val;
     uint32_t mask;
 
+    addr &= 0x01FFF;
     val = s->script_ram[addr >> 2];
     mask = ((uint64_t)1 << (size * 8)) - 1;
     val >>= (addr & 3) * 8;
-- 
2.17.2

Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value

Posted by Peter Maydell 6 years, 12 months ago

On 6 November 2018 at 11:53, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While accessing script ram[2048] via 'lsi_ram_read/write' routines,
> 'addr' could exceed the ram range. Mask high order bits to avoid
> OOB access.
>
> Reported-by: Mark Kanda <mark.kanda@oracle.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/scsi/lsi53c895a.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index 3f207f607c..0800df416e 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -2035,6 +2035,7 @@ static void lsi_ram_write(void *opaque, hwaddr addr,
>      uint32_t mask;
>      int shift;
>
> +    addr &= 0x01FFF;
>      newval = s->script_ram[addr >> 2];
>      shift = (addr & 3) * 8;
>      mask = ((uint64_t)1 << (size * 8)) - 1;
> @@ -2050,6 +2051,7 @@ static uint64_t lsi_ram_read(void *opaque, hwaddr addr,
>      uint32_t val;
>      uint32_t mask;
>
> +    addr &= 0x01FFF;
>      val = s->script_ram[addr >> 2];
>      mask = ((uint64_t)1 << (size * 8)) - 1;
>      val >>= (addr & 3) * 8;
> --

When can this masking have any effect? These functions are
the read and write ops for lsi_ram_ops, which we register with
    memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s,
                          "lsi-ram", 0x2000);
which specifies a memory region size of 0x2000. So the input
addr must be in the 0..0x1fff range already -- or have I missed
something ?

It would probably be helpful (for readers and static analysers)
to assert() that addr is < 0x2000, though.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value

Posted by Paolo Bonzini 6 years, 12 months ago

On 06/11/2018 13:03, Peter Maydell wrote:
> When can this masking have any effect? These functions are
> the read and write ops for lsi_ram_ops, which we register with
>     memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s,
>                           "lsi-ram", 0x2000);
> which specifies a memory region size of 0x2000. So the input
> addr must be in the 0..0x1fff range already -- or have I missed
> something ?
> 
> It would probably be helpful (for readers and static analysers)
> to assert() that addr is < 0x2000, though.

Indeed, there are cases where the address is used blindly in a memcpy
with size>1, but this is not one of them.

Paolo

Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value

Posted by P J P 6 years, 11 months ago

  Hello Petr, Paolo,

+-- On Tue, 6 Nov 2018, Paolo Bonzini wrote --+
| On 06/11/2018 13:03, Peter Maydell wrote:
| > When can this masking have any effect? These functions are
| > the read and write ops for lsi_ram_ops, which we register with
| >     memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s,
| >                           "lsi-ram", 0x2000);
| > which specifies a memory region size of 0x2000. So the input
| > addr must be in the 0..0x1fff range already -- or have I missed
| > something ?
| > 
| > It would probably be helpful (for readers and static analysers)
| > to assert() that addr is < 0x2000, though.
| 
| Indeed, there are cases where the address is used blindly in a memcpy
| with size>1, but this is not one of them.

True, the lsi r/w mmio ops are initialized to be within MemoryRegion size of 
0x2000. IIUC memory_region_access_valid() does not seem to check that given 
'addr' is within mr->size limit. ie 'addr > 0x01FFF' may lead to oob access 
in doing

   val = s->script_ram[addr >> 2];

Hope I'm not misreading. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value

Posted by li qiang 6 years, 12 months ago

在 2018/11/6 20:03, Peter Maydell 写道:
> On 6 November 2018 at 11:53, P J P <ppandit@redhat.com> wrote:
>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>
>> While accessing script ram[2048] via 'lsi_ram_read/write' routines,
>> 'addr' could exceed the ram range. Mask high order bits to avoid
>> OOB access.
>>
>> Reported-by: Mark Kanda <mark.kanda@oracle.com>
>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>> ---
>>   hw/scsi/lsi53c895a.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
>> index 3f207f607c..0800df416e 100644
>> --- a/hw/scsi/lsi53c895a.c
>> +++ b/hw/scsi/lsi53c895a.c
>> @@ -2035,6 +2035,7 @@ static void lsi_ram_write(void *opaque, hwaddr addr,
>>       uint32_t mask;
>>       int shift;
>>
>> +    addr &= 0x01FFF;
>>       newval = s->script_ram[addr >> 2];
>>       shift = (addr & 3) * 8;
>>       mask = ((uint64_t)1 << (size * 8)) - 1;
>> @@ -2050,6 +2051,7 @@ static uint64_t lsi_ram_read(void *opaque, hwaddr addr,
>>       uint32_t val;
>>       uint32_t mask;
>>
>> +    addr &= 0x01FFF;
>>       val = s->script_ram[addr >> 2];
>>       mask = ((uint64_t)1 << (size * 8)) - 1;
>>       val >>= (addr & 3) * 8;
>> --
> When can this masking have any effect? These functions are
> the read and write ops for lsi_ram_ops, which we register with
>      memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s,
>                            "lsi-ram", 0x2000);
> which specifies a memory region size of 0x2000. So the input
> addr must be in the 0..0x1fff range already -- or have I missed
> something ?


Hello Peter,

There is a oob access indeed,

The addr is 0~0x1fff, but when addr is at the near the end ,for example 
0x1fffe, the add>>2 can be 2047

and as script_ram is a uint32_t and so s->script_ram[addr >> 2] can read 
out of the script_ram.

Some of the debug data.

Thread 5 "qemu-system-x86" hit Breakpoint 1, lsi_ram_read 
(opaque=0x62600000f100, addr=8188, size=4) at hw/scsi/lsi53c895a.c:2046
2046        LSIState *s = opaque;
(gdb) p /x addr
$12 = 0x1ffc
(gdb) p addr
$13 = 8188
(gdb) n
...
2050        val = s->script_ram[addr >> 2];
(gdb) p addr
$14 = 8188
(gdb) p addr >>2
$15 = 2047
(gdb) n
2051        mask = ((uint64_t)1 << (size * 8)) - 1;
(gdb) p /x val
$16 = 0x0
(gdb) n
2052        val >>= (addr & 3) * 8;
(gdb) n
2053        return val & mask;
(gdb) p /x mask
$17 = 0xffffffff
(gdb) p /s size
$18 = 4

But as you point Prasad's patch does nothing.

Hello Prasad,

I think you should check the addr with size to ensure the access

doesn't exceed script_ram.


Thanks,

Li Qiang


> It would probably be helpful (for readers and static analysers)
> to assert() that addr is < 0x2000, though.
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value

Posted by Paolo Bonzini 6 years, 12 months ago

On 06/11/2018 13:27, li qiang wrote:
> The addr is 0~0x1fff, but when addr is at the near the end ,for example 
> 0x1fffe, the add>>2 can be 2047
> 
> and as script_ram is a uint32_t and so s->script_ram[addr >> 2] can read 
> out of the script_ram.

How so?  s->script_ram has size 2048, it's okay to access it at 2047.

Paolo

Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value

Posted by li qiang 6 years, 12 months ago

在 2018/11/6 20:28, Paolo Bonzini 写道:
> On 06/11/2018 13:27, li qiang wrote:
>> The addr is 0~0x1fff, but when addr is at the near the end ,for example
>> 0x1fffe, the add>>2 can be 2047
>>
>> and as script_ram is a uint32_t and so s->script_ram[addr >> 2] can read
>> out of the script_ram.
> How so?  s->script_ram has size 2048, it's okay to access it at 2047.

Oh, right.

  I'm confused by the script_ram, it's not byte array.




> Paolo

Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value

Posted by Peter Maydell 6 years, 12 months ago

On 6 November 2018 at 12:38, li qiang <liq3ea@outlook.com> wrote:
>
> 在 2018/11/6 20:28, Paolo Bonzini 写道:
>> On 06/11/2018 13:27, li qiang wrote:
>>> The addr is 0~0x1fff, but when addr is at the near the end ,for example
>>> 0x1fffe, the add>>2 can be 2047
>>>
>>> and as script_ram is a uint32_t and so s->script_ram[addr >> 2] can read
>>> out of the script_ram.
>> How so?  s->script_ram has size 2048, it's okay to access it at 2047.
>
> Oh, right.
>
>   I'm confused by the script_ram, it's not byte array.

Incidentally, I think the read and write functions here
would be somewhat clearer written as

static void lsi_ram_write(void *opaque, hwaddr addr,
                          uint64_t val, unsigned size)
{
    LSIState *s = opaque;
    void *p = ((void *)s->script_ram) + addr;

    assert(addr + size <= sizeof(s->script_ram));
    stn_p(p, size, val);
}

static uint64_t lsi_ram_read(void *opaque, hwaddr addr,
                             unsigned size)
{
    LSIState *s = opaque;
    void *p = ((void *)s->script_ram) + addr;

    assert(addr + size <= sizeof(s->script_ram));
    return ldn_p(p, size);
}

thanks
-- PMM

Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value

Posted by Peter Maydell 6 years, 12 months ago

On 6 November 2018 at 12:27, li qiang <liq3ea@outlook.com> wrote:
> The addr is 0~0x1fff, but when addr is at the near the end ,for example
> 0x1fffe, the add>>2 can be 2047
>
> and as script_ram is a uint32_t and so s->script_ram[addr >> 2] can read
> out of the script_ram.

But script_ram is declared as
  uint32_t script_ram[2048];
so if addr >> 2 == 2047, that's still in-bounds, isn't it?

thanks
-- PMM