[PATCH net] net: sched: prevent a use after free

[PATCH net] net: sched: prevent a use after free

[Dan Carpenter]

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

[PATCH net] net: sched: prevent a use after free

[Dan Carpenter]

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

Re: [PATCH net] net: sched: prevent a use after free

[Cong Wang]

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.

Re: [PATCH net] net: sched: prevent a use after free

[Cong Wang]

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.

Re: [PATCH net] net: sched: prevent a use after free

[Dan Carpenter]

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

Re: [PATCH net] net: sched: prevent a use after free

[Dan Carpenter]

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

Re: [PATCH net] net: sched: prevent a use after free

[Cong Wang]

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.

Re: [PATCH net] net: sched: prevent a use after free

[Cong Wang]

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.

Re: [PATCH net] net: sched: prevent a use after free

[Cong Wang]

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.

Re: [PATCH net] net: sched: prevent a use after free

[Cong Wang]

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.

[PATCH v2 net] net: sched: prevent a use after free

[Dan Carpenter]

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

[PATCH v2 net] net: sched: prevent a use after free

[Dan Carpenter]

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

Re: [PATCH v2 net] net: sched: prevent a use after free

[Cong Wang]

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!

Re: [PATCH v2 net] net: sched: prevent a use after free

[Cong Wang]

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!

Re: [PATCH v2 net] net: sched: prevent a use after free

[David Miller]

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.

Re: [PATCH v2 net] net: sched: prevent a use after free

[David Miller]

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.