[PATCH net-next v2] virtio-net: fix possible unsigned integer overflow

[PATCH net-next v2] virtio-net: fix possible unsigned integer overflow

[Heng Qi]

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 <hengqi@linux.alibaba.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
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
@@ -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

Re: [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 31 Jan 2023 16:50:04 +0800 you 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 <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> [...]

Here is the summary with links:
  - [net-next,v2] virtio-net: fix possible unsigned integer overflow
    https://git.kernel.org/netdev/net-next/c/981f14d42a7f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow

[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 <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 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.

> @@ -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

Re: [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow

[Heng Qi]

在 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 <hengqi@linux.alibaba.com>
>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> 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.

>
>> @@ -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

Re: [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow

[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 <hengqi@linux.alibaba.com>
> > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > 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?

> > 
> > > @@ -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

Re: [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow

[Heng Qi]

在 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 <hengqi@linux.alibaba.com>
>>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>> 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.

>
>>>> @@ -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

Re: [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow

[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 <hengqi@linux.alibaba.com>
> > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > > 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.

> > 
> > > > > @@ -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

Re: [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow

[Heng Qi]

在 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 <hengqi@linux.alibaba.com>
>>>>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>> ---
>>>>>> 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