tcf_ct_handle_fragments() runs its header sanity checks before handing
anything to the defragmentation engine:
if (family == NFPROTO_IPV4)
err = tcf_ct_ipv4_is_fragment(skb, &frag);
else
err = tcf_ct_ipv6_is_fragment(skb, &frag);
if (err || !frag)
return err;
tcf_ct_ipv4_is_fragment() returns -EINVAL or -ENOMEM;
tcf_ct_ipv6_is_fragment() adds -EPROTO when ipv6_find_hdr() fails. None of
them frees or queues the skb, so on that path the caller still owns it.
tcf_ct_act() however funnels every non-zero return into the
ownership-transfer exit:
err = tcf_ct_handle_fragments(net, skb, family, p->zone, &defrag);
if (err)
goto out_frag;
...
out_frag:
if (err != -EINPROGRESS)
tcf_action_inc_drop_qstats(&c->common);
return TC_ACT_CONSUMED;
TC_ACT_CONSUMED means the action took ownership of the skb, so no caller
frees it - sch_handle_ingress(), sch_handle_egress() and
tcf_qevent_handle() all deliberately skip the free for that verdict. The
skb is therefore orphaned: one sk_buff plus its data buffer is leaked per
malformed packet, unbounded. Note the drop counter is already incremented
for these errors, so the statistics claim a drop that never happens.
Three different ownership states reach out_frag: today - the skb may be
queued by the defrag engine (-EINPROGRESS), already freed by
nf_ct_handle_fragments(), or still owned by us. Tell the caller which of
those it is, and free the packet ourselves in the last case, which
restores the TC_ACT_SHOT behaviour that predated the Fixes: commit.
Reproduced on v7.2-rc6 with a 54-byte frame carrying a 40-byte IPv6
header with nexthdr = 0 (hop-by-hop) and nothing after it, on a
clsact ingress chain with "action ct". kmemleak reports one leaked
232-byte skbuff_head_cache object plus its 704-byte data buffer per
packet; with this patch it reports none.
Fixes: 3f14b377d01d ("net/sched: act_ct: fix skb leak and crash on ooo frags")
Cc: [email protected] # v6.8+
Assisted-by: Anthropic-Claude-Code:Claude-Opus-5
Signed-off-by: Hyunjung Ko <[email protected]>
---
net/sched/act_ct.c | 29 +++++++++++++++++++++++++----
1 file changed, 25 insertions(+), 4 deletions(-)
v2:
- add a tdc selftest (patch 2/2), as requested by Jamal
- add Assisted-by: tag
- no functional change to the fix itself
v1: https://lore.kernel.org/netdev/[email protected]/
Reproducer needs CONFIG_NET_ACT_CT, plus CONFIG_DEBUG_KMEMLEAK and
kmemleak=on to observe it:
ip link add veth0 type veth peer name veth1
ip link set veth0 up; ip link set veth1 up
tc qdisc add dev veth0 clsact
tc filter add dev veth0 ingress matchall action ct
then inject at veth1 a 54-byte frame: ethertype 0x86DD, a 40-byte IPv6
header with nexthdr = 0 (hop-by-hop) and nothing after it, so
ipv6_find_hdr() fails with -EBADMSG and tcf_ct_ipv6_is_fragment()
returns -EPROTO.
Before, one sk_buff plus its data buffer per packet:
kmemleak: 50 new suspected memory leaks
unreferenced object 0xffff888103ed13c0 (size 232):
kmem_cache_alloc_node_noprof+0x2f1/0x3e0
__alloc_skb+0xe5/0x860
alloc_skb_with_frags+0x82/0x750
sock_alloc_send_pskb+0x658/0x7e0
packet_sendmsg+0x1833/0x4860
__x64_sys_sendto+0xe0/0x1c0
do_syscall_64+0x102/0x5a0
After: kmemleak reports no unreferenced objects.
Note /proc/slabinfo is not a usable check here on a KASAN build -
skbuff_head_cache active_objs still grows because the quarantine holds
the freed objects. kmemleak is the reliable signal.
Patch 2/2 turns the same case into a tdc test, using the clsact drop
counter as the discriminator: before the fix act_ct returns
TC_ACT_CONSUMED, so tc_run() never reaches its TC_ACT_SHOT arm and the
counter stays at zero while the skbs leak.
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index be535a261fa0..e250969c84ac 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -840,8 +840,15 @@ static int tcf_ct_ipv6_is_fragment(struct sk_buff *skb,
bool *frag)
return 0;
}
+/* On error, tells the caller whether it still owns @skb and must free it
+ * itself. @skb is ours only when the header checks below reject the packet
+ * before it is handed to the defragmentation engine; once nf_ct_handle_
+ * fragments() has been called the skb is either queued (-EINPROGRESS) or has
+ * already been freed by it.
+ */
static int tcf_ct_handle_fragments(struct net *net, struct sk_buff *skb,
- u8 family, u16 zone, bool *defrag)
+ u8 family, u16 zone, bool *defrag,
+ bool *skb_is_ours)
{
enum ip_conntrack_info ctinfo;
struct tc_skb_cb cb;
@@ -859,8 +866,12 @@ static int tcf_ct_handle_fragments(struct net *net, struct
sk_buff *skb,
err = tcf_ct_ipv4_is_fragment(skb, &frag);
else
err = tcf_ct_ipv6_is_fragment(skb, &frag);
- if (err || !frag)
+ if (err) {
+ *skb_is_ours = true;
return err;
+ }
+ if (!frag)
+ return 0;
cb = *tc_skb_cb(skb);
err = nf_ct_handle_fragments(net, skb, zone, family, &proto, &cb.mru);
@@ -977,6 +988,7 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const
struct tc_action *a,
int nh_ofs, err, retval;
struct tcf_ct_params *p;
bool add_helper = false;
+ bool skb_is_ours = false;
bool skip_add = false;
bool defrag = false;
struct nf_conn *ct;
@@ -1012,9 +1024,18 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb,
const struct tc_action *a,
*/
nh_ofs = skb_network_offset(skb);
skb_pull_rcsum(skb, nh_ofs);
- err = tcf_ct_handle_fragments(net, skb, family, p->zone, &defrag);
- if (err)
+ err = tcf_ct_handle_fragments(net, skb, family, p->zone, &defrag,
+ &skb_is_ours);
+ if (err) {
+ /* The skb is still ours only when the header checks rejected
+ * it; returning TC_ACT_CONSUMED for such a packet would leak
+ * it, since no caller frees an skb it was told it no longer
+ * owns.
+ */
+ if (skb_is_ours)
+ goto drop;
goto out_frag;
+ }
err = nf_ct_skb_network_trim(skb, family);
if (err)
--
2.43.0
