Message ID | 1451537606-16030-1-git-send-email-ppandit@redhat.com |
---|---|
State | New |
Headers | show |
On 12/31/2015 12:53 PM, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > While doing ioport r/w operations, ne2000 device emulation suffers > from OOB r/w errors. Update respective array bounds check to avoid > OOB access. > > Reported-by: Ling Liu <liuling-it@360.cn> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/net/ne2000.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > Updated as per review in > -> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg04856.html > > diff --git a/hw/net/ne2000.c b/hw/net/ne2000.c > index 010f9ef..d1f764b 100644 > --- a/hw/net/ne2000.c > +++ b/hw/net/ne2000.c > @@ -447,8 +447,7 @@ static uint32_t ne2000_ioport_read(void *opaque, uint32_t addr) > static inline void ne2000_mem_writeb(NE2000State *s, uint32_t addr, > uint32_t val) > { > - if (addr < 32 || > - (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { > + if (addr < 32 || (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { The change is unnecessary. > s->mem[addr] = val; > } > } > @@ -457,9 +456,10 @@ static inline void ne2000_mem_writew(NE2000State *s, uint32_t addr, > uint32_t val) > { > addr &= ~1; /* XXX: check exact behaviour if not even */ > - if (addr < 32 || > - (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { > - *(uint16_t *)(s->mem + addr) = cpu_to_le16(val); > + if (addr < 32 > + || (addr >= NE2000_PMEM_START > + && addr + sizeof(uint16_t) < NE2000_MEM_SIZE)) { I think you mean '<=' instead of '<' here? (And for the other checks below). > + *(uint16_t *)(s->mem + addr) = cpu_to_le16(data); > } > } > > @@ -467,16 +467,16 @@ static inline void ne2000_mem_writel(NE2000State *s, uint32_t addr, > uint32_t val) > { > addr &= ~1; /* XXX: check exact behaviour if not even */ > - if (addr < 32 || > - (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { > + if (addr < 32 > + || (addr >= NE2000_PMEM_START > + && addr + sizeof(uint32_t) < NE2000_MEM_SIZE)) { > stl_le_p(s->mem + addr, val); > } > } > > static inline uint32_t ne2000_mem_readb(NE2000State *s, uint32_t addr) > { > - if (addr < 32 || > - (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { > + if (addr < 32 || (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { > return s->mem[addr]; > } else { > return 0xff; > @@ -486,8 +486,9 @@ static inline uint32_t ne2000_mem_readb(NE2000State *s, uint32_t addr) > static inline uint32_t ne2000_mem_readw(NE2000State *s, uint32_t addr) > { > addr &= ~1; /* XXX: check exact behaviour if not even */ > - if (addr < 32 || > - (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { > + if (addr < 32 > + || (addr >= NE2000_PMEM_START > + && addr + sizeof(uint16_t) < NE2000_MEM_SIZE)) { > return le16_to_cpu(*(uint16_t *)(s->mem + addr)); > } else { > return 0xffff; > @@ -497,8 +498,9 @@ static inline uint32_t ne2000_mem_readw(NE2000State *s, uint32_t addr) > static inline uint32_t ne2000_mem_readl(NE2000State *s, uint32_t addr) > { > addr &= ~1; /* XXX: check exact behaviour if not even */ > - if (addr < 32 || > - (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { > + if (addr < 32 > + || (addr >= NE2000_PMEM_START > + && addr + sizeof(uint32_t) < NE2000_MEM_SIZE)) { > return ldl_le_p(s->mem + addr); > } else { > return 0xffffffff;
+-- On Thu, 31 Dec 2015, Jason Wang wrote --+ | > - (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { | > + if (addr < 32 || (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { | | The change is unnecessary. Okay. | > + if (addr < 32 | > + || (addr >= NE2000_PMEM_START | > + && addr + sizeof(uint16_t) < NE2000_MEM_SIZE)) { | | I think you mean '<=' instead of '<' here? (And for the other checks below). I think <= would lead to an off-by-one, no? As the last array index would be one less than the size; Same as ne2000_mem_readb() above. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
On 12/31/2015 01:56 PM, P J P wrote: > +-- On Thu, 31 Dec 2015, Jason Wang wrote --+ > | > - (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { > | > + if (addr < 32 || (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { > | > | The change is unnecessary. > > Okay. > > | > + if (addr < 32 > | > + || (addr >= NE2000_PMEM_START > | > + && addr + sizeof(uint16_t) < NE2000_MEM_SIZE)) { > | > | I think you mean '<=' instead of '<' here? (And for the other checks below). > > I think <= would lead to an off-by-one, no? The real byte we could touch is in fact addr + sizeof(uint16_t) -1 here. Consider we should allow double bytes access at NE2000_MEM_SIZE - 2, but this patch forbids this. Btw, looking at ne2000_mem_writew(), it has: addr &= ~1; at the beginning, so looks like we are really safe, Need only to care about writel? > As the last array index would be > one less than the size; Same as ne2000_mem_readb() above. > > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
+-- On Thu, 31 Dec 2015, Jason Wang wrote --+ | Btw, looking at ne2000_mem_writew(), it has: | addr &= ~1; Yes, this seems to ensure that write starts at an even address. | at the beginning, so looks like we are really safe, Need only to care | about writel? Right, I've sent an updated patch for the same. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
diff --git a/hw/net/ne2000.c b/hw/net/ne2000.c index 010f9ef..d1f764b 100644 --- a/hw/net/ne2000.c +++ b/hw/net/ne2000.c @@ -447,8 +447,7 @@ static uint32_t ne2000_ioport_read(void *opaque, uint32_t addr) static inline void ne2000_mem_writeb(NE2000State *s, uint32_t addr, uint32_t val) { - if (addr < 32 || - (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { + if (addr < 32 || (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { s->mem[addr] = val; } } @@ -457,9 +456,10 @@ static inline void ne2000_mem_writew(NE2000State *s, uint32_t addr, uint32_t val) { addr &= ~1; /* XXX: check exact behaviour if not even */ - if (addr < 32 || - (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { - *(uint16_t *)(s->mem + addr) = cpu_to_le16(val); + if (addr < 32 + || (addr >= NE2000_PMEM_START + && addr + sizeof(uint16_t) < NE2000_MEM_SIZE)) { + *(uint16_t *)(s->mem + addr) = cpu_to_le16(data); } } @@ -467,16 +467,16 @@ static inline void ne2000_mem_writel(NE2000State *s, uint32_t addr, uint32_t val) { addr &= ~1; /* XXX: check exact behaviour if not even */ - if (addr < 32 || - (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { + if (addr < 32 + || (addr >= NE2000_PMEM_START + && addr + sizeof(uint32_t) < NE2000_MEM_SIZE)) { stl_le_p(s->mem + addr, val); } } static inline uint32_t ne2000_mem_readb(NE2000State *s, uint32_t addr) { - if (addr < 32 || - (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { + if (addr < 32 || (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { return s->mem[addr]; } else { return 0xff; @@ -486,8 +486,9 @@ static inline uint32_t ne2000_mem_readb(NE2000State *s, uint32_t addr) static inline uint32_t ne2000_mem_readw(NE2000State *s, uint32_t addr) { addr &= ~1; /* XXX: check exact behaviour if not even */ - if (addr < 32 || - (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { + if (addr < 32 + || (addr >= NE2000_PMEM_START + && addr + sizeof(uint16_t) < NE2000_MEM_SIZE)) { return le16_to_cpu(*(uint16_t *)(s->mem + addr)); } else { return 0xffff; @@ -497,8 +498,9 @@ static inline uint32_t ne2000_mem_readw(NE2000State *s, uint32_t addr) static inline uint32_t ne2000_mem_readl(NE2000State *s, uint32_t addr) { addr &= ~1; /* XXX: check exact behaviour if not even */ - if (addr < 32 || - (addr >= NE2000_PMEM_START && addr < NE2000_MEM_SIZE)) { + if (addr < 32 + || (addr >= NE2000_PMEM_START + && addr + sizeof(uint32_t) < NE2000_MEM_SIZE)) { return ldl_le_p(s->mem + addr); } else { return 0xffffffff;