mirror of
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
synced 2026-08-28 02:53:16 -04:00
net/sched: act_ct: fix sk_buff leak when the header checks reject a packet
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: 3f14b377d0 ("net/sched: act_ct: fix skb leak and crash on ooo frags")
Cc: stable@vger.kernel.org # v6.8+
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Link: https://patch.msgid.link/20260806101235.809370-1-hj351016@gmail.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
This commit is contained in:
committed by
Jakub Kicinski
parent
202fef9bbb
commit
8a7ed56167
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user