* [Qemu-devel] [PATVH v2] net: ne2000: fix bounds check in ioport operations
@ 2015-12-31 4:53 P J P
2015-12-31 5:20 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: P J P @ 2015-12-31 4:53 UTC (permalink / raw)
To: Jason Wang; +Cc: Prasad J Pandit, QEMU Developers, Ling Liu
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)) {
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;
--
2.4.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATVH v2] net: ne2000: fix bounds check in ioport operations
2015-12-31 4:53 [Qemu-devel] [PATVH v2] net: ne2000: fix bounds check in ioport operations P J P
@ 2015-12-31 5:20 ` Jason Wang
2015-12-31 5:56 ` P J P
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2015-12-31 5:20 UTC (permalink / raw)
To: P J P; +Cc: Ling Liu, Prasad J Pandit, QEMU Developers
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;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATVH v2] net: ne2000: fix bounds check in ioport operations
2015-12-31 5:20 ` Jason Wang
@ 2015-12-31 5:56 ` P J P
2015-12-31 7:18 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: P J P @ 2015-12-31 5:56 UTC (permalink / raw)
To: Jason Wang; +Cc: Ling Liu, Prasad J Pandit, QEMU Developers
+-- 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATVH v2] net: ne2000: fix bounds check in ioport operations
2015-12-31 5:56 ` P J P
@ 2015-12-31 7:18 ` Jason Wang
2015-12-31 11:49 ` P J P
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2015-12-31 7:18 UTC (permalink / raw)
To: P J P; +Cc: Ling Liu, Prasad J Pandit, QEMU Developers
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATVH v2] net: ne2000: fix bounds check in ioport operations
2015-12-31 7:18 ` Jason Wang
@ 2015-12-31 11:49 ` P J P
0 siblings, 0 replies; 5+ messages in thread
From: P J P @ 2015-12-31 11:49 UTC (permalink / raw)
To: Jason Wang; +Cc: QEMU Developers, Ling Liu
+-- 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-12-31 11:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-31 4:53 [Qemu-devel] [PATVH v2] net: ne2000: fix bounds check in ioport operations P J P
2015-12-31 5:20 ` Jason Wang
2015-12-31 5:56 ` P J P
2015-12-31 7:18 ` Jason Wang
2015-12-31 11:49 ` P J P
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.