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

[Jason Wang <jasowang@redhat.com>]

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