netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>,
	davem@davemloft.net, netdev@vger.kernel.org, edumazet@google.com,
	Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>,
	Davide Caratti <dcaratti@redhat.com>,
	xiyou.wangcong@gmail.com, jiri@resnulli.us,
	shmulik.ladkani@gmail.com, victor@mojatatu.com
Subject: Re: [PATCH net] net/sched: act_mirred: use the backlog for mirred ingress
Date: Tue, 13 Feb 2024 16:27:44 -0800	[thread overview]
Message-ID: <20240213162744.6dcd6667@kernel.org> (raw)
In-Reply-To: <93a346087193c57f4df807c478d0f7fc8e7db6aa.camel@redhat.com>

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@kernel.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


  reply	other threads:[~2024-02-14  0:27 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-09 23:54 [PATCH net] net/sched: act_mirred: use the backlog for mirred ingress Jakub Kicinski
2024-02-12 14:51 ` Jamal Hadi Salim
2024-02-12 15:02   ` Jakub Kicinski
2024-02-12 15:11   ` Jamal Hadi Salim
2024-02-13 11:06     ` Paolo Abeni
2024-02-14  0:27       ` Jakub Kicinski [this message]
2024-02-14  3:40         ` Jakub Kicinski
2024-02-14 15:11 ` Jamal Hadi Salim
2024-02-14 15:28   ` Jamal Hadi Salim
2024-02-14 16:10     ` Davide Caratti
2024-02-15  0:31       ` Jakub Kicinski
2024-02-15 17:55         ` Davide Caratti
  -- strict thread matches above, loose matches on Subject: below --
2022-09-23 15:11 Davide Caratti
2022-09-25 18:08 ` Cong Wang
2022-10-04 17:40   ` Davide Caratti
2022-10-16 17:28     ` Cong Wang
2022-11-18 23:07 ` Peilin Ye

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240213162744.6dcd6667@kernel.org \
    --to=kuba@kernel.org \
    --cc=davem@davemloft.net \
    --cc=dcaratti@redhat.com \
    --cc=edumazet@google.com \
    --cc=jhs@mojatatu.com \
    --cc=jiri@resnulli.us \
    --cc=marcelo.leitner@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=shmulik.ladkani@gmail.com \
    --cc=victor@mojatatu.com \
    --cc=xiyou.wangcong@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).