All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] virtio-net: Add validation for used length
@ 2021-05-28 12:11 Xie Yongji
  2021-05-31  6:49   ` Jason Wang
  0 siblings, 1 reply; 7+ messages in thread
From: Xie Yongji @ 2021-05-28 12:11 UTC (permalink / raw)
  To: mst, jasowang, kuba; +Cc: virtualization, netdev, linux-kernel

This adds validation for used length (might come
from an untrusted device) to avoid data corruption
or loss.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/net/virtio_net.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 073fec4c0df1..01f15b65824c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -732,6 +732,17 @@ static struct sk_buff *receive_small(struct net_device *dev,
 
 	rcu_read_lock();
 	xdp_prog = rcu_dereference(rq->xdp_prog);
+	if (unlikely(len > GOOD_PACKET_LEN)) {
+		pr_debug("%s: rx error: len %u exceeds max size %d\n",
+			 dev->name, len, GOOD_PACKET_LEN);
+		dev->stats.rx_length_errors++;
+		if (xdp_prog)
+			goto err_xdp;
+
+		rcu_read_unlock();
+		put_page(page);
+		return NULL;
+	}
 	if (xdp_prog) {
 		struct virtio_net_hdr_mrg_rxbuf *hdr = buf + header_offset;
 		struct xdp_frame *xdpf;
@@ -888,6 +899,16 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 
 	rcu_read_lock();
 	xdp_prog = rcu_dereference(rq->xdp_prog);
+	if (unlikely(len > truesize)) {
+		pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
+			 dev->name, len, (unsigned long)ctx);
+		dev->stats.rx_length_errors++;
+		if (xdp_prog)
+			goto err_xdp;
+
+		rcu_read_unlock();
+		goto err_skb;
+	}
 	if (xdp_prog) {
 		struct xdp_frame *xdpf;
 		struct page *xdp_page;
@@ -1012,13 +1033,6 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 	}
 	rcu_read_unlock();
 
-	if (unlikely(len > truesize)) {
-		pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
-			 dev->name, len, (unsigned long)ctx);
-		dev->stats.rx_length_errors++;
-		goto err_skb;
-	}
-
 	head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
 			       metasize, !!headroom);
 	curr_skb = head_skb;
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [PATCH v3] virtio-net: Add validation for used length
  2021-05-28 12:11 [PATCH v3] virtio-net: Add validation for used length Xie Yongji
@ 2021-05-31  6:49   ` Jason Wang
  0 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2021-05-31  6:49 UTC (permalink / raw)
  To: Xie Yongji, mst, kuba; +Cc: virtualization, netdev, linux-kernel


在 2021/5/28 下午8:11, Xie Yongji 写道:
> This adds validation for used length (might come
> from an untrusted device) to avoid data corruption
> or loss.
>
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>   drivers/net/virtio_net.c | 28 +++++++++++++++++++++-------
>   1 file changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 073fec4c0df1..01f15b65824c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -732,6 +732,17 @@ static struct sk_buff *receive_small(struct net_device *dev,
>   
>   	rcu_read_lock();
>   	xdp_prog = rcu_dereference(rq->xdp_prog);
> +	if (unlikely(len > GOOD_PACKET_LEN)) {
> +		pr_debug("%s: rx error: len %u exceeds max size %d\n",
> +			 dev->name, len, GOOD_PACKET_LEN);
> +		dev->stats.rx_length_errors++;
> +		if (xdp_prog)
> +			goto err_xdp;
> +
> +		rcu_read_unlock();
> +		put_page(page);
> +		return NULL;
> +	}
>   	if (xdp_prog) {
>   		struct virtio_net_hdr_mrg_rxbuf *hdr = buf + header_offset;
>   		struct xdp_frame *xdpf;
> @@ -888,6 +899,16 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>   
>   	rcu_read_lock();
>   	xdp_prog = rcu_dereference(rq->xdp_prog);
> +	if (unlikely(len > truesize)) {
> +		pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
> +			 dev->name, len, (unsigned long)ctx);
> +		dev->stats.rx_length_errors++;
> +		if (xdp_prog)
> +			goto err_xdp;
> +
> +		rcu_read_unlock();
> +		goto err_skb;
> +	}


Patch looks correct but I'd rather not bother XDP here. It would be 
better if we just do the check before rcu_read_lock() and use err_skb 
directly() to avoid RCU/XDP stuffs.

Thanks


>   	if (xdp_prog) {
>   		struct xdp_frame *xdpf;
>   		struct page *xdp_page;
> @@ -1012,13 +1033,6 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>   	}
>   	rcu_read_unlock();
>   
> -	if (unlikely(len > truesize)) {
> -		pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
> -			 dev->name, len, (unsigned long)ctx);
> -		dev->stats.rx_length_errors++;
> -		goto err_skb;
> -	}
> -
>   	head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
>   			       metasize, !!headroom);
>   	curr_skb = head_skb;


