[PATCH net] sfc: fix NULL pointer dereference in efx_tc_flower_replace_foreign()

[PATCH net] sfc: fix NULL pointer dereference in efx_tc_flower_replace_foreign()

[Rand Deeb]

efx_tc_flower_replace_foreign() walks the rule's actions with an 'act'
cursor. After a redirect action it adds the action set to the rule and
sets the cursor to NULL:

	list_add_tail(&act->list, &rule->acts.list);
	act = NULL;
	if (fa->id == FLOW_ACTION_REDIRECT)
		break; /* end of the line */

The break only leaves the switch statement, so flow_action_for_each()
carries on with the next action while act is NULL. A following redirect
or mirred action then does "save = *act", and a following tunnel decap
action passes act to efx_tc_flower_action_order_ok(), which reads
act->decap. Both dereference a NULL pointer.

The loop is missing the check that efx_tc_flower_replace() performs at
the start of every iteration for the same cursor: reject the rule if an
action follows a non-pipe action. The foreign variant was added without
it.

As a result, a TC flower rule on a tunnel netdevice that redirects to an
sfc representor and lists another redirect, mirred or tunnel decap
action after the first redirect causes a kernel NULL pointer
dereference. It happens in the context of the task adding the filter,
with efx->tc->mutex held. Adding such a rule requires CAP_NET_ADMIN.

Add the same check at the top of the action loop in
efx_tc_flower_replace_foreign(), returning -EINVAL with the same extack
message as efx_tc_flower_replace(). The release path already copes with
act being NULL.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 17654d84b47c ("sfc: add offloading of 'foreign' TC (decap) rules")
Signed-off-by: Rand Deeb <rand.sec96@gmail.com>
---
 drivers/net/ethernet/sfc/tc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c
index 25341acfb..4a740da04 100644
--- a/drivers/net/ethernet/sfc/tc.c
+++ b/drivers/net/ethernet/sfc/tc.c
@@ -1828,6 +1828,13 @@ static int efx_tc_flower_replace_foreign(struct efx_nic *efx,
 	flow_action_for_each(i, fa, &fr->action) {
 		struct efx_tc_action_set save;
 
+		if (!act) {
+			/* more actions after a non-pipe action */
+			NL_SET_ERR_MSG_MOD(extack, "Action follows non-pipe action");
+			rc = -EINVAL;
+			goto release;
+		}
+
 		switch (fa->id) {
 		case FLOW_ACTION_REDIRECT:
 		case FLOW_ACTION_MIRRED:

base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.39.5

Re: [PATCH net] sfc: fix NULL pointer dereference in efx_tc_flower_replace_foreign()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.