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

Pavel Machek
2019-08-03T21:49:35Z

[-- Attachment #1: Type: text/plain, Size: 2265 bytes --]

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?

> @@ -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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]