* [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow
@ 2023-01-31 8:50 Heng Qi
2023-02-02 6:20 ` patchwork-bot+netdevbpf
2023-02-02 8:07 ` Michael S. Tsirkin
0 siblings, 2 replies; 8+ messages in thread
From: Heng Qi @ 2023-01-31 8:50 UTC (permalink / raw)
To: netdev, bpf
Cc: Michael S . Tsirkin, Jason Wang, Paolo Abeni, Jakub Kicinski,
John Fastabend, David S . Miller, Daniel Borkmann,
Alexei Starovoitov, Eric Dumazet, Xuan Zhuo
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow
2023-01-31 8:50 [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow Heng Qi
@ 2023-02-02 6:20 ` patchwork-bot+netdevbpf
2023-02-02 8:07 ` Michael S. Tsirkin
1 sibling, 0 replies; 8+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-02-02 6:20 UTC (permalink / raw)
To: Heng Qi
Cc: netdev, bpf, mst, jasowang, pabeni, kuba, john.fastabend, davem,
daniel, ast, edumazet, xuanzhuo
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow
2023-01-31 8:50 [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow Heng Qi
2023-02-02 6:20 ` patchwork-bot+netdevbpf
@ 2023-02-02 8:07 ` Michael S. Tsirkin
2023-02-02 8:14 ` Heng Qi
1 sibling, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2023-02-02 8:07 UTC (permalink / raw)
To: Heng Qi
Cc: netdev, bpf, Jason Wang, Paolo Abeni, Jakub Kicinski,
John Fastabend, David S . Miller, Daniel Borkmann,
Alexei Starovoitov, Eric Dumazet, Xuan Zhuo
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow
2023-02-02 8:07 ` Michael S. Tsirkin
@ 2023-02-02 8:14 ` Heng Qi
2023-02-02 8:16 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Heng Qi @ 2023-02-02 8:14 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, bpf, Jason Wang, Paolo Abeni, Jakub Kicinski,
John Fastabend, David S . Miller, Daniel Borkmann,
Alexei Starovoitov, Eric Dumazet, Xuan Zhuo
在 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow
2023-02-02 8:14 ` Heng Qi
@ 2023-02-02 8:16 ` Michael S. Tsirkin
2023-02-02 9:07 ` Heng Qi
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2023-02-02 8:16 UTC (permalink / raw)
To: Heng Qi
Cc: netdev, bpf, Jason Wang, Paolo Abeni, Jakub Kicinski,
John Fastabend, David S . Miller, Daniel Borkmann,
Alexei Starovoitov, Eric Dumazet, Xuan Zhuo
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow
2023-02-02 8:16 ` Michael S. Tsirkin
@ 2023-02-02 9:07 ` Heng Qi
2023-02-02 10:31 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Heng Qi @ 2023-02-02 9:07 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, bpf, Jason Wang, Paolo Abeni, Jakub Kicinski,
John Fastabend, David S . Miller, Daniel Borkmann,
Alexei Starovoitov, Eric Dumazet, Xuan Zhuo
在 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow
2023-02-02 9:07 ` Heng Qi
@ 2023-02-02 10:31 ` Michael S. Tsirkin
2023-02-02 10:55 ` Heng Qi
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2023-02-02 10:31 UTC (permalink / raw)
To: Heng Qi
Cc: netdev, bpf, Jason Wang, Paolo Abeni, Jakub Kicinski,
John Fastabend, David S . Miller, Daniel Borkmann,
Alexei Starovoitov, Eric Dumazet, Xuan Zhuo
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow
2023-02-02 10:31 ` Michael S. Tsirkin
@ 2023-02-02 10:55 ` Heng Qi
0 siblings, 0 replies; 8+ messages in thread
From: Heng Qi @ 2023-02-02 10:55 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, bpf, Jason Wang, Paolo Abeni, Jakub Kicinski,
John Fastabend, David S . Miller, Daniel Borkmann,
Alexei Starovoitov, Eric Dumazet, Xuan Zhuo
在 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-02-02 10:55 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-31 8:50 [PATCH net-next v2] virtio-net: fix possible unsigned integer overflow Heng Qi
2023-02-02 6:20 ` patchwork-bot+netdevbpf
2023-02-02 8:07 ` Michael S. Tsirkin
2023-02-02 8:14 ` Heng Qi
2023-02-02 8:16 ` Michael S. Tsirkin
2023-02-02 9:07 ` Heng Qi
2023-02-02 10:31 ` Michael S. Tsirkin
2023-02-02 10:55 ` Heng Qi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.