* [Qemu-arm] [PATCH v3] net: smc91c111: check packet number and data register index
@ 2016-10-26 20:02 P J P
2016-10-27 14:47 ` [Qemu-devel] " Peter Maydell
0 siblings, 1 reply; 2+ messages in thread
From: P J P @ 2016-10-26 20:02 UTC (permalink / raw)
To: Qemu Developers
Cc: Azure Yang, Jason Wang, qemu-arm, Prasad J Pandit, Peter Maydell
From: Prasad J Pandit <pjp@fedoraproject.org>
SMSC91C111 Ethernet interface emulator has registers to store
'packet number' and a 'pointer' to Tx/Rx FIFO buffer area.
These two are used to derive an address to access into 'data'
registers. If they are set incorrectly, they could lead to an
OOB r/w access beyond packet 'data' area. Add check to avoid it.
Reported-by: Azure Yang <azureyang@tencent.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
hw/net/smc91c111.c | 32 ++++++++++++++++++++++++--------
1 file changed, 24 insertions(+), 8 deletions(-)
Update per: add check to _release_packet and for 's->allocated'
-> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06530.html
diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c
index 3b16dcf..e9b37c2 100644
--- a/hw/net/smc91c111.c
+++ b/hw/net/smc91c111.c
@@ -203,6 +203,9 @@ static void smc91c111_pop_tx_fifo_done(smc91c111_state *s)
/* Release the memory allocated to a packet. */
static void smc91c111_release_packet(smc91c111_state *s, int packet)
{
+ if (packet >= NUM_PACKETS || !(packet & s->allocated)) {
+ return;
+ }
s->allocated &= ~(1 << packet);
if (s->tx_alloc == 0x80)
smc91c111_tx_alloc(s);
@@ -224,6 +227,9 @@ static void smc91c111_do_tx(smc91c111_state *s)
return;
for (i = 0; i < s->tx_fifo_len; i++) {
packetnum = s->tx_fifo[i];
+ if (packetnum >= NUM_PACKETS || !(packetnum & s->allocated)) {
+ return;
+ }
p = &s->data[packetnum][0];
/* Set status word. */
*(p++) = 0x01;
@@ -418,7 +424,7 @@ static void smc91c111_writeb(void *opaque, hwaddr offset,
/* Ignore. */
return;
case 2: /* Packet Number Register */
- s->packet_num = value;
+ s->packet_num = value & 0x03F;
return;
case 3: case 4: case 5:
/* Should be readonly, but linux writes to them anyway. Ignore. */
@@ -438,13 +444,17 @@ static void smc91c111_writeb(void *opaque, hwaddr offset,
n = s->rx_fifo[0];
else
n = s->packet_num;
- p = s->ptr & 0x07ff;
+ p = s->ptr;
if (s->ptr & 0x4000) {
s->ptr = (s->ptr & 0xf800) | ((s->ptr + 1) & 0x7ff);
} else {
p += (offset & 3);
}
- s->data[n][p] = value;
+ p &= 0x07ff;
+ if (s->allocated < NUM_PACKETS
+ && n < NUM_PACKETS && n & s->allocated) {
+ s->data[n][p] = value;
+ }
}
return;
case 12: /* Interrupt ACK. */
@@ -517,7 +527,8 @@ static uint32_t smc91c111_readb(void *opaque, hwaddr offset)
int i;
int n;
n = 0;
- for (i = 0; i < NUM_PACKETS; i++) {
+ for (i = 0;
+ s->allocated < NUM_PACKETS && i < NUM_PACKETS; i++) {
if (s->allocated & (1 << i))
n++;
}
@@ -558,9 +569,9 @@ static uint32_t smc91c111_readb(void *opaque, hwaddr offset)
case 0: case 1: /* MMUCR Busy bit. */
return 0;
case 2: /* Packet Number. */
- return s->packet_num;
+ return s->packet_num & 0x3F;
case 3: /* Allocation Result. */
- return s->tx_alloc;
+ return s->tx_alloc < NUM_PACKETS ? s->tx_alloc : 0;
case 4: /* TX FIFO */
if (s->tx_fifo_done_len == 0)
return 0x80;
@@ -584,13 +595,18 @@ static uint32_t smc91c111_readb(void *opaque, hwaddr offset)
n = s->rx_fifo[0];
else
n = s->packet_num;
- p = s->ptr & 0x07ff;
+ p = s->ptr;
if (s->ptr & 0x4000) {
s->ptr = (s->ptr & 0xf800) | ((s->ptr + 1) & 0x07ff);
} else {
p += (offset & 3);
}
- return s->data[n][p];
+ p &= 0x07ff;
+ if (s->allocated < NUM_PACKETS
+ && n < NUM_PACKETS && n & s->allocated) {
+ return s->data[n][p];
+ }
+ return 0;
}
case 12: /* Interrupt status. */
return s->int_level;
--
2.7.4
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Qemu-devel] [PATCH v3] net: smc91c111: check packet number and data register index
2016-10-26 20:02 [Qemu-arm] [PATCH v3] net: smc91c111: check packet number and data register index P J P
@ 2016-10-27 14:47 ` Peter Maydell
0 siblings, 0 replies; 2+ messages in thread
From: Peter Maydell @ 2016-10-27 14:47 UTC (permalink / raw)
To: P J P; +Cc: Azure Yang, Jason Wang, qemu-arm, Qemu Developers,
Prasad J Pandit
On 26 October 2016 at 21:02, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> SMSC91C111 Ethernet interface emulator has registers to store
> 'packet number' and a 'pointer' to Tx/Rx FIFO buffer area.
> These two are used to derive an address to access into 'data'
> registers. If they are set incorrectly, they could lead to an
> OOB r/w access beyond packet 'data' area. Add check to avoid it.
>
> Reported-by: Azure Yang <azureyang@tencent.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> hw/net/smc91c111.c | 32 ++++++++++++++++++++++++--------
> 1 file changed, 24 insertions(+), 8 deletions(-)
>
> Update per: add check to _release_packet and for 's->allocated'
> -> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06530.html
>
> diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c
> index 3b16dcf..e9b37c2 100644
> --- a/hw/net/smc91c111.c
> +++ b/hw/net/smc91c111.c
> @@ -203,6 +203,9 @@ static void smc91c111_pop_tx_fifo_done(smc91c111_state *s)
> /* Release the memory allocated to a packet. */
> static void smc91c111_release_packet(smc91c111_state *s, int packet)
> {
> + if (packet >= NUM_PACKETS
Negative numbers ?
> || !(packet & s->allocated)) {
Is this supposed to be checking "is the bit number corresponding
to 'packet' in s->allocated set" ? It doesn't do that.
This change seems likely to break the normal operation of
the device -- how are you testing this patch?
> + return;
> + }
> s->allocated &= ~(1 << packet);
> if (s->tx_alloc == 0x80)
> smc91c111_tx_alloc(s);
> @@ -224,6 +227,9 @@ static void smc91c111_do_tx(smc91c111_state *s)
> return;
> for (i = 0; i < s->tx_fifo_len; i++) {
> packetnum = s->tx_fifo[i];
> + if (packetnum >= NUM_PACKETS || !(packetnum & s->allocated)) {
> + return;
> + }
Ditto.
> p = &s->data[packetnum][0];
> /* Set status word. */
> *(p++) = 0x01;
> @@ -418,7 +424,7 @@ static void smc91c111_writeb(void *opaque, hwaddr offset,
> /* Ignore. */
> return;
> case 2: /* Packet Number Register */
> - s->packet_num = value;
> + s->packet_num = value & 0x03F;
> return;
> case 3: case 4: case 5:
> /* Should be readonly, but linux writes to them anyway. Ignore. */
> @@ -438,13 +444,17 @@ static void smc91c111_writeb(void *opaque, hwaddr offset,
> n = s->rx_fifo[0];
> else
> n = s->packet_num;
> - p = s->ptr & 0x07ff;
> + p = s->ptr;
> if (s->ptr & 0x4000) {
> s->ptr = (s->ptr & 0xf800) | ((s->ptr + 1) & 0x7ff);
> } else {
> p += (offset & 3);
> }
> - s->data[n][p] = value;
> + p &= 0x07ff;
> + if (s->allocated < NUM_PACKETS
> + && n < NUM_PACKETS && n & s->allocated) {
This condition makes no sense. s->allocated is a bitmap, so
checking whether it is < NUM_PACKETS doesn't do anything
useful.
> + s->data[n][p] = value;
> + }
> }
> return;
> case 12: /* Interrupt ACK. */
> @@ -517,7 +527,8 @@ static uint32_t smc91c111_readb(void *opaque, hwaddr offset)
> int i;
> int n;
> n = 0;
> - for (i = 0; i < NUM_PACKETS; i++) {
> + for (i = 0;
> + s->allocated < NUM_PACKETS && i < NUM_PACKETS; i++) {
This isn't doing anything sensible either.
> if (s->allocated & (1 << i))
> n++;
> }
> @@ -558,9 +569,9 @@ static uint32_t smc91c111_readb(void *opaque, hwaddr offset)
> case 0: case 1: /* MMUCR Busy bit. */
> return 0;
> case 2: /* Packet Number. */
> - return s->packet_num;
> + return s->packet_num & 0x3F;
No point masking on register-reads.
> case 3: /* Allocation Result. */
> - return s->tx_alloc;
> + return s->tx_alloc < NUM_PACKETS ? s->tx_alloc : 0;
> case 4: /* TX FIFO */
> if (s->tx_fifo_done_len == 0)
> return 0x80;
> @@ -584,13 +595,18 @@ static uint32_t smc91c111_readb(void *opaque, hwaddr offset)
> n = s->rx_fifo[0];
> else
> n = s->packet_num;
> - p = s->ptr & 0x07ff;
> + p = s->ptr;
> if (s->ptr & 0x4000) {
> s->ptr = (s->ptr & 0xf800) | ((s->ptr + 1) & 0x07ff);
> } else {
> p += (offset & 3);
> }
> - return s->data[n][p];
> + p &= 0x07ff;
> + if (s->allocated < NUM_PACKETS
> + && n < NUM_PACKETS && n & s->allocated) {
> + return s->data[n][p];
> + }
> + return 0;
> }
> case 12: /* Interrupt status. */
> return s->int_level;
> --
> 2.7.4
thanks
-- PMM
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-10-27 14:55 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-26 20:02 [Qemu-arm] [PATCH v3] net: smc91c111: check packet number and data register index P J P
2016-10-27 14:47 ` [Qemu-devel] " 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).