Re: [PATCH bpf] lwt_bpf: fix crash when using bpf_skb_set_tunnel_key() from bpf_xmit lwt hook

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 4/20/22 6:52 PM, Eyal Birger wrote:
xmit_check_hhlen() observes the dst for getting the device hard header
length to make sure a modified packet can fit. When a helper which changes
the dst - such as bpf_skb_set_tunnel_key() - is called as part of the xmit
program the accessed dst is no longer valid.

This leads to the following splat:

  BUG: kernel NULL pointer dereference, address: 00000000000000de
  #PF: supervisor read access in kernel mode
  #PF: error_code(0x0000) - not-present page
  PGD 0 P4D 0
  Oops: 0000 [#1] PREEMPT SMP PTI
  CPU: 0 PID: 798 Comm: ping Not tainted 5.18.0-rc2+ #103
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-2 04/01/2014
  RIP: 0010:bpf_xmit+0xfb/0x17f
  Code: c6 c0 4d cd 8e 48 c7 c7 7d 33 f0 8e e8 42 09 fb ff 48 8b 45 58 48 8b 95 c8 00 00 00 48 2b 95 c0 00 00 00 48 83 e0 fe 48 8b 00 <0f> b7 80 de 00 00 00 39 c2 73 22 29 d0 b9 20 0a 00 00 31 d2 48 89
  RSP: 0018:ffffb148c0bc7b98 EFLAGS: 00010282
  RAX: 0000000000000000 RBX: 0000000000240008 RCX: 0000000000000000
  RDX: 0000000000000010 RSI: 00000000ffffffea RDI: 00000000ffffffff
  RBP: ffff922a828a4e00 R08: ffffffff8f1350e8 R09: 00000000ffffdfff
  R10: ffffffff8f055100 R11: ffffffff8f105100 R12: 0000000000000000
  R13: ffff922a828a4e00 R14: 0000000000000040 R15: 0000000000000000
  FS:  00007f414e8f0080(0000) GS:ffff922afdc00000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 00000000000000de CR3: 0000000002d80006 CR4: 0000000000370ef0
  Call Trace:
   <TASK>
   lwtunnel_xmit.cold+0x71/0xc8
   ip_finish_output2+0x279/0x520
   ? __ip_finish_output.part.0+0x21/0x130

Fix by fetching the device hard header length before running the bpf code.

Cc: stable@xxxxxxxxxxxxxxx
Fixes: commit 3a0af8fd61f9 ("bpf: BPF for lightweight tunnel infrastructure")
Signed-off-by: Eyal Birger <eyal.birger@xxxxxxxxx>
---
  net/core/lwt_bpf.c | 7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/core/lwt_bpf.c b/net/core/lwt_bpf.c
index 349480ef68a5..8b6b5e72b217 100644
--- a/net/core/lwt_bpf.c
+++ b/net/core/lwt_bpf.c
@@ -159,10 +159,8 @@ static int bpf_output(struct net *net, struct sock *sk, struct sk_buff *skb)
  	return dst->lwtstate->orig_output(net, sk, skb);
  }
-static int xmit_check_hhlen(struct sk_buff *skb)
+static int xmit_check_hhlen(struct sk_buff *skb, int hh_len)
  {
-	int hh_len = skb_dst(skb)->dev->hard_header_len;
-
  	if (skb_headroom(skb) < hh_len) {
  		int nhead = HH_DATA_ALIGN(hh_len - skb_headroom(skb));
@@ -274,6 +272,7 @@ static int bpf_xmit(struct sk_buff *skb) bpf = bpf_lwt_lwtunnel(dst->lwtstate);
  	if (bpf->xmit.prog) {
+		int hh_len = dst->dev->hard_header_len;
  		__be16 proto = skb->protocol;
  		int ret;
@@ -291,7 +290,7 @@ static int bpf_xmit(struct sk_buff *skb)
  			/* If the header was expanded, headroom might be too
  			 * small for L2 header to come, expand as needed.
  			 */
-			ret = xmit_check_hhlen(skb);
+			ret = xmit_check_hhlen(skb, hh_len);

Ok, makes sense given for BPF_OK the dst->dev shouldn't change here (e.g. as opposed
to BPF_REDIRECT). Applied, please also follow-up with a BPF selftest for test_progs
so that this won't break in future when it's running as part of BPF CI.

  			if (unlikely(ret))
  				return ret;

Thanks,
Daniel



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux