* [Qemu-devel] [PATCH] lsi53c895a: check script ram address value
@ 2018-11-06 11:53 P J P
2018-11-06 12:03 ` Peter Maydell
0 siblings, 1 reply; 9+ messages in thread
From: P J P @ 2018-11-06 11:53 UTC (permalink / raw)
To: Qemu Developers; +Cc: Paolo Bonzini, Fam Zheng, Prasad J Pandit
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value
2018-11-06 11:53 [Qemu-devel] [PATCH] lsi53c895a: check script ram address value P J P
@ 2018-11-06 12:03 ` Peter Maydell
2018-11-06 12:22 ` Paolo Bonzini
2018-11-06 12:27 ` li qiang
0 siblings, 2 replies; 9+ messages in thread
From: Peter Maydell @ 2018-11-06 12:03 UTC (permalink / raw)
To: P J P; +Cc: Qemu Developers, Paolo Bonzini, Fam Zheng, Prasad J Pandit
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value
2018-11-06 12:03 ` Peter Maydell
@ 2018-11-06 12:22 ` Paolo Bonzini
2018-11-21 9:29 ` P J P
2018-11-06 12:27 ` li qiang
1 sibling, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2018-11-06 12:22 UTC (permalink / raw)
To: Peter Maydell, P J P; +Cc: Qemu Developers, Fam Zheng, Prasad J Pandit
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value
2018-11-06 12:03 ` Peter Maydell
2018-11-06 12:22 ` Paolo Bonzini
@ 2018-11-06 12:27 ` li qiang
2018-11-06 12:28 ` Paolo Bonzini
2018-11-06 12:37 ` Peter Maydell
1 sibling, 2 replies; 9+ messages in thread
From: li qiang @ 2018-11-06 12:27 UTC (permalink / raw)
To: Peter Maydell, P J P
Cc: Paolo Bonzini, Fam Zheng, Qemu Developers, Prasad J Pandit
在 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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value
2018-11-06 12:27 ` li qiang
@ 2018-11-06 12:28 ` Paolo Bonzini
2018-11-06 12:38 ` li qiang
2018-11-06 12:37 ` Peter Maydell
1 sibling, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2018-11-06 12:28 UTC (permalink / raw)
To: li qiang, Peter Maydell, P J P
Cc: Fam Zheng, Qemu Developers, Prasad J Pandit
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value
2018-11-06 12:27 ` li qiang
2018-11-06 12:28 ` Paolo Bonzini
@ 2018-11-06 12:37 ` Peter Maydell
1 sibling, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2018-11-06 12:37 UTC (permalink / raw)
To: li qiang
Cc: P J P, Paolo Bonzini, Fam Zheng, Qemu Developers, Prasad J Pandit
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value
2018-11-06 12:28 ` Paolo Bonzini
@ 2018-11-06 12:38 ` li qiang
2018-11-06 12:44 ` Peter Maydell
0 siblings, 1 reply; 9+ messages in thread
From: li qiang @ 2018-11-06 12:38 UTC (permalink / raw)
To: Paolo Bonzini, Peter Maydell, P J P
Cc: Fam Zheng, Qemu Developers, Prasad J Pandit
在 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value
2018-11-06 12:38 ` li qiang
@ 2018-11-06 12:44 ` Peter Maydell
0 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2018-11-06 12:44 UTC (permalink / raw)
To: li qiang
Cc: Paolo Bonzini, P J P, Fam Zheng, Qemu Developers, Prasad J Pandit
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check script ram address value
2018-11-06 12:22 ` Paolo Bonzini
@ 2018-11-21 9:29 ` P J P
0 siblings, 0 replies; 9+ messages in thread
From: P J P @ 2018-11-21 9:29 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Peter Maydell, Qemu Developers, Fam Zheng
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-11-21 9:29 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-06 11:53 [Qemu-devel] [PATCH] lsi53c895a: check script ram address value P J P
2018-11-06 12:03 ` Peter Maydell
2018-11-06 12:22 ` Paolo Bonzini
2018-11-21 9:29 ` P J P
2018-11-06 12:27 ` li qiang
2018-11-06 12:28 ` Paolo Bonzini
2018-11-06 12:38 ` li qiang
2018-11-06 12:44 ` Peter Maydell
2018-11-06 12:37 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).