| Message ID | 20241217030751.11226-1-mazin@getstate.dev (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ip6_tunnel: Fix uninit-value in ip6_tnl_xmit | expand |
On Tue, Dec 17, 2024 at 4:09 AM Mazin Al Haddad <mazin@getstate.dev> wrote: > > When taking the branch with skb_realloc_headroom, pskb_expand_head is > called, as such, pointers referencing content within the new skb's header > are invalid. Currently, the assignment of hop_limit accesses the now > invalid pointer in the network header of this "new" skb. Fix this by > moving the logic to assign hop_limit earlier so that the assignment > references the original un-resized skb instead. Unfortunately this is not fixing anything. If the IPv6 header was in the skb head before skb_realloc_headroom() and/or pskb_expand_head(), it would be copied in the new skb head. Note how the repro is sending a packet with vlan tag (88A8 : ETH_P_8021AD) endto$packet(r0, &(0x7f0000000180)="a6bea8a120e5f8320c30ce5088a8", 0x12, 0x0, &(0x7f0000000140)={0x11, 0x0, r3, 0x1, 0x0, 0x6, @local}, 0x14) Current code, using pskb_inet_may_pull() is not ready yet. My patch has been tested by syzbot and I was about to submit it. diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c index 235808cfec705032b545d6f396f8e58f4693e8d8..c4f0383a136cf5f5e6846293078ec8b826c754c9 100644 --- a/net/ipv6/ip6_gre.c +++ b/net/ipv6/ip6_gre.c @@ -910,7 +910,7 @@ static netdev_tx_t ip6gre_tunnel_xmit(struct sk_buff *skb, __be16 payload_protocol; int ret; - if (!pskb_inet_may_pull(skb)) + if (!skb_vlan_inet_prepare(skb, false)) goto tx_err; if (!ip6_tnl_xmit_ctl(t, &t->parms.laddr, &t->parms.raddr)) @@ -958,7 +958,7 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb, __u32 mtu; int nhoff; - if (!pskb_inet_may_pull(skb)) + if (!skb_vlan_inet_prepare(skb, false)) goto tx_err; if (!ip6_tnl_xmit_ctl(t, &t->parms.laddr, &t->parms.raddr))
Hi Eric, On Tue Dec 17, 2024 at 8:42 AM +03, Eric Dumazet wrote: > On Tue, Dec 17, 2024 at 4:09 AM Mazin Al Haddad <mazin@getstate.dev> wrote: > > > > When taking the branch with skb_realloc_headroom, pskb_expand_head is > > called, as such, pointers referencing content within the new skb's header > > are invalid. Currently, the assignment of hop_limit accesses the now > > invalid pointer in the network header of this "new" skb. Fix this by > > moving the logic to assign hop_limit earlier so that the assignment > > references the original un-resized skb instead. > > Unfortunately this is not fixing anything. > > If the IPv6 header was in the skb head before skb_realloc_headroom() > and/or pskb_expand_head(), > it would be copied in the new skb head. > > Note how the repro is sending a packet with vlan tag (88A8 : ETH_P_8021AD) > > endto$packet(r0, &(0x7f0000000180)="a6bea8a120e5f8320c30ce5088a8", > 0x12, 0x0, &(0x7f0000000140)={0x11, 0x0, r3, 0x1, 0x0, 0x6, @local}, > 0x14) > > Current code, using pskb_inet_may_pull() is not ready yet. > I understand the issue better now. Thank you for taking the time to explain, I appreciate your time and effort. BR, Mazin
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c index 48fd53b98972..62a51f03360d 100644 --- a/net/ipv6/ip6_tunnel.c +++ b/net/ipv6/ip6_tunnel.c @@ -1098,7 +1098,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield, unsigned int max_headroom = psh_hlen; __be16 payload_protocol; bool use_cache = false; - u8 hop_limit; + u8 hop_limit = 0; int err = -1; payload_protocol = skb_protocol(skb, true); @@ -1215,6 +1215,15 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield, skb_scrub_packet(skb, !net_eq(t->net, dev_net(dev))); + if (hop_limit == 0) { + if (payload_protocol == htons(ETH_P_IP)) + hop_limit = ip_hdr(skb)->ttl; + else if (payload_protocol == htons(ETH_P_IPV6)) + hop_limit = ipv6_hdr(skb)->hop_limit; + else + hop_limit = ip6_dst_hoplimit(dst); + } + /* * Okay, now see if we can stuff it in the buffer as-is. */ @@ -1243,15 +1252,6 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield, } skb_dst_set(skb, dst); - if (hop_limit == 0) { - if (payload_protocol == htons(ETH_P_IP)) - hop_limit = ip_hdr(skb)->ttl; - else if (payload_protocol == htons(ETH_P_IPV6)) - hop_limit = ipv6_hdr(skb)->hop_limit; - else - hop_limit = ip6_dst_hoplimit(dst); - } - /* Calculate max headroom for all the headers and adjust * needed_headroom if necessary. */
When taking the branch with skb_realloc_headroom, pskb_expand_head is called, as such, pointers referencing content within the new skb's header are invalid. Currently, the assignment of hop_limit accesses the now invalid pointer in the network header of this "new" skb. Fix this by moving the logic to assign hop_limit earlier so that the assignment references the original un-resized skb instead. uninit-value in ip6table_mangle_hook+0x97d/0x9c0 net/ipv6/netfilter/ip6table_mangle.c:72 ip6t_mangle_out net/ipv6/netfilter/ip6table_mangle.c:56 [inline] ip6table_mangle_hook+0x97d/0x9c0 net/ipv6/netfilter/ip6table_mangle.c:72 nf_hook_entry_hookfn include/linux/netfilter.h:154 [inline] nf_hook_slow+0xf4/0x400 net/netfilter/core.c:626 nf_hook include/linux/netfilter.h:269 [inline] __ip6_local_out+0x5ac/0x640 net/ipv6/output_core.c:143 ip6_local_out+0x4c/0x210 net/ipv6/output_core.c:153 ip6tunnel_xmit+0x129/0x460 include/net/ip6_tunnel.h:161 ip6_tnl_xmit+0x341a/0x3860 net/ipv6/ip6_tunnel.c:1281 Uninit was stored to memory at: ip6_tnl_xmit+0x34f7/0x3860 net/ipv6/ip6_tunnel.c:1277 __gre6_xmit+0x14b9/0x1550 net/ipv6/ip6_gre.c:815 ip6gre_xmit_ipv4 net/ipv6/ip6_gre.c:839 [inline] ip6gre_tunnel_xmit+0x18f7/0x2030 net/ipv6/ip6_gre.c:922 Uninit was created at: slab_post_alloc_hook mm/slub.c:4091 [inline] slab_alloc_node mm/slub.c:4134 [inline] __do_kmalloc_node mm/slub.c:4263 [inline] __kmalloc_node_track_caller_noprof+0x6c7/0xf90 mm/slub.c:4283 kmalloc_reserve+0x23e/0x4a0 net/core/skbuff.c:609 pskb_expand_head+0x226/0x1a60 net/core/skbuff.c:2275 skb_realloc_headroom+0x140/0x2b0 net/core/skbuff.c:2355 ip6_tnl_xmit+0x2106/0x3860 net/ipv6/ip6_tunnel.c:1227 __gre6_xmit+0x14b9/0x1550 net/ipv6/ip6_gre.c:815 ip6gre_xmit_ipv4 net/ipv6/ip6_gre.c:839 [inline] ip6gre_tunnel_xmit+0x18f7/0x2030 net/ipv6/ip6_gre.c:922 Reported-by: syzbot+6023ea32e206eef7920a@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=6023ea32e206eef7920a Signed-off-by: Mazin Al Haddad <mazin@getstate.dev> --- net/ipv6/ip6_tunnel.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)