On 26 October 2016 at 21:02, P J P <ppan...@redhat.com> wrote:
> From: Prasad J Pandit <p...@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 <azurey...@tencent.com>
> Signed-off-by: Prasad J Pandit <p...@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
