syzbot reported a warning in skb_network_header_len() triggered by tcf_skbmod_act(): !skb_transport_header_was_set(skb) WARNING: CPU: 0 PID: 14949 at include/linux/skbuff.h:3243 skb_network_header_len include/linux/skbuff.h:3243 [inline] WARNING: CPU: 0 PID: 14949 at net/sched/act_skbmod.c:55 tcf_skbmod_act+0xfe8/0x1810 net/sched/act_skbmod.c:55 There are a few issues in tcf_skbmod_act(): 1. Calling skb_network_header_len() assumes skb->transport_header is set, which is not guaranteed when tcf_skbmod_act() runs at TC ingress. 2. Unconditionally calling skb_mac_header_len() at the beginning of tcf_skbmod_act() triggers a warning on L3 devices (e.g. TUN) where the MAC header is unset, evaluating to an underflowed garbage length. 3. On TC ingress, skb->data points to the network header. Adding the MAC header length to the IP header length causes skb_ensure_writable() to request more bytes than the actual IP packet length, dropping valid short packets (e.g. 28-byte UDP/IPv4 packets). Fix these by: - Using skb_network_offset(skb) + sizeof(struct iphdr/ipv6hdr) for SKBMOD_F_ECN so that the required length is correctly calculated on both ingress (offset == 0) and egress (offset == mac_len). - Setting max_edit_len to ETH_HLEN for Ethernet header modifications after validating ARPHRD_ETHER. Fixes: 56af5e749f20 ("net/sched: act_skbmod: Add SKBMOD_F_ECN option support") Reported-by: syzbot+1d56f14f95c0480cfdc9@syzkaller.appspotmail.com Closes: https://lore.kernel.org/netdev/6a8b39c0.dbb3a75c.13dd47.0051.GAE@google.com/T/#u Signed-off-by: Eric Dumazet --- net/sched/act_skbmod.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c index a464b0a3c1b81dba6c28c1141aa38c5c7cad3acb..7579cf1e0ff37c1314d112a6aaeed0a965280834 100644 --- a/net/sched/act_skbmod.c +++ b/net/sched/act_skbmod.c @@ -38,7 +38,6 @@ TC_INDIRECT_SCOPE int tcf_skbmod_act(struct sk_buff *skb, if (unlikely(p->action == TC_ACT_SHOT)) goto drop; - max_edit_len = skb_mac_header_len(skb); flags = p->flags; /* tcf_skbmod_init() guarantees "flags" to be one of the following: @@ -51,14 +50,19 @@ TC_INDIRECT_SCOPE int tcf_skbmod_act(struct sk_buff *skb, if (flags == SKBMOD_F_ECN) { switch (skb_protocol(skb, true)) { case cpu_to_be16(ETH_P_IP): + max_edit_len = sizeof(struct iphdr); + break; case cpu_to_be16(ETH_P_IPV6): - max_edit_len += skb_network_header_len(skb); + max_edit_len = sizeof(struct ipv6hdr); break; default: goto out; } - } else if (!skb->dev || skb->dev->type != ARPHRD_ETHER) { - goto out; + max_edit_len += skb_network_offset(skb); + } else { + if (!skb->dev || skb->dev->type != ARPHRD_ETHER) + goto out; + max_edit_len = ETH_HLEN; } err = skb_ensure_writable(skb, max_edit_len); -- 2.55.0.766.g2966f0265a-goog