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

[li qiang]

在 2018/11/6 20:03, Peter Maydell 写道:
> On 6 November 2018 at 11:53, P J P <address@hidden> wrote:
>> From: Prasad J Pandit <address@hidden>
>>
>> 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 <address@hidden>
>> Signed-off-by: Prasad J Pandit <address@hidden>
>> ---
>>   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
>