On 9/22/26 4:58 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] tcf_ct_act() now discards an unconfirmed conntrack entry
> purely because its refcount is >1, without checking whether the…
>
> Pre-existing issues:
> - [Medium] At this commit, tcf_ct_act() still calls nf_ct_helper()
> (net/sched/act_ct.c:1106) before the commit block that grows the…
>
> --
>
> net/sched: act_ct: avoid modifying shared unconfirmed ct entry
>
> tcf_ct_act() now resets the skb conntrack association when the cached entry
> is unconfirmed and its refcount shows it is shared with another skb, so that
> a commit or nat pass cannot grow the extension area of an entry another
> clone still points into. The nat flag is also hoisted into a local
> variable.
>
>> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
>> index 55f3521edb4c9..e72143d36b119 100644
>> --- a/net/sched/act_ct.c
>> +++ b/net/sched/act_ct.c
>
> [ ... ]
>
>> @@ -998,6 +998,7 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb,
>> const struct tc_action *a,
>> retval = p->action;
>> commit = p->ct_action & TCA_CT_ACT_COMMIT;
>> clear = p->ct_action & TCA_CT_ACT_CLEAR;
>> + nat = p->ct_action & TCA_CT_ACT_NAT;
>> tmpl = p->tmpl;
>>
>> tcf_lastuse_update(&c->tcf_tm);
>> @@ -1046,6 +1047,19 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb,
>> const struct tc_action *a,
>> * different zone.
>> */
>> cached = tcf_ct_skb_nfct_cached(net, skb, p);
>> +
>> + /* If the ct entry is not confirmed and shared with some other skb,
>> + * e.g., a cloned one, we can't just modify it with a commit or nat
>> + * as we must not modify the extension set. Reset.
>> + */
>> + if (cached && (commit || nat)) {
>> + ct = nf_ct_get(skb, &ctinfo);
>> + if (ct && !nf_ct_is_confirmed(ct) && nf_ct_shared(ct)) {
>> + nf_reset_ct(skb);
>> + cached = false;
>> + }
>> + }
>> +
>
> [Severity: Medium]
> Can this drop NAT state that was already applied to the packet?
>
> The condition keys off nf_ct_shared(ct), i.e. refcount > 1, and not off
> whether the sharing skb will ever be re-processed by conntrack. act_ct
> permits nat without commit, and in that case
> tcf_ct_act_nat()->nf_ct_nat() (net/netfilter/nf_nat_ovs.c) already
> installs the binding and translates the headers on the unconfirmed entry:
>
> net/netfilter/nf_nat_ovs.c:nf_ct_nat() {
> ...
> /* Add NAT extension if not confirmed yet. */
> if (!nf_ct_is_confirmed(ct) && !nf_ct_nat_ext_add(ct))
> return NF_DROP; /* Can't NAT. */
> ...
> err = nf_ct_nat_execute(skb, ct, ctinfo, action, range, maniptype);
> }
>
> If the skb is then cloned by a path that keeps the nfct reference, and a
> later filter runs "action ct zone N commit" on the original, the new block
> sees cached && commit with an unconfirmed, shared entry, calls
> nf_reset_ct(skb) and sets cached = false. nf_conntrack_in() then re-tracks
> the already translated packet, so the entry that gets committed carries the
> post-NAT tuple as its ORIGINAL tuple and no matching binding, and reply
> traffic is no longer reverse translated. Before this change the cached,
> NAT'ed unconfirmed entry was simply committed.
It's true that some information will be lost on reset, but it is expected.
The described sequence of events should also not happen in a practical
networking pipeline. Alternative is to forbid nat without commit, which
would be a significant uAPI break.
>
> One note on the trigger: act_mirred and AF_PACKET taps do not produce this
> sharing, since both clear the association on the clone:
>
> net/sched/act_mirred.c:tcf_mirred_to_dev() {
> /* All mirred/redirected skbs should clear previous ct info */
> nf_reset_ct(skb_to_send);
> }
>
> net/packet/af_packet.c:packet_rcv() {
> /* drop conntrack reference */
> nf_reset_ct(skb);
> }
>
> Bridge flood/multicast delivery does keep it, though -
> net/bridge/br_forward.c:deliver_clone() calls skb_clone() with no
> nf_reset_ct(), so __nf_copy() raises ct_general.use on the clone.
>
> Would it be worth mentioning in the changelog that the reset also fires
> when the second pass does commit? The changelog currently only argues
> about the "without commit" case:
>
> This, however, doesn't really change the picture for sensible
> networking cases as there should be no need to run the same packet
> twice (before and after the clone) through conntrack without packet
> header or zone changes and without commit.
Flood cases will be addressed separately, there is no need to spell out
every single thing.
>
> [Severity: Medium]
> This is a pre-existing ordering problem and is not introduced by this
> patch, but it is worth flagging for backports since the new guard does not
> cover it.
>
> At this commit tcf_ct_act() still runs the helper before the commit block
> grows the extension area of an unconfirmed entry:
>
> net/sched/act_ct.c:tcf_ct_act() {
> ...
> if (nf_ct_is_confirmed(ct) ? ((!cached && !skip_add) || add_helper) :
> commit) {
> err = nf_ct_helper(skb, ct, ctinfo, family);
> if (err != NF_ACCEPT)
> goto nf_error;
> }
>
> if (commit) {
> tcf_ct_act_set_mark(ct, p->mark, p->mark_mask);
> tcf_ct_act_set_labels(ct, p->labels, p->labels_mask);
>
> if (!nf_ct_is_confirmed(ct))
> nf_conn_act_ct_ext_add(skb, ct, ctinfo);
> ...
> }
>
> A helper that inserts an expectation links exp->lnode into a list head that
> lives inside ct->ext:
>
> net/netfilter/nf_conntrack_expect.c:nf_ct_expect_insert() {
> hlist_add_head_rcu(&exp->lnode, &master_help->expectations);
> }
>
> and the following nf_ct_ext_add(NF_CT_EXT_ACT_CT) relocates that memory:
>
> net/netfilter/nf_conntrack_extend.c:nf_ct_ext_add() {
> new = krealloc(old, alloc, gfp);
> }
>
> leaving exp->lnode.pprev pointing into the freed block, which is written
> through when the expectation is unlinked via nf_ct_remove_expectations().
>
> The new reset only fires for cached entries with refcount > 1, so an entry
> that was just tracked (cached == false), or the replacement entry created
> after the reset, still follows the helper-then-realloc sequence with a
> single skb reference.
>
> The later patch in this series ("net/sched: act_ct: fix helper UAF due to
> extensions realloc") moves nf_ct_helper() after the
> nf_conn_act_ct_ext_add() block with the comment "This has to be done after
> all the extensions are already added.", so the ordering is resolved within
> the series. Given the stable tag here, should the two patches be marked so
> that backporters take the reordering change together with this one?
Preexisting. Fixed later in the set.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
