* [PATCH net] net: sched: prevent a use after free
@ 2020-01-31 6:56 Dan Carpenter
2020-02-01 19:38 ` Cong Wang
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2020-01-31 6:56 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Cong Wang, Jiri Pirko, David S. Miller, Jakub Kicinski,
Mohit Bhasi, Mohit P. Tahiliani, V. Saicharan,
Gautam Ramakrishnan, netdev, kernel-janitors
The code calls kfree_skb(skb); and then re-uses "skb" on the next line.
Let's re-order these lines to solve the problem.
Fixes: ec97ecf1ebe4 ("net: sched: add Flow Queue PIE packet scheduler")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
net/sched/sch_fq_pie.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/sch_fq_pie.c b/net/sched/sch_fq_pie.c
index bbd0dea6b6b9..78472e0773e9 100644
--- a/net/sched/sch_fq_pie.c
+++ b/net/sched/sch_fq_pie.c
@@ -349,9 +349,9 @@ static int fq_pie_change(struct Qdisc *sch, struct nlattr *opt,
while (sch->q.qlen > sch->limit) {
struct sk_buff *skb = fq_pie_qdisc_dequeue(sch);
- kfree_skb(skb);
len_dropped += qdisc_pkt_len(skb);
num_dropped += 1;
+ kfree_skb(skb);
}
qdisc_tree_reduce_backlog(sch, num_dropped, len_dropped);
--
2.11.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH net] net: sched: prevent a use after free
2020-01-31 6:56 [PATCH net] net: sched: prevent a use after free Dan Carpenter
@ 2020-02-01 19:38 ` Cong Wang
2020-02-03 8:38 ` Dan Carpenter
0 siblings, 1 reply; 8+ messages in thread
From: Cong Wang @ 2020-02-01 19:38 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski,
Mohit Bhasi, Mohit P. Tahiliani, V. Saicharan,
Gautam Ramakrishnan, Linux Kernel Network Developers,
kernel-janitors
On Thu, Jan 30, 2020 at 10:57 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> The code calls kfree_skb(skb); and then re-uses "skb" on the next line.
> Let's re-order these lines to solve the problem.
>
> Fixes: ec97ecf1ebe4 ("net: sched: add Flow Queue PIE packet scheduler")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> net/sched/sch_fq_pie.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_fq_pie.c b/net/sched/sch_fq_pie.c
> index bbd0dea6b6b9..78472e0773e9 100644
> --- a/net/sched/sch_fq_pie.c
> +++ b/net/sched/sch_fq_pie.c
> @@ -349,9 +349,9 @@ static int fq_pie_change(struct Qdisc *sch, struct nlattr *opt,
> while (sch->q.qlen > sch->limit) {
> struct sk_buff *skb = fq_pie_qdisc_dequeue(sch);
>
> - kfree_skb(skb);
> len_dropped += qdisc_pkt_len(skb);
> num_dropped += 1;
> + kfree_skb(skb);
Or even better: use rtnl_kfree_skbs().
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net] net: sched: prevent a use after free
2020-02-01 19:38 ` Cong Wang
@ 2020-02-03 8:38 ` Dan Carpenter
2020-02-03 19:58 ` Cong Wang
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2020-02-03 8:38 UTC (permalink / raw)
To: Cong Wang
Cc: Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski,
Mohit Bhasi, Mohit P. Tahiliani, V. Saicharan,
Gautam Ramakrishnan, Linux Kernel Network Developers,
kernel-janitors
On Sat, Feb 01, 2020 at 11:38:43AM -0800, Cong Wang wrote:
> On Thu, Jan 30, 2020 at 10:57 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > The code calls kfree_skb(skb); and then re-uses "skb" on the next line.
> > Let's re-order these lines to solve the problem.
> >
> > Fixes: ec97ecf1ebe4 ("net: sched: add Flow Queue PIE packet scheduler")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > net/sched/sch_fq_pie.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/sched/sch_fq_pie.c b/net/sched/sch_fq_pie.c
> > index bbd0dea6b6b9..78472e0773e9 100644
> > --- a/net/sched/sch_fq_pie.c
> > +++ b/net/sched/sch_fq_pie.c
> > @@ -349,9 +349,9 @@ static int fq_pie_change(struct Qdisc *sch, struct nlattr *opt,
> > while (sch->q.qlen > sch->limit) {
> > struct sk_buff *skb = fq_pie_qdisc_dequeue(sch);
> >
> > - kfree_skb(skb);
> > len_dropped += qdisc_pkt_len(skb);
> > num_dropped += 1;
> > + kfree_skb(skb);
>
> Or even better: use rtnl_kfree_skbs().
Why is that better?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net] net: sched: prevent a use after free
2020-02-03 8:38 ` Dan Carpenter
@ 2020-02-03 19:58 ` Cong Wang
2020-02-03 20:33 ` Cong Wang
0 siblings, 1 reply; 8+ messages in thread
From: Cong Wang @ 2020-02-03 19:58 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski,
Mohit Bhasi, Mohit P. Tahiliani, V. Saicharan,
Gautam Ramakrishnan, Linux Kernel Network Developers,
kernel-janitors
On Mon, Feb 3, 2020 at 12:39 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Sat, Feb 01, 2020 at 11:38:43AM -0800, Cong Wang wrote:
> > On Thu, Jan 30, 2020 at 10:57 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > The code calls kfree_skb(skb); and then re-uses "skb" on the next line.
> > > Let's re-order these lines to solve the problem.
> > >
> > > Fixes: ec97ecf1ebe4 ("net: sched: add Flow Queue PIE packet scheduler")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > ---
> > > net/sched/sch_fq_pie.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/sched/sch_fq_pie.c b/net/sched/sch_fq_pie.c
> > > index bbd0dea6b6b9..78472e0773e9 100644
> > > --- a/net/sched/sch_fq_pie.c
> > > +++ b/net/sched/sch_fq_pie.c
> > > @@ -349,9 +349,9 @@ static int fq_pie_change(struct Qdisc *sch, struct nlattr *opt,
> > > while (sch->q.qlen > sch->limit) {
> > > struct sk_buff *skb = fq_pie_qdisc_dequeue(sch);
> > >
> > > - kfree_skb(skb);
> > > len_dropped += qdisc_pkt_len(skb);
> > > num_dropped += 1;
> > > + kfree_skb(skb);
> >
> > Or even better: use rtnl_kfree_skbs().
>
> Why is that better?
Because it is designed to be used in this scenario,
as it defers the free after RTNL unlock which is after
sch_tree_unlock() too.
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net] net: sched: prevent a use after free
2020-02-03 19:58 ` Cong Wang
@ 2020-02-03 20:33 ` Cong Wang
2020-02-05 11:53 ` [PATCH v2 " Dan Carpenter
0 siblings, 1 reply; 8+ messages in thread
From: Cong Wang @ 2020-02-03 20:33 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski,
Mohit Bhasi, Mohit P. Tahiliani, V. Saicharan,
Gautam Ramakrishnan, Linux Kernel Network Developers,
kernel-janitors
On Mon, Feb 3, 2020 at 11:58 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Mon, Feb 3, 2020 at 12:39 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > Why is that better?
>
> Because it is designed to be used in this scenario,
> as it defers the free after RTNL unlock which is after
> sch_tree_unlock() too.
Just in case of misunderstanding: I am _not_ suggesting to
use rtnl_kfree_skbs() to workaround this use-after-free,
rtnl_kfree_skbs() still has to be called after qdisc_pkt_len(),
at least for readability, despite that it could indeed
workaround the bug.
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 net] net: sched: prevent a use after free
2020-02-03 20:33 ` Cong Wang
@ 2020-02-05 11:53 ` Dan Carpenter
2020-02-05 18:03 ` Cong Wang
2020-02-06 13:01 ` David Miller
0 siblings, 2 replies; 8+ messages in thread
From: Dan Carpenter @ 2020-02-05 11:53 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Cong Wang, Jiri Pirko, David S. Miller, Jakub Kicinski,
Mohit Bhasi, V. Saicharan, Leslie Monis, Sachin D. Patil,
Gautam Ramakrishnan, netdev, kernel-janitors
The bug is that we call kfree_skb(skb) and then pass "skb" to
qdisc_pkt_len(skb) on the next line, which is a use after free.
Also Cong Wang points out that it's better to delay the actual
frees until we drop the rtnl lock so we should use rtnl_kfree_skbs()
instead of kfree_skb().
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Fixes: ec97ecf1ebe4 ("net: sched: add Flow Queue PIE packet scheduler")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Use rtnl_kfree_skbs() instead of kfree_skb(). From static analysis.
Not tested, but I have audited the code pretty close and I think
switing to rtnl_kfree_skbs() is harmless.
net/sched/sch_fq_pie.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/sch_fq_pie.c b/net/sched/sch_fq_pie.c
index bbd0dea6b6b9..214657eb3dfd 100644
--- a/net/sched/sch_fq_pie.c
+++ b/net/sched/sch_fq_pie.c
@@ -349,9 +349,9 @@ static int fq_pie_change(struct Qdisc *sch, struct nlattr *opt,
while (sch->q.qlen > sch->limit) {
struct sk_buff *skb = fq_pie_qdisc_dequeue(sch);
- kfree_skb(skb);
len_dropped += qdisc_pkt_len(skb);
num_dropped += 1;
+ rtnl_kfree_skbs(skb, skb);
}
qdisc_tree_reduce_backlog(sch, num_dropped, len_dropped);
--
2.11.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 net] net: sched: prevent a use after free
2020-02-05 11:53 ` [PATCH v2 " Dan Carpenter
@ 2020-02-05 18:03 ` Cong Wang
2020-02-06 13:01 ` David Miller
1 sibling, 0 replies; 8+ messages in thread
From: Cong Wang @ 2020-02-05 18:03 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski,
Mohit Bhasi, V. Saicharan, Leslie Monis, Sachin D. Patil,
Gautam Ramakrishnan, Linux Kernel Network Developers,
kernel-janitors
On Wed, Feb 5, 2020 at 3:56 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> The bug is that we call kfree_skb(skb) and then pass "skb" to
> qdisc_pkt_len(skb) on the next line, which is a use after free.
> Also Cong Wang points out that it's better to delay the actual
> frees until we drop the rtnl lock so we should use rtnl_kfree_skbs()
> instead of kfree_skb().
>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Fixes: ec97ecf1ebe4 ("net: sched: add Flow Queue PIE packet scheduler")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
Thanks!
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 net] net: sched: prevent a use after free
2020-02-05 11:53 ` [PATCH v2 " Dan Carpenter
2020-02-05 18:03 ` Cong Wang
@ 2020-02-06 13:01 ` David Miller
1 sibling, 0 replies; 8+ messages in thread
From: David Miller @ 2020-02-06 13:01 UTC (permalink / raw)
To: dan.carpenter
Cc: jhs, xiyou.wangcong, jiri, kuba, mohitbhasi1998, vsaicharan1998,
lesliemonis, sdp.sachin, gautamramk, netdev, kernel-janitors
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Wed, 5 Feb 2020 14:53:30 +0300
> The bug is that we call kfree_skb(skb) and then pass "skb" to
> qdisc_pkt_len(skb) on the next line, which is a use after free.
> Also Cong Wang points out that it's better to delay the actual
> frees until we drop the rtnl lock so we should use rtnl_kfree_skbs()
> instead of kfree_skb().
>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Fixes: ec97ecf1ebe4 ("net: sched: add Flow Queue PIE packet scheduler")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: Use rtnl_kfree_skbs() instead of kfree_skb(). From static analysis.
> Not tested, but I have audited the code pretty close and I think
> switing to rtnl_kfree_skbs() is harmless.
Applied, thanks Dan.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-02-06 13:01 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-31 6:56 [PATCH net] net: sched: prevent a use after free Dan Carpenter
2020-02-01 19:38 ` Cong Wang
2020-02-03 8:38 ` Dan Carpenter
2020-02-03 19:58 ` Cong Wang
2020-02-03 20:33 ` Cong Wang
2020-02-05 11:53 ` [PATCH v2 " Dan Carpenter
2020-02-05 18:03 ` Cong Wang
2020-02-06 13:01 ` David Miller
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).