Subject: Re: [PATCH net] net/sched: act_mirred: use the backlog for mirred ingress

From: Jakub Kicinski <[email-masked]>
Date: Tue, 13 Feb 2024 16:27:44 -0800

On Tue, 13 Feb 2024 12:06:04 +0100 Paolo Abeni wrote:
> > Something broke.
> > Create a ns. Put one half of veth into the namespace. Create a filter
> > inside the net ns.
> > at_ns$ tc qdisc add dev port0 ingress_block 21 clsact
> > at_ns$ tc filter add block 21 egress protocol ip prio 10 matchall
> > action mirred ingress redirect dev port0
> > 
> > Send a ping from host:
> > at_host@ ping 10.0.0.2 -c 1 -I <vethportonhostside>
> > 
> > And.. hits uaf.... see attached.  
> 
> It looks like:
> 
> netif_receive_skb
> run_tc()
> 	act_mirred	
> 		netif_receive_skb
> 			sch_handle_ingress
> 				act_mirred // nesting limit hit
> 			// free skb
> 		// netif_receive_skb returns NET_RX_DROP
> 	// act_mirred returns TC_ACT_SHOT
> // UaF while de-referencing the (freed) skb
> 
> 
> No idea how to solve it on top of my mind :(

If I'm looking right the bug seems fairly straightforward but tricky 
to cleanly fix :( I also haven't dug deep enough in the history to
be provide a real Fixes tag...

--->8-------------
net/sched: act_mirred: don't override retval if we already lost the skb

If we're redirecting the skb, and haven't called tcf_mirred_forward(),
yet, we need to tell the core to drop the skb by setting the retcode
to SHOT. If we have called tcf_mirred_forward(), however, the skb
is out of our hands and returning SHOT will lead to UaF.

Move the overrides up to the error paths which actually need them.
Note that the err variable is only used to store return code from
tcf_mirred_forward() and we don't have to set it.

Fixes: 16085e48cb48 ("net/sched: act_mirred: Create function tcf_mirred_to_dev and improve readability")
Signed-off-by: Jakub Kicinski <[email-masked]>
---
 net/sched/act_mirred.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 93a96e9d8d90..922a018329cd 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -270,7 +270,8 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
 	if (unlikely(!(dev->flags & IFF_UP)) || !netif_carrier_ok(dev)) {
 		net_notice_ratelimited("tc mirred to Houston: device %s is down\n",
 				       dev->name);
-		err = -ENODEV;
+		if (is_redirect)
+			retval = TC_ACT_SHOT;
 		goto out;
 	}
 
@@ -284,7 +285,8 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
 	if (!dont_clone) {
 		skb_to_send = skb_clone(skb, GFP_ATOMIC);
 		if (!skb_to_send) {
-			err =  -ENOMEM;
+			if (is_redirect)
+				retval = TC_ACT_SHOT;
 			goto out;
 		}
 	}
@@ -327,8 +329,6 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
 	if (err) {
 out:
 		tcf_action_inc_overlimit_qstats(&m->common);
-		if (is_redirect)
-			retval = TC_ACT_SHOT;
 	}
 
 	return retval;
-- 
2.43.0