db34050
From: Laszlo Ersek <lersek@redhat.com>
db34050
Date: Tue, 19 Jan 2016 14:17:20 +0100
db34050
Subject: [PATCH] e1000: eliminate infinite loops on out-of-bounds transfer
db34050
 start
db34050
db34050
The start_xmit() and e1000_receive_iov() functions implement DMA transfers
db34050
iterating over a set of descriptors that the guest's e1000 driver
db34050
prepares:
db34050
db34050
- the TDLEN and RDLEN registers store the total size of the descriptor
db34050
  area,
db34050
db34050
- while the TDH and RDH registers store the offset (in whole tx / rx
db34050
  descriptors) into the area where the transfer is supposed to start.
db34050
db34050
Each time a descriptor is processed, the TDH and RDH register is bumped
db34050
(as appropriate for the transfer direction).
db34050
db34050
QEMU already contains logic to deal with bogus transfers submitted by the
db34050
guest:
db34050
db34050
- Normally, the transmit case wants to increase TDH from its initial value
db34050
  to TDT. (TDT is allowed to be numerically smaller than the initial TDH
db34050
  value; wrapping at or above TDLEN bytes to zero is normal.) The failsafe
db34050
  that QEMU currently has here is a check against reaching the original
db34050
  TDH value again -- a complete wraparound, which should never happen.
db34050
db34050
- In the receive case RDH is increased from its initial value until
db34050
  "total_size" bytes have been received; preferably in a single step, or
db34050
  in "s->rxbuf_size" byte steps, if the latter is smaller. However, null
db34050
  RX descriptors are skipped without receiving data, while RDH is
db34050
  incremented just the same. QEMU tries to prevent an infinite loop
db34050
  (processing only null RX descriptors) by detecting whether RDH assumes
db34050
  its original value during the loop. (Again, wrapping from RDLEN to 0 is
db34050
  normal.)
db34050
db34050
What both directions miss is that the guest could program TDLEN and RDLEN
db34050
so low, and the initial TDH and RDH so high, that these registers will
db34050
immediately be truncated to zero, and then never reassume their initial
db34050
values in the loop -- a full wraparound will never occur.
db34050
db34050
The condition that expresses this is:
db34050
db34050
  xdh_start >= s->mac_reg[XDLEN] / sizeof(desc)
db34050
db34050
i.e., TDH or RDH start out after the last whole rx or tx descriptor that
db34050
fits into the TDLEN or RDLEN sized area.
db34050
db34050
This condition could be checked before we enter the loops, but
db34050
pci_dma_read() / pci_dma_write() knows how to fill in buffers safely for
db34050
bogus DMA addresses, so we just extend the existing failsafes with the
db34050
above condition.
db34050
db34050
This is CVE-2016-1981.
db34050
db34050
Cc: "Michael S. Tsirkin" <mst@redhat.com>
db34050
Cc: Petr Matousek <pmatouse@redhat.com>
db34050
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
db34050
Cc: Prasad Pandit <ppandit@redhat.com>
db34050
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
db34050
Cc: Jason Wang <jasowang@redhat.com>
db34050
Cc: qemu-stable@nongnu.org
db34050
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1296044
db34050
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
db34050
Reviewed-by: Jason Wang <jasowang@redhat.com>
db34050
Signed-off-by: Jason Wang <jasowang@redhat.com>
db34050
(cherry picked from commit dd793a74882477ca38d49e191110c17dfee51dcc)
db34050
---
db34050
 hw/net/e1000.c | 6 ++++--
db34050
 1 file changed, 4 insertions(+), 2 deletions(-)
db34050
db34050
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
db34050
index f02b9ce..a365357 100644
db34050
--- a/hw/net/e1000.c
db34050
+++ b/hw/net/e1000.c
db34050
@@ -816,7 +816,8 @@ start_xmit(E1000State *s)
db34050
          * bogus values to TDT/TDLEN.
db34050
          * there's nothing too intelligent we could do about this.
db34050
          */
db34050
-        if (s->mac_reg[TDH] == tdh_start) {
db34050
+        if (s->mac_reg[TDH] == tdh_start ||
db34050
+            tdh_start >= s->mac_reg[TDLEN] / sizeof(desc)) {
db34050
             DBGOUT(TXERR, "TDH wraparound @%x, TDT %x, TDLEN %x\n",
db34050
                    tdh_start, s->mac_reg[TDT], s->mac_reg[TDLEN]);
db34050
             break;
db34050
@@ -1062,7 +1063,8 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
db34050
         if (++s->mac_reg[RDH] * sizeof(desc) >= s->mac_reg[RDLEN])
db34050
             s->mac_reg[RDH] = 0;
db34050
         /* see comment in start_xmit; same here */
db34050
-        if (s->mac_reg[RDH] == rdh_start) {
db34050
+        if (s->mac_reg[RDH] == rdh_start ||
db34050
+            rdh_start >= s->mac_reg[RDLEN] / sizeof(desc)) {
db34050
             DBGOUT(RXERR, "RDH wraparound @%x, RDT %x, RDLEN %x\n",
db34050
                    rdh_start, s->mac_reg[RDT], s->mac_reg[RDLEN]);
db34050
             set_ics(s, 0, E1000_ICS_RXO);