Re: [Qemu-devel] [PATCH] e1000: eliminate infinite loops on out-of-bounds transfer start

[Jason Wang]

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

Applied in my -net.

Thanks