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