^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v3] virtio-net: Add validation for used length
@ 2021-05-31  6:49   ` Jason Wang
  0 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2021-05-31  6:49 UTC (permalink / raw)
  To: Xie Yongji, mst, kuba; +Cc: netdev, linux-kernel, virtualization


在 2021/5/28 下午8:11, Xie Yongji 写道:
> This adds validation for used length (might come
> from an untrusted device) to avoid data corruption
> or loss.
>
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>   drivers/net/virtio_net.c | 28 +++++++++++++++++++++-------
>   1 file changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 073fec4c0df1..01f15b65824c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -732,6 +732,17 @@ static struct sk_buff *receive_small(struct net_device *dev,
>   
>   	rcu_read_lock();
>   	xdp_prog = rcu_dereference(rq->xdp_prog);
> +	if (unlikely(len > GOOD_PACKET_LEN)) {
> +		pr_debug("%s: rx error: len %u exceeds max size %d\n",
> +			 dev->name, len, GOOD_PACKET_LEN);
> +		dev->stats.rx_length_errors++;
> +		if (xdp_prog)
> +			goto err_xdp;
> +
> +		rcu_read_unlock();
> +		put_page(page);
> +		return NULL;
> +	}
>   	if (xdp_prog) {
>   		struct virtio_net_hdr_mrg_rxbuf *hdr = buf + header_offset;
>   		struct xdp_frame *xdpf;
> @@ -888,6 +899,16 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>   
>   	rcu_read_lock();
>   	xdp_prog = rcu_dereference(rq->xdp_prog);
> +	if (unlikely(len > truesize)) {
> +		pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
> +			 dev->name, len, (unsigned long)ctx);
> +		dev->stats.rx_length_errors++;
> +		if (xdp_prog)
> +			goto err_xdp;
> +
> +		rcu_read_unlock();
> +		goto err_skb;
> +	}


Patch looks correct but I'd rather not bother XDP here. It would be 
better if we just do the check before rcu_read_lock() and use err_skb 
directly() to avoid RCU/XDP stuffs.

Thanks


>   	if (xdp_prog) {
>   		struct xdp_frame *xdpf;
>   		struct page *xdp_page;
> @@ -1012,13 +1033,6 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>   	}
>   	rcu_read_unlock();
>   
> -	if (unlikely(len > truesize)) {
> -		pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
> -			 dev->name, len, (unsigned long)ctx);
> -		dev->stats.rx_length_errors++;
> -		goto err_skb;
> -	}
> -
>   	head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
>   			       metasize, !!headroom);
>   	curr_skb = head_skb;

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: Re: [PATCH v3] virtio-net: Add validation for used length
  2021-05-31  6:49   ` Jason Wang
  (?)
@ 2021-05-31  7:19   ` Yongji Xie
  2021-05-31  7:51       ` Jason Wang
  -1 siblings, 1 reply; 7+ messages in thread
From: Yongji Xie @ 2021-05-31  7:19 UTC (permalink / raw)
  To: Jason Wang
  Cc: Michael S. Tsirkin, Jakub Kicinski, virtualization, netdev, linux-kernel

On Mon, May 31, 2021 at 2:49 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2021/5/28 下午8:11, Xie Yongji 写道:
> > This adds validation for used length (might come
> > from an untrusted device) to avoid data corruption
> > or loss.
> >
> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > ---
> >   drivers/net/virtio_net.c | 28 +++++++++++++++++++++-------
> >   1 file changed, 21 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 073fec4c0df1..01f15b65824c 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -732,6 +732,17 @@ static struct sk_buff *receive_small(struct net_device *dev,
> >
> >       rcu_read_lock();
> >       xdp_prog = rcu_dereference(rq->xdp_prog);
> > +     if (unlikely(len > GOOD_PACKET_LEN)) {
> > +             pr_debug("%s: rx error: len %u exceeds max size %d\n",
> > +                      dev->name, len, GOOD_PACKET_LEN);
> > +             dev->stats.rx_length_errors++;
> > +             if (xdp_prog)
> > +                     goto err_xdp;
> > +
> > +             rcu_read_unlock();
> > +             put_page(page);
> > +             return NULL;
> > +     }
> >       if (xdp_prog) {
> >               struct virtio_net_hdr_mrg_rxbuf *hdr = buf + header_offset;
> >               struct xdp_frame *xdpf;
> > @@ -888,6 +899,16 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> >
> >       rcu_read_lock();
> >       xdp_prog = rcu_dereference(rq->xdp_prog);
> > +     if (unlikely(len > truesize)) {
> > +             pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
> > +                      dev->name, len, (unsigned long)ctx);
> > +             dev->stats.rx_length_errors++;
> > +             if (xdp_prog)
> > +                     goto err_xdp;
> > +
> > +             rcu_read_unlock();
> > +             goto err_skb;
> > +     }
>
>
> Patch looks correct but I'd rather not bother XDP here. It would be
> better if we just do the check before rcu_read_lock() and use err_skb
> directly() to avoid RCU/XDP stuffs.
>

If so, we will miss the statistics of xdp_drops. Is it OK?

Thanks,
Yongji

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v3] virtio-net: Add validation for used length
  2021-05-31  7:19   ` Yongji Xie
@ 2021-05-31  7:51       ` Jason Wang
  0 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2021-05-31  7:51 UTC (permalink / raw)
  To: Yongji Xie
  Cc: Michael S. Tsirkin, Jakub Kicinski, virtualization, netdev, linux-kernel


