| Message ID | 20181106115351.9422-1-ppandit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | lsi53c895a: check script ram address value | expand |
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
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
在 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 >
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
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
在 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
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
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
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;