All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrew Kanner <andrew.kanner@gmail.com>
To: davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
	pabeni@redhat.com, jasowang@redhat.com, netdev@vger.kernel.org,
	brouer@redhat.com, dsahern@gmail.com, jbrouer@redhat.com,
	john.fastabend@gmail.com, linux-kernel@vger.kernel.org
Cc: syzbot+f817490f5bd20541b90a@syzkaller.appspotmail.com,
	linux-kernel-mentees@lists.linuxfoundation.org
Subject: [PATCH v4 1/2] drivers: net: prevent tun_build_skb() to exceed the packet size limit
Date: Wed,  2 Aug 2023 01:07:11 +0300	[thread overview]
Message-ID: <20230801220710.464-1-andrew.kanner@gmail.com> (raw)

Using the syzkaller repro with reduced packet size it was discovered
that XDP_PACKET_HEADROOM is not checked in tun_can_build_skb(),
although pad may be incremented in tun_build_skb(). This may end up
with exceeding the PAGE_SIZE limit in tun_build_skb().

Fixes: 7df13219d757 ("tun: reserve extra headroom only when XDP is set")
Link: https://syzkaller.appspot.com/bug?extid=f817490f5bd20541b90a
Signed-off-by: Andrew Kanner <andrew.kanner@gmail.com>
---