在 2021/5/31 下午3:19, Yongji Xie 写道:
> On Mon, May 31, 2021 at 2:49 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> 在 2021/5/28 下午8:11, Xie Yongji 写道:
>>> This adds validation for used length (might come
>>> from an untrusted device) to avoid data corruption
>>> or loss.
>>>
>>> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
>>> ---
>>>    drivers/net/virtio_net.c | 28 +++++++++++++++++++++-------
>>>    1 file changed, 21 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 073fec4c0df1..01f15b65824c 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -732,6 +732,17 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>
>>>        rcu_read_lock();
>>>        xdp_prog = rcu_dereference(rq->xdp_prog);
>>> +     if (unlikely(len > GOOD_PACKET_LEN)) {
>>> +             pr_debug("%s: rx error: len %u exceeds max size %d\n",
>>> +                      dev->name, len, GOOD_PACKET_LEN);
>>> +             dev->stats.rx_length_errors++;
>>> +             if (xdp_prog)
>>> +                     goto err_xdp;
>>> +
>>> +             rcu_read_unlock();
>>> +             put_page(page);
>>> +             return NULL;
>>> +     }
>>>        if (xdp_prog) {
>>>                struct virtio_net_hdr_mrg_rxbuf *hdr = buf + header_offset;
>>>                struct xdp_frame *xdpf;
>>> @@ -888,6 +899,16 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>>>
>>>        rcu_read_lock();
>>>        xdp_prog = rcu_dereference(rq->xdp_prog);
>>> +     if (unlikely(len > truesize)) {
>>> +             pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
>>> +                      dev->name, len, (unsigned long)ctx);
>>> +             dev->stats.rx_length_errors++;
>>> +             if (xdp_prog)
>>> +                     goto err_xdp;
>>> +
>>> +             rcu_read_unlock();
>>> +             goto err_skb;
>>> +     }
>>
>> Patch looks correct but I'd rather not bother XDP here. It would be
>> better if we just do the check before rcu_read_lock() and use err_skb
>> directly() to avoid RCU/XDP stuffs.
>>
> If so, we will miss the statistics of xdp_drops. Is it OK?


It should be ok, we still had drops and it was dropped before dealing 
with XDP.

The motivation is to have simple codes.

Thanks


>
> Thanks,
> Yongji
>


^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v3] virtio-net: Add validation for used length
@ 2021-05-31  7:51       ` Jason Wang
  0 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2021-05-31  7:51 UTC (permalink / raw)
  To: Yongji Xie
  Cc: Jakub Kicinski, virtualization, netdev, linux-kernel, Michael S. Tsirkin


在 2021/5/31 下午3:19, Yongji Xie 写道:
> On Mon, May 31, 2021 at 2:49 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> 在 2021/5/28 下午8:11, Xie Yongji 写道:
>>> This adds validation for used length (might come
>>> from an untrusted device) to avoid data corruption
>>> or loss.
>>>
>>> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
>>> ---
>>>    drivers/net/virtio_net.c | 28 +++++++++++++++++++++-------
>>>    1 file changed, 21 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 073fec4c0df1..01f15b65824c 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -732,6 +732,17 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>
>>>        rcu_read_lock();
>>>        xdp_prog = rcu_dereference(rq->xdp_prog);
>>> +     if (unlikely(len > GOOD_PACKET_LEN)) {
>>> +             pr_debug("%s: rx error: len %u exceeds max size %d\n",
>>> +                      dev->name, len, GOOD_PACKET_LEN);
>>> +             dev->stats.rx_length_errors++;
>>> +             if (xdp_prog)
>>> +                     goto err_xdp;
>>> +
>>> +             rcu_read_unlock();
>>> +             put_page(page);
>>> +             return NULL;
>>> +     }
>>>        if (xdp_prog) {
>>>                struct virtio_net_hdr_mrg_rxbuf *hdr = buf + header_offset;
>>>                struct xdp_frame *xdpf;
>>> @@ -888,6 +899,16 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>>>
>>>        rcu_read_lock();
>>>        xdp_prog = rcu_dereference(rq->xdp_prog);
>>> +     if (unlikely(len > truesize)) {
>>> +             pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
>>> +                      dev->name, len, (unsigned long)ctx);
>>> +             dev->stats.rx_length_errors++;
>>> +             if (xdp_prog)
>>> +                     goto err_xdp;
>>> +
>>> +             rcu_read_unlock();
>>> +             goto err_skb;
>>> +     }
>>
>> Patch looks correct but I'd rather not bother XDP here. It would be
>> better if we just do the check before rcu_read_lock() and use err_skb
>> directly() to avoid RCU/XDP stuffs.
>>
> If so, we will miss the statistics of xdp_drops. Is it OK?


It should be ok, we still had drops and it was dropped before dealing 
with XDP.

The motivation is to have simple codes.

Thanks


>
> Thanks,
> Yongji
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: Re: [PATCH v3] virtio-net: Add validation for used length
  2021-05-31  7:51       ` Jason Wang
  (?)
@ 2021-05-31  8:23       ` Yongji Xie
  -1 siblings, 0 replies; 7+ messages in thread
From: Yongji Xie @ 2021-05-31  8:23 UTC (permalink / raw)
  To: Jason Wang
  Cc: Michael S. Tsirkin, Jakub Kicinski, virtualization, netdev, linux-kernel

On Mon, May 31, 2021 at 3:51 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2021/5/31 下午3:19, Yongji Xie 写道:
> > On Mon, May 31, 2021 at 2:49 PM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> 在 2021/5/28 下午8:11, Xie Yongji 写道:
> >>> This adds validation for used length (might come
> >>> from an untrusted device) to avoid data corruption
> >>> or loss.
> >>>
> >>> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> >>> ---
> >>>    drivers/net/virtio_net.c | 28 +++++++++++++++++++++-------
> >>>    1 file changed, 21 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>> index 073fec4c0df1..01f15b65824c 100644
> >>> --- a/drivers/net/virtio_net.c
> >>> +++ b/drivers/net/virtio_net.c
> >>> @@ -732,6 +732,17 @@ static struct sk_buff *receive_small(struct net_device *dev,
> >>>
> >>>        rcu_read_lock();
> >>>        xdp_prog = rcu_dereference(rq->xdp_prog);
> >>> +     if (unlikely(len > GOOD_PACKET_LEN)) {
> >>> +             pr_debug("%s: rx error: len %u exceeds max size %d\n",
> >>> +                      dev->name, len, GOOD_PACKET_LEN);
> >>> +             dev->stats.rx_length_errors++;
> >>> +             if (xdp_prog)
> >>> +                     goto err_xdp;
> >>> +
> >>> +             rcu_read_unlock();
> >>> +             put_page(page);
> >>> +             return NULL;
> >>> +     }
> >>>        if (xdp_prog) {
> >>>                struct virtio_net_hdr_mrg_rxbuf *hdr = buf + header_offset;
> >>>                struct xdp_frame *xdpf;
> >>> @@ -888,6 +899,16 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> >>>
> >>>        rcu_read_lock();
> >>>        xdp_prog = rcu_dereference(rq->xdp_prog);
> >>> +     if (unlikely(len > truesize)) {
> >>> +             pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
> >>> +                      dev->name, len, (unsigned long)ctx);
> >>> +             dev->stats.rx_length_errors++;
> >>> +             if (xdp_prog)
> >>> +                     goto err_xdp;
> >>> +
> >>> +             rcu_read_unlock();
> >>> +             goto err_skb;
> >>> +     }
> >>
> >> Patch looks correct but I'd rather not bother XDP here. It would be
> >> better if we just do the check before rcu_read_lock() and use err_skb
> >> directly() to avoid RCU/XDP stuffs.
> >>
> > If so, we will miss the statistics of xdp_drops. Is it OK?
>
>
> It should be ok, we still had drops and it was dropped before dealing
> with XDP.
>
> The motivation is to have simple codes.
>

OK, will send v4 soon.

Thanks,
Yongji

^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-05-31  8:24 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-28 12:11 [PATCH v3] virtio-net: Add validation for used length Xie Yongji
2021-05-31  6:49 ` Jason Wang
2021-05-31  6:49   ` Jason Wang
2021-05-31  7:19   ` Yongji Xie
2021-05-31  7:51     ` Jason Wang
2021-05-31  7:51       ` Jason Wang
2021-05-31  8:23       ` Yongji Xie

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.