| Message ID | 20200131065647.joonbg3wzcw26x3b@kili.mountain |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: sched: prevent a use after free | expand |
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.
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
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.
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.
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);
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(-)