Notes:
    v3 -> v4:
    * fall back to v1, fixing only missing XDP_PACKET_HEADROOM in pad and
      removing bpf_xdp_adjust_tail() check for frame_sz.
    * added rcu read lock, noted by Jason Wang <jasowang@redhat.com> in v1
    * I decided to leave the packet length check in tun_can_build_skb()
      instead of moving to tun_build_skb() suggested by Jason Wang
      <jasowang@redhat.com>. Otherwise extra packets will be dropped
      without falling back to tun_alloc_skb(). And in the discussion of v3
      Jesper Dangaard Brouer <jbrouer@redhat.com> noticed that XDP is ok
      with a higher order pages if it's a contiguous physical memory
      allocation, so falling to tun_alloc_skb() -> do_xdp_generic() should
      be ok.
    
    v2 -> v3:
    * attach the forgotten changelog
    
    v1 -> v2:
    * merged 2 patches in 1, fixing both issues: WARN_ON_ONCE with
      syzkaller repro and missing XDP_PACKET_HEADROOM in pad
    * changed the title and description of the execution path, suggested
      by Jason Wang <jasowang@redhat.com>
    * move the limit check from tun_can_build_skb() to tun_build_skb() to
      remove duplication and locking issue, and also drop the packet in
      case of a failed check - noted by Jason Wang <jasowang@redhat.com>

 drivers/net/tun.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index d75456adc62a..a1d04bc9485f 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1582,6 +1582,9 @@ static void tun_rx_batched(struct tun_struct *tun, struct tun_file *tfile,
 static bool tun_can_build_skb(struct tun_struct *tun, struct tun_file *tfile,
 			      int len, int noblock, bool zerocopy)
 {
+	struct bpf_prog *xdp_prog;
+	int pad = TUN_RX_PAD;
+
 	if ((tun->flags & TUN_TYPE_MASK) != IFF_TAP)
 		return false;
 
@@ -1594,7 +1597,13 @@ static bool tun_can_build_skb(struct tun_struct *tun, struct tun_file *tfile,
 	if (zerocopy)
 		return false;
 
-	if (SKB_DATA_ALIGN(len + TUN_RX_PAD) +
+	rcu_read_lock();
+	xdp_prog = rcu_dereference(tun->xdp_prog);
+	if (xdp_prog)
+		pad += XDP_PACKET_HEADROOM;
+	rcu_read_unlock();
+
+	if (SKB_DATA_ALIGN(len + pad) +
 	    SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) > PAGE_SIZE)
 		return false;
 
-- 
2.39.3

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

WARNING: multiple messages have this Message-ID (diff)
From: Andrew Kanner <andrew.kanner@gmail.com>
To: davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
	pabeni@redhat.com, jasowang@redhat.com, netdev@vger.kernel.org,
	brouer@redhat.com, dsahern@gmail.com, jbrouer@redhat.com,
	john.fastabend@gmail.com, linux-kernel@vger.kernel.org
Cc: linux-kernel-mentees@lists.linuxfoundation.org,
	syzbot+f817490f5bd20541b90a@syzkaller.appspotmail.com,
	Andrew Kanner <andrew.kanner@gmail.com>
Subject: [PATCH v4 1/2] drivers: net: prevent tun_build_skb() to exceed the packet size limit
Date: Wed,  2 Aug 2023 01:07:11 +0300	[thread overview]
Message-ID: <20230801220710.464-1-andrew.kanner@gmail.com> (raw)

Using the syzkaller repro with reduced packet size it was discovered
that XDP_PACKET_HEADROOM is not checked in tun_can_build_skb(),
although pad may be incremented in tun_build_skb(). This may end up
with exceeding the PAGE_SIZE limit in tun_build_skb().

Fixes: 7df13219d757 ("tun: reserve extra headroom only when XDP is set")
Link: https://syzkaller.appspot.com/bug?extid=f817490f5bd20541b90a
Signed-off-by: Andrew Kanner <andrew.kanner@gmail.com>
---

Notes:
    v3 -> v4:
    * fall back to v1, fixing only missing XDP_PACKET_HEADROOM in pad and
      removing bpf_xdp_adjust_tail() check for frame_sz.
    * added rcu read lock, noted by Jason Wang <jasowang@redhat.com> in v1
    * I decided to leave the packet length check in tun_can_build_skb()
      instead of moving to tun_build_skb() suggested by Jason Wang
      <jasowang@redhat.com>. Otherwise extra packets will be dropped
      without falling back to tun_alloc_skb(). And in the discussion of v3
      Jesper Dangaard Brouer <jbrouer@redhat.com> noticed that XDP is ok
      with a higher order pages if it's a contiguous physical memory
      allocation, so falling to tun_alloc_skb() -> do_xdp_generic() should
      be ok.
    
    v2 -> v3:
    * attach the forgotten changelog
    
    v1 -> v2:
    * merged 2 patches in 1, fixing both issues: WARN_ON_ONCE with
      syzkaller repro and missing XDP_PACKET_HEADROOM in pad
    * changed the title and description of the execution path, suggested
      by Jason Wang <jasowang@redhat.com>
    * move the limit check from tun_can_build_skb() to tun_build_skb() to
      remove duplication and locking issue, and also drop the packet in
      case of a failed check - noted by Jason Wang <jasowang@redhat.com>

 drivers/net/tun.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index d75456adc62a..a1d04bc9485f 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1582,6 +1582,9 @@ static void tun_rx_batched(struct tun_struct *tun, struct tun_file *tfile,
 static bool tun_can_build_skb(struct tun_struct *tun, struct tun_file *tfile,
 			      int len, int noblock, bool zerocopy)
 {
+	struct bpf_prog *xdp_prog;
+	int pad = TUN_RX_PAD;
+
 	if ((tun->flags & TUN_TYPE_MASK) != IFF_TAP)
 		return false;
 
@@ -1594,7 +1597,13 @@ static bool tun_can_build_skb(struct tun_struct *tun, struct tun_file *tfile,
 	if (zerocopy)
 		return false;
 
-	if (SKB_DATA_ALIGN(len + TUN_RX_PAD) +
+	rcu_read_lock();
+	xdp_prog = rcu_dereference(tun->xdp_prog);
+	if (xdp_prog)
+		pad += XDP_PACKET_HEADROOM;
+	rcu_read_unlock();
+
+	if (SKB_DATA_ALIGN(len + pad) +
 	    SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) > PAGE_SIZE)
 		return false;
 
-- 
2.39.3


             reply	other threads:[~2023-08-01 22:08 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-01 22:07 Andrew Kanner [this message]
2023-08-01 22:07 ` [PATCH v4 1/2] drivers: net: prevent tun_build_skb() to exceed the packet size limit Andrew Kanner
2023-08-01 22:07 ` [PATCH v4 2/2] net: core: remove unnecessary frame_sz check in bpf_xdp_adjust_tail() Andrew Kanner
2023-08-01 22:07   ` Andrew Kanner
2023-08-02 13:56   ` Jesper Dangaard Brouer
2023-08-02 13:56     ` Jesper Dangaard Brouer
2023-08-03  3:20   ` Jason Wang
2023-08-03  3:20     ` Jason Wang
2023-08-03 18:06     ` Andrew Kanner
2023-08-03 18:06       ` Andrew Kanner
2023-08-02  1:07 ` [PATCH v4 1/2] drivers: net: prevent tun_build_skb() to exceed the packet size limit David Ahern
2023-08-02  1:07   ` David Ahern
2023-08-02 12:16   ` Andrew Kanner
2023-08-02 12:16     ` Andrew Kanner
2023-08-02 14:13 ` Jesper Dangaard Brouer
2023-08-02 14:13   ` Jesper Dangaard Brouer
2023-08-03  3:19   ` Jason Wang
2023-08-03  3:19     ` Jason Wang
2023-08-03 17:53     ` Andrew Kanner
2023-08-03 17:53       ` Andrew Kanner
2023-08-04  9:30       ` Jesper Dangaard Brouer
2023-08-04  9:30         ` Jesper Dangaard Brouer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230801220710.464-1-andrew.kanner@gmail.com \
    --to=andrew.kanner@gmail.com \
    --cc=brouer@redhat.com \
    --cc=davem@davemloft.net \
    --cc=dsahern@gmail.com \
    --cc=edumazet@google.com \
    --cc=jasowang@redhat.com \
    --cc=jbrouer@redhat.com \
    --cc=john.fastabend@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=syzbot+f817490f5bd20541b90a@syzkaller.appspotmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.