netdev - Re: [PATCH net] net: clear the dst when changing skb protocol

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]

Message-ID: <20250605062234.1df7e74a@kernel.org>
Date: Thu, 5 Jun 2025 06:22:34 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Maciej Żenczykowski <maze@...gle.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
 pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
 martin.lau@...ux.dev, daniel@...earbox.net, john.fastabend@...il.com,
 eddyz87@...il.com, sdf@...ichev.me, haoluo@...gle.com, willemb@...gle.com,
 william.xuanziyang@...wei.com, alan.maguire@...cle.com, bpf@...r.kernel.org
Subject: Re: [PATCH net] net: clear the dst when changing skb protocol

On Wed, 4 Jun 2025 23:21:02 +0200 Maciej Żenczykowski wrote:
> > @@ -3550,10 +3557,10 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
> >                 /* Match skb->protocol to new outer l3 protocol */
> >                 if (skb->protocol == htons(ETH_P_IP) &&
> >                     flags & BPF_F_ADJ_ROOM_ENCAP_L3_IPV6)
> > -                       skb->protocol = htons(ETH_P_IPV6);
> > +                       bpf_skb_change_protocol(skb, ETH_P_IPV6);
> >                 else if (skb->protocol == htons(ETH_P_IPV6) &&
> >                          flags & BPF_F_ADJ_ROOM_ENCAP_L3_IPV4)
> > -                       skb->protocol = htons(ETH_P_IP);
> > +                       bpf_skb_change_protocol(skb, ETH_P_IP);  
> 
> I wonder if this shouldn't drop dst even when doing ipv4->ipv4 or
> ipv6->ipv6 -- it's encapping, presumably old dst is irrelevant...

I keep going back and forth on this. You definitely have a point, 
but I feel like there are levels to how BPF prog can make the dst
irrelevant:
 - change proto
 - encap
 - adjust room but not set any encap flag
 - overwrite the addrs without calling any helpers
First case we have to cover for safety, last we can't possibly cover.
So the question is whether we should draw the line somewhere in
the middle, or leave this patch as is and if the actual use case arrives
- let BPF call skb_dst_drop() as a kfunc. Right now I'm leaning towards
the latter.

Does that make sense? Does anyone else have an opinion?