f7153f
------------------------------------------------------------------------
f7153f
*From*: 	Laszlo Ersek
f7153f
*Subject*: 	[Qemu-devel] [PATCH] e1000: eliminate infinite loops on
f7153f
out-of-bounds transfer start
f7153f
*Date*: 	Tue, 19 Jan 2016 14:17:20 +0100
f7153f
f7153f
------------------------------------------------------------------------
f7153f
f7153f
The start_xmit() and e1000_receive_iov() functions implement DMA transfers
f7153f
iterating over a set of descriptors that the guest's e1000 driver
f7153f
prepares:
f7153f
f7153f
- the TDLEN and RDLEN registers store the total size of the descriptor
f7153f
  area,
f7153f
f7153f
- while the TDH and RDH registers store the offset (in whole tx / rx
f7153f
  descriptors) into the area where the transfer is supposed to start.
f7153f
f7153f
Each time a descriptor is processed, the TDH and RDH register is bumped
f7153f
(as appropriate for the transfer direction).
f7153f
f7153f
QEMU already contains logic to deal with bogus transfers submitted by the
f7153f
guest:
f7153f
f7153f
- Normally, the transmit case wants to increase TDH from its initial value
f7153f
  to TDT. (TDT is allowed to be numerically smaller than the initial TDH
f7153f
  value; wrapping at or above TDLEN bytes to zero is normal.) The failsafe
f7153f
  that QEMU currently has here is a check against reaching the original
f7153f
  TDH value again -- a complete wraparound, which should never happen.
f7153f
f7153f
- In the receive case RDH is increased from its initial value until
f7153f
  "total_size" bytes have been received; preferably in a single step, or
f7153f
  in "s->rxbuf_size" byte steps, if the latter is smaller. However, null
f7153f
  RX descriptors are skipped without receiving data, while RDH is
f7153f
  incremented just the same. QEMU tries to prevent an infinite loop
f7153f
  (processing only null RX descriptors) by detecting whether RDH assumes
f7153f
  its original value during the loop. (Again, wrapping from RDLEN to 0 is
f7153f
  normal.)
f7153f
f7153f
What both directions miss is that the guest could program TDLEN and RDLEN
f7153f
so low, and the initial TDH and RDH so high, that these registers will
f7153f
immediately be truncated to zero, and then never reassume their initial
f7153f
values in the loop -- a full wraparound will never occur.
f7153f
f7153f
The condition that expresses this is:
f7153f
f7153f
  xdh_start >= s->mac_reg[XDLEN] / sizeof(desc)
f7153f
f7153f
i.e., TDH or RDH start out after the last whole rx or tx descriptor that
f7153f
fits into the TDLEN or RDLEN sized area.
f7153f
f7153f
This condition could be checked before we enter the loops, but
f7153f
pci_dma_read() / pci_dma_write() knows how to fill in buffers safely for
f7153f
bogus DMA addresses, so we just extend the existing failsafes with the
f7153f
above condition.
f7153f
f7153f
Cc: "Michael S. Tsirkin" <address@hidden>
f7153f
Cc: Petr Matousek <address@hidden>
f7153f
Cc: Stefano Stabellini <address@hidden>
f7153f
Cc: Prasad Pandit <address@hidden>
f7153f
Cc: Michael Roth <address@hidden>
f7153f
Cc: Jason Wang <address@hidden>
f7153f
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1296044
f7153f
Signed-off-by: Laszlo Ersek <address@hidden>
f7153f
Reviewed-by: Jason Wang <address@hidden>
f7153f
---
f7153f
f7153f
Notes:
f7153f
    Regarding the public posting: we made an honest effort to vet this
f7153f
    vulnerability, and the impact seems low -- no host side reads/writes,
f7153f
    "just" a DoS (infinite loop). We decided the patch could be posted
f7153f
    publicly, for the usual review process. Jason and Prasad checked the
f7153f
    patch in the internal discussion already, but comments, improvements
f7153f
    etc. are clearly welcome. The CVE request is underway. Thanks.
f7153f
f7153f
 hw/net/e1000.c | 6 ++++--
f7153f
 1 file changed, 4 insertions(+), 2 deletions(-)
f7153f
f7153f
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
f7153f
index bec06e9..34d0823 100644
f7153f
--- a/tools/qemu-xen-traditional/hw/e1000.c
f7153f
+++ b/tools/qemu-xen-traditional/hw/e1000.c
f7153f
@@ -908,7 +908,8 @@ start_xmit(E1000State *s)
f7153f
          * bogus values to TDT/TDLEN.
f7153f
          * there's nothing too intelligent we could do about this.
f7153f
          */
f7153f
-        if (s->mac_reg[TDH] == tdh_start) {
f7153f
+        if (s->mac_reg[TDH] == tdh_start ||
f7153f
+            tdh_start >= s->mac_reg[TDLEN] / sizeof(desc)) {
f7153f
             DBGOUT(TXERR, "TDH wraparound @%x, TDT %x, TDLEN %x\n",
f7153f
                    tdh_start, s->mac_reg[TDT], s->mac_reg[TDLEN]);
f7153f
             break;
f7153f
@@ -1165,7 +1166,8 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
f7153f
             s->mac_reg[RDH] = 0;
f7153f
         s->check_rxov = 1;
f7153f
         /* see comment in start_xmit; same here */
f7153f
-        if (s->mac_reg[RDH] == rdh_start) {
f7153f
+        if (s->mac_reg[RDH] == rdh_start ||
f7153f
+            rdh_start >= s->mac_reg[RDLEN] / sizeof(desc)) {
f7153f
             DBGOUT(RXERR, "RDH wraparound @%x, RDT %x, RDLEN %x\n",
f7153f
                    rdh_start, s->mac_reg[RDT], s->mac_reg[RDLEN]);
f7153f
             set_ics(s, 0, E1000_ICS_RXO);
f7153f
-- 
f7153f
1.8.3.1