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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190803214930.GB22416@amd>
Date:   Sat, 3 Aug 2019 23:49:30 +0200
From:   Pavel Machek <pavel@...x.de>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Jason Wang <jasowang@...hat.com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Jack Wang <jinpu.wang@...ud.ionos.com>
Subject: Re: [PATCH 4.19 21/32] vhost_net: fix possible infinite loop

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

Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)