netdev - Re: [PATCH bpf] lwt_bpf: fix crash when using bpf_skb_set_tunnel_key() from bpf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 22 Apr 2022 17:48:47 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Eyal Birger <eyal.birger@...il.com>, davem@...emloft.net,
        kuba@...nel.org, pabeni@...hat.com, ast@...nel.org,
        andrii@...nel.org
Cc:     kafai@...com, songliubraving@...com, yhs@...com,
        john.fastabend@...il.com, kpsingh@...nel.org, mkl@...gutronix.de,
        tgraf@...g.ch, shmulik.ladkani@...il.com, netdev@...r.kernel.org,
        bpf@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH bpf] lwt_bpf: fix crash when using
 bpf_skb_set_tunnel_key() from bpf_xmit lwt hook

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@...r.kernel.org
> Fixes: commit 3a0af8fd61f9 ("bpf: BPF for lightweight tunnel infrastructure")
> Signed-off-by: Eyal Birger <eyal.birger@...il.com>
> ---
>   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