From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D2337C61DA4 for ; Thu, 2 Feb 2023 10:55:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230036AbjBBKzp (ORCPT ); Thu, 2 Feb 2023 05:55:45 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55518 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230169AbjBBKzp (ORCPT ); Thu, 2 Feb 2023 05:55:45 -0500 Received: from out30-113.freemail.mail.aliyun.com (out30-113.freemail.mail.aliyun.com [115.124.30.113]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 06D022687; Thu, 2 Feb 2023 02:55:39 -0800 (PST) X-Alimail-AntiSpam: AC=PASS;BC=-1|-1;BR=01201311R141e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=ay29a033018046050;MF=hengqi@linux.alibaba.com;NM=1;PH=DS;RN=12;SR=0;TI=SMTPD_---0Vakpwi2_1675335336; Received: from 30.221.147.171(mailfrom:hengqi@linux.alibaba.com fp:SMTPD_---0Vakpwi2_1675335336) by smtp.aliyun-inc.com; Thu, 02 Feb 2023 18:55:37 +0800 Message-ID: Date: Thu, 2 Feb 2023 18:55:34 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:109.0) Gecko/20100101 Thunderbird/109.0 Subject: Re: [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow To: "Michael S. Tsirkin" Cc: netdev@vger.kernel.org, bpf@vger.kernel.org, Jason Wang , Paolo Abeni , Jakub Kicinski , John Fastabend , "David S . Miller" , Daniel Borkmann , Alexei Starovoitov , Eric Dumazet , Xuan Zhuo References: <20230131085004.98687-1-hengqi@linux.alibaba.com> <20230202030550-mutt-send-email-mst@kernel.org> <20230202031609-mutt-send-email-mst@kernel.org> <20230202052930-mutt-send-email-mst@kernel.org> From: Heng Qi In-Reply-To: <20230202052930-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org 在 2023/2/2 下午6:31, Michael S. Tsirkin 写道: > On Thu, Feb 02, 2023 at 05:07:04PM +0800, Heng Qi wrote: >> >> 在 2023/2/2 下午4:16, Michael S. Tsirkin 写道: >>> On Thu, Feb 02, 2023 at 04:14:51PM +0800, Heng Qi wrote: >>>> 在 2023/2/2 下午4:07, Michael S. Tsirkin 写道: >>>>> On Tue, Jan 31, 2023 at 04:50:04PM +0800, Heng Qi wrote: >>>>>> When the single-buffer xdp is loaded and after xdp_linearize_page() >>>>>> is called, *num_buf becomes 0 and (*num_buf - 1) may overflow into >>>>>> a large integer in virtnet_build_xdp_buff_mrg(), resulting in >>>>>> unexpected packet dropping. >>>>>> >>>>>> Fixes: ef75cb51f139 ("virtio-net: build xdp_buff with multi buffers") >>>>>> Signed-off-by: Heng Qi >>>>>> Reviewed-by: Xuan Zhuo >>>>>> Signed-off-by: Michael S. Tsirkin >>>>>> --- >>>>>> v1->v2: >>>>>> - Change the type of num_buf from unsigned int to int. @Michael S . Tsirkin >>>>>> - Some cleaner codes. @Michael S . Tsirkin >>>>>> >>>>>> drivers/net/virtio_net.c | 15 +++++++++------ >>>>>> 1 file changed, 9 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >>>>>> index aaa6fe9b214a..8102861785a2 100644 >>>>>> --- a/drivers/net/virtio_net.c >>>>>> +++ b/drivers/net/virtio_net.c >>>>>> @@ -716,7 +716,7 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi) >>>>>> * have enough headroom. >>>>>> */ >>>>>> static struct page *xdp_linearize_page(struct receive_queue *rq, >>>>>> - u16 *num_buf, >>>>>> + int *num_buf, >>>>>> struct page *p, >>>>>> int offset, >>>>>> int page_off, >>>>>> @@ -816,7 +816,7 @@ static struct sk_buff *receive_small(struct net_device *dev, >>>>>> if (unlikely(xdp_headroom < virtnet_get_headroom(vi))) { >>>>>> int offset = buf - page_address(page) + header_offset; >>>>>> unsigned int tlen = len + vi->hdr_len; >>>>>> - u16 num_buf = 1; >>>>>> + int num_buf = 1; >>>>>> xdp_headroom = virtnet_get_headroom(vi); >>>>>> header_offset = VIRTNET_RX_PAD + xdp_headroom; >>>>>> @@ -989,7 +989,7 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, >>>>>> void *buf, >>>>>> unsigned int len, >>>>>> unsigned int frame_sz, >>>>>> - u16 *num_buf, >>>>>> + int *num_buf, >>>>>> unsigned int *xdp_frags_truesize, >>>>>> struct virtnet_rq_stats *stats) >>>>>> { >>>>>> @@ -1007,6 +1007,9 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, >>>>>> xdp_prepare_buff(xdp, buf - VIRTIO_XDP_HEADROOM, >>>>>> VIRTIO_XDP_HEADROOM + vi->hdr_len, len - vi->hdr_len, true); >>>>>> + if (!*num_buf) >>>>>> + return 0; >>>>>> + >>>>>> if (*num_buf > 1) { >>>>>> /* If we want to build multi-buffer xdp, we need >>>>>> * to specify that the flags of xdp_buff have the >>>>> Ouch. Why is this here? Merged so pls remove by a follow up patch, the >>>>> rest of the code handles 0 fine. I'm not sure this introduces a bug by >>>>> we don't want spaghetti code. >>>> Yes it would work without this, but I was keeping this because I wanted it >>>> to handle 0 early and exit early. >>>> >>>> Do you want to remove this? >>>> >>>> Thanks. >>> why do you want to exit early? >> If num_buf is 0, we don't need to judge the subsequent process, because the >> latter process >> is used to build multi-buffer xdp, but this fix solves the possible problems >> of single-buffer xdp. >> >> Thanks. > An optimization then? As any optimization I'd like to see some numbers. > > Should have been documented as such not as part of a bugfix. Sure, I'm going to remove this with a follow-up patch.😁 Thanks. > >>>>>> @@ -1020,10 +1023,10 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, >>>>>> shinfo->xdp_frags_size = 0; >>>>>> } >>>>>> - if ((*num_buf - 1) > MAX_SKB_FRAGS) >>>>>> + if (*num_buf > MAX_SKB_FRAGS + 1) >>>>>> return -EINVAL; >>>>>> - while ((--*num_buf) >= 1) { >>>>>> + while (--*num_buf > 0) { >>>>>> buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx); >>>>>> if (unlikely(!buf)) { >>>>>> pr_debug("%s: rx error: %d buffers out of %d missing\n", >>>>>> @@ -1076,7 +1079,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, >>>>>> struct virtnet_rq_stats *stats) >>>>>> { >>>>>> struct virtio_net_hdr_mrg_rxbuf *hdr = buf; >>>>>> - u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers); >>>>>> + int num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers); >>>>>> struct page *page = virt_to_head_page(buf); >>>>>> int offset = buf - page_address(page); >>>>>> struct sk_buff *head_skb, *curr_skb; >>>>>> -- >>>>>> 2.19.1.6.gb485710b