Re: [PATCH 4.19 21/32] vhost_net: fix possible infinite loop

[Jason Wang]

On 2019/8/4 上午5:49, Pavel Machek wrote:
Hi!

This makes it possible to trigger a infinite while..continue loop
through the co-opreation of two VMs like:

1) Malicious VM1 allocate 1 byte rx buffer and try to slow down the
    vhost process as much as possible e.g using indirect descriptors or
    other.
2) Malicious VM2 generate packets to VM1 as fast as possible

Fixing this by checking against weight at the end of RX and TX
loop. This also eliminate other similar cases when:

- userspace is consuming the packets in the meanwhile
- theoretical TOCTOU attack if guest moving avail index back and forth
   to hit the continue after vhost find guest just add new buffers

This addresses CVE-2019-3900.

@@ -551,7 +551,7 @@ static void handle_tx_copy(struct vhost_
        int err;
        int sent_pkts = 0;
  
-	for (;;) {
+       do {
                bool busyloop_intr = false;
  
  		head = get_tx_bufs(net, nvq, &msg, &out, &in, &len,
@@ -592,9 +592,7 @@ static void handle_tx_copy(struct vhost_
                                 err, len);
                if (++nvq->done_idx >= VHOST_NET_BATCH)
                        vhost_net_signal_used(nvq);
-               if (vhost_exceeds_weight(vq, ++sent_pkts, total_len))
-                       break;
-       }
+       } while (likely(!vhost_exceeds_weight(vq, ++sent_pkts, total_len)));
  
  	vhost_net_signal_used(nvq);
  }
So this part does not really change anything, right?


Nope, if you check the loop you can see we used to use "continue" inside 
the loop which may bypass the check:


        head = get_tx_bufs(net, nvq, &msg, &out, &in, &len,
                   &busyloop_intr);
        /* On error, stop handling until the next kick. */
        if (unlikely(head < 0))
            break;
        /* Nothing new?  Wait for eventfd to tell us they refilled. */
        if (head == vq->num) {
            if (unlikely(busyloop_intr)) {
                vhost_poll_queue(&vq->poll);
            } else if (unlikely(vhost_enable_notify(&net->dev,
                                vq))) {
                vhost_disable_notify(&net->dev, vq);
                continue;
            }
            break;
        }



@@ -618,7 +616,7 @@ static void handle_tx_zerocopy(struct vh
        bool zcopy_used;
        int sent_pkts = 0;
  
-	for (;;) {
+       do {
                bool busyloop_intr;
  
  		/* Release DMAs done buffers first */
@@ -693,10 +691,7 @@ static void handle_tx_zerocopy(struct vh
                else
                        vhost_zerocopy_signal_used(net, vq);
                vhost_net_tx_packet(net);
-               if (unlikely(vhost_exceeds_weight(vq, ++sent_pkts,
-                                                 total_len)))
-                       break;
-       }
+       } while (likely(!vhost_exceeds_weight(vq, ++sent_pkts, total_len)));
  }
  
  /* Expects to be always run from workqueue - which acts as
Neither does this. Equivalent code. Changelog says it fixes something
for the transmit so... is that intentional?

                                                                        Pavel


The same as above. So yes.

Thanks