[PATCH v3] virtio-net: Add validation for used length

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

Signed-off-by: Xie Yongji <redacted>
---
 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

在 2021/5/28 下午8:11, Xie Yongji 写道:

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

On Mon, May 31, 2021 at 2:49 PM Jason Wang [off-list ref] wrote:

在 2021/5/28 下午8:11, Xie Yongji 写道:

quoted

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

Signed-off-by: Xie Yongji <redacted>
---
  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

在 2021/5/31 下午3:19, Yongji Xie 写道:

On Mon, May 31, 2021 at 2:49 PM Jason Wang [off-list ref] wrote:

quoted

在 2021/5/28 下午8:11, Xie Yongji 写道:

quoted

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

Signed-off-by: Xie Yongji <redacted>
---
   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

On Mon, May 31, 2021 at 3:51 PM Jason Wang [off-list ref] wrote:

在 2021/5/31 下午3:19, Yongji Xie 写道:

quoted

On Mon, May 31, 2021 at 2:49 PM Jason Wang [off-list ref] wrote:

quoted

在 2021/5/28 下午8:11, Xie Yongji 写道:

quoted

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

Signed-off-by: Xie Yongji <redacted>
---
   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