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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240213162744.6dcd6667@kernel.org>
Date: Tue, 13 Feb 2024 16:27:44 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Jamal Hadi Salim <jhs@...atatu.com>, davem@...emloft.net,
 netdev@...r.kernel.org, edumazet@...gle.com, Marcelo Ricardo Leitner
 <marcelo.leitner@...il.com>, Davide Caratti <dcaratti@...hat.com>,
 xiyou.wangcong@...il.com, jiri@...nulli.us, shmulik.ladkani@...il.com,
 victor@...atatu.com
Subject: Re: [PATCH net] net/sched: act_mirred: use the backlog for mirred
 ingress

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 <kuba@...nel.org>
---
 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