Skip to contentSkip to conversation

lsi53c895a: check script ram address value

PATCH In review P J P 4 participants 9 messages Nov 6, 2018 – Nov 21, 2018

lore
Share & tools
lore

P J P

author
#0 lore raw

PATCH 1/1 lsi53c895a: check script ram address value

From: Prasad J Pandit <[email protected]>

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 <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
---
 hw/scsi/lsi53c895a.c | 2 ++
 1 file changed, 2 insertions(+)
hw/scsi/lsi53c895a.c +2 -0
index 3f207f607c..0800df416e 100644
@@ -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;
Signature · 1 line
2.17.2
#1 lore raw
Quoted 25 lines · P J P

From: Prasad J Pandit <[email protected]>

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 <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
---
 hw/scsi/lsi53c895a.c | 2 ++
 1 file changed, 2 insertions(+)
hw/scsi/lsi53c895a.c +1 -0
index 3f207f607c..0800df416e 100644
@@ -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;

hw/scsi/lsi53c895a.c static uint64_t lsi_ram_read(void *opaque, hwaddr addr,

Quoted 8 lines
hw/scsi/lsi53c895a.c +1 -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

#2 lore raw
Quoted 10 lines · Peter Maydell

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

author
#3 lore raw

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

#4 lore raw
In reply to Peter Maydell
Quoted 36 lines · Peter Maydell
Quoted 25 lines · P J P

From: Prasad J Pandit <[email protected]>

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 <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
---
  hw/scsi/lsi53c895a.c | 2 ++
  1 file changed, 2 insertions(+)
hw/scsi/lsi53c895a.c +1 -0
index 3f207f607c..0800df416e 100644
@@ -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;

hw/scsi/lsi53c895a.c static uint64_t lsi_ram_read(void *opaque, hwaddr addr,

Quoted 8 lines
hw/scsi/lsi53c895a.c +1 -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

Quoted 5 lines
     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

Quoted 6 lines

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

thanks
-- PMM

#5 lore raw
Quoted 5 lines · li qiang

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

#6 lore raw
Quoted 7 lines · Paolo Bonzini
Quoted 5 lines · li qiang

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.

Quoted 1 line

Paolo

#7 lore raw
Quoted 6 lines · li qiang
Quoted 4 lines · Paolo Bonzini
Quoted 3 lines · li qiang

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

Quoted 7 lines
Quoted 3 lines
Quoted 2 lines

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

#8 lore raw
In reply to li qiang
Quoted 5 lines · li qiang

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