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

[Pavel Machek <pavel@denx.de>]

[-- 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 --]