Re: [Qemu-devel] [PATCH V4 for 3.1 1/4] net: drop too large packet early

[Jason Wang <jasowang@redhat.com>]

On 2018/12/4 上午2:13, Thomas Huth wrote:
> On 2018-12-03 11:06, Jason Wang wrote:
>> We try to detect and drop too large packet (>INT_MAX) in 1592a9947036
>> ("net: ignore packet size greater than INT_MAX") during packet
>> delivering. Unfortunately, this is not sufficient as we may hit
>> another integer overflow when trying to queue such large packet in
>> qemu_net_queue_append_iov():
>>
>> - size of the allocation may overflow on 32bit
>> - packet->size is integer which may overflow even on 64bit
>>
>> Fixing this by move the check to qemu_sendv_packet_async() which is
>> the entrance of all networking codes and reduce the limit to
>> NET_BUFSIZE to be more conservative.
>>
>> Cc: qemu-stable@nongnu.org
>> Cc: Li Qiang <liq3ea@163.com>
>> Reported-by: Li Qiang <liq3ea@gmail.com>
>> Reviewed-by: Li Qiang <liq3ea@gmail.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   net/net.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
> Since this is a critical patch for rc4, here's a verbose review...
>
>> diff --git a/net/net.c b/net/net.c
>> index 07c194a8f6..affe1877cf 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -712,15 +712,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender,
>>                                   void *opaque)
>>   {
>>       NetClientState *nc = opaque;
>> -    size_t size = iov_size(iov, iovcnt);
>>       int ret;
>>   
>> -    if (size > INT_MAX) {
>> -        return size;
>> -    }
>>   
>>       if (nc->link_down) {
>> -        return size;
>> +        return iov_size(iov, iovcnt);
>>       }
> In case you respin this patch again, please make
> qemu_deliver_packet_iov() "static", so that it is clear that it can not
> be called directly from the outside anymore. And in case there is no
> need to respin, please consider to send a separate patch for 4.0 instead.


Yes, will try to make it for the next version.


>
> Ok, thinking now load about the call chain:
>
> Anyway, qemu_deliver_packet_iov is not directly called from any other
> file currently, so this is ok ...
>
> So let's see how it is used in net.c ... it's only used as paramter
> here: qemu_new_net_queue(qemu_deliver_packet_iov, nc) ...
> qemu_new_net_queue() assigns it to NetQueue->deliver which is only used
> within queue.c.
>
> Functions using that ->deliver function pointer are the static functions
> qemu_net_queue_deliver() and qemu_net_queue_deliver_iov(). First one
> only uses one iov, so I don't think we can overflow the size here.
> Second one is used in turn are used by the public function
> qemu_net_queue_send_iov(). This has two callers, one in net.c in
> qemu_sendv_packet_async() which you guard below ==> OK.
> The other caller is in qemu_netfilter_pass_to_next() in filter.c - and
> this function is called from many more other places ... but as far as I
> can see, these don't call it in a way where the size could overflow.
>
> ==> Removing the check in qemu_deliver_packet_iov() sounds ok to me, if
> it is checked in qemu_sendv_packet_async() instead.


Yes. Let me add more in the commit message.


>
>>       if (nc->receive_disabled) {
>> @@ -745,10 +741,15 @@ ssize_t qemu_sendv_packet_async(NetClientState *sender,
>>                                   NetPacketSent *sent_cb)
>>   {
>>       NetQueue *queue;
>> +    size_t size = iov_size(iov, iovcnt);
>>       int ret;
>>   
>> +    if (size > NET_BUFSIZE) {
>> +        return size;
>> +    }
> It's a little bit unfortunate that the unsigned size will be cast to
> ssize_t, so a very large size could suddenly change sign. But as Eric
> already wrote in his mail, it seems like the callers are either ignoring
> the return value, or just checking for != 0, so it should be ok for now.
>
> To be more consistent, maybe it would be better to always return an
> negative error code here instead?


Yes, and we can do it for 4.0.


>
>>       if (sender->link_down || !sender->peer) {
>> -        return iov_size(iov, iovcnt);
>> +        return size;
>>       }
>>   
>>       /* Let filters handle the packet first */
>>
> I think I'm fine if this patch is applied in its current shape for rc4, so:
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
> ... but please consider some follow-up patches to make
> qemu_deliver_packet_iov() static, and maybe to return an error code in
> qemu_sendv_packet_async() instead.


Yes.


Thanks for the reviewing.