net/sched: act_pedit: fix TOCTOU heap OOB write in tc offload

There is a TOCTOU race condition in flower lockless approach between sizing
a flow_rule buffer and filling it.
zdi-disclosures@trendmicro.com reports:
The cls_flower classifier operates with TCF_PROTO_OPS_DOIT_UNLOCKED
(fl_change runs without RTNL), while RTM_NEWACTION holds RTNL, so the
independent locking domains make the race reachable in practice.  KASAN
confirms:
  BUG: KASAN: slab-out-of-bounds in tcf_pedit_offload_act_setup+0x81b/0x930
  Write of size 4 at addr ffff888001f27520 by task poc-toctou/312
  The buggy address is located 0 bytes to the right of
   allocated 288-byte region [ffff888001f27400, ffff888001f27520)
   (cache kmalloc-512)

Note: The result is a heap OOB write attacker-controlled content into the
adjacent slab object (requires CAP_NET_ADMIN).

The fix introduces reading tcfp_nkeys under act->tcfa_lock in all places
using a new tcf_pedit_nkeys_locked() which replaces the old tcf_pedit_nkeys().
Additionally we close the remaining TOCTOU window between the sizing read and
the fill reads by more careful accounting.
Rather than silently truncating the key count, which leads to incorrect
action semantics offloaded to hardware and secondary OOB writes if
the remaining capacity is zero or consumed by prior actions, we enforce
remaining capacity checks and return -ENOSPC if the required space exceeds
the remaining capacity.

Fixes: 71d0ed7079df ("net/act_pedit: Support using offset relative to the conventional network headers")
Reported-by: zdi-disclosures@trendmicro.com
Tested-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Link: https://patch.msgid.link/20260701161912.125355-1-jhs@mojatatu.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>

diff --git a/include/net/tc_act/tc_pedit.h b/include/net/tc_act/tc_pedit.h
index cb7b82f2cbc7fd8778a262edf4813a2734cb6127..97754ea0a8279d5ef1830f180dffcd5a86edf87e 100644 (file)
--- a/include/net/tc_act/tc_pedit.h
+++ b/include/net/tc_act/tc_pedit.h
@@ -37,17 +37,15 @@ static inline bool is_tcf_pedit(const struct tc_action *a)
        return false;
 }
 
-static inline int tcf_pedit_nkeys(const struct tc_action *a)
+/* Must be called with act->tcfa_lock held to ensure consistency of parallel
+ * reads of the same action's pedit keys (e.g. flow_offload count vs fill).
+ * Note, this is only used for pedit offload.
+ */
+static inline int tcf_pedit_nkeys_locked(const struct tc_action *a)
 {
-       struct tcf_pedit_parms *parms;
-       int nkeys;
-
-       rcu_read_lock();
-       parms = to_pedit_parms(a);
-       nkeys = parms->tcfp_nkeys;
-       rcu_read_unlock();
-
-       return nkeys;
+       lockdep_assert_held(&a->tcfa_lock);
+       return rcu_dereference_protected(to_pedit(a)->parms,
+                                        lockdep_is_held(&a->tcfa_lock))->tcfp_nkeys;
 }
 
 static inline u32 tcf_pedit_htype(const struct tc_action *a, int index)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index b68be143a067f9b7c5a1ab36195d05150d1ec234..f141634df21489495e933efbb80a18b7a5687d42 100644 (file)
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -148,10 +148,15 @@ static void offload_action_hw_count_dec(struct tc_action *act,
 
 static unsigned int tcf_offload_act_num_actions_single(struct tc_action *act)
 {
-       if (is_tcf_pedit(act))
-               return tcf_pedit_nkeys(act);
-       else
-               return 1;
+       unsigned int count;
+
+       if (is_tcf_pedit(act)) {
+               spin_lock_bh(&act->tcfa_lock);
+               count = tcf_pedit_nkeys_locked(act);
+               spin_unlock_bh(&act->tcfa_lock);
+               return count;
+       }
+       return 1;
 }
 
 static bool tc_act_skip_hw(u32 flags)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 0d652dea4a691fce18f467bc7f256718d62f5981..d4d47a9921f45777eb11afac9ab2e6b9c25cf770 100644 (file)
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -567,9 +567,18 @@ static int tcf_pedit_offload_act_setup(struct tc_action *act, void *entry_data,
 {
        if (bind) {
                struct flow_action_entry *entry = entry_data;
+               int nkeys = tcf_pedit_nkeys_locked(act);
                int k;
 
-               for (k = 0; k < tcf_pedit_nkeys(act); k++) {
+               /* If the required keys exceed the remaining capacity return
+                * -ENOSPC to abort the offload and fallback to software.
+                */
+               if (nkeys > *index_inc) {
+                       NL_SET_ERR_MSG_MOD(extack, "Not enough space to offload all pedit keys");
+                       return -ENOSPC;
+               }
+
+               for (k = 0; k < nkeys; k++) {
                        switch (tcf_pedit_cmd(act, k)) {
                        case TCA_PEDIT_KEY_EX_CMD_SET:
                                entry->id = FLOW_ACTION_MANGLE;
@@ -606,7 +615,7 @@ static int tcf_pedit_offload_act_setup(struct tc_action *act, void *entry_data,
                        return -EOPNOTSUPP;
                }
 
-               for (k = 1; k < tcf_pedit_nkeys(act); k++) {
+               for (k = 1; k < tcf_pedit_nkeys_locked(act); k++) {
                        if (cmd != tcf_pedit_cmd(act, k)) {
                                NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit command offload");
                                return -EOPNOTSUPP;

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 3e67600a4a1a10a8d63c7c4216f09fd4223fefe3..ffeea6db833747b5befee4d6c0ce2f889771a7e1 100644 (file)
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3886,12 +3886,21 @@ int tc_setup_action(struct flow_action *flow_action,
 
                entry = &flow_action->entries[j];
                spin_lock_bh(&act->tcfa_lock);
+
+               /* Abort the offload if we have exhausted the allocated capacity */
+               if (j >= flow_action->num_entries) {
+                       NL_SET_ERR_MSG_MOD(extack, "Flow action buffer overflow");
+                       err = -ENOSPC;
+                       goto err_out_locked;
+               }
+
                err = tcf_act_get_user_cookie(entry, act);
                if (err)
                        goto err_out_locked;
 
-               index = 0;
-               err = tc_setup_offload_act(act, entry, &index, extack);
+               index = flow_action->num_entries - j;
+               err = tc_setup_offload_act(act, entry, &index,
+                                          extack);
                if (err)
                        goto err_out_locked;
 
@@ -3945,10 +3954,13 @@ unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
        int i;
 
        tcf_exts_for_each_action(i, act, exts) {
-               if (is_tcf_pedit(act))
-                       num_acts += tcf_pedit_nkeys(act);
-               else
+               if (is_tcf_pedit(act)) {
+                       spin_lock_bh(&act->tcfa_lock);
+                       num_acts += tcf_pedit_nkeys_locked(act);
+                       spin_unlock_bh(&act->tcfa_lock);
+               } else {
                        num_acts++;
+               }
        }
        return num_acts;
 }