* [PATCH v2 net] net: sched: fix uses after free
@ 2018-03-15 1:53 Eric Dumazet
2018-03-15 3:10 ` John Fastabend
2018-03-17 21:04 ` David Miller
0 siblings, 2 replies; 4+ messages in thread
From: Eric Dumazet @ 2018-03-15 1:53 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Eric Dumazet, John Fastabend, Jamal Hadi Salim
syzbot reported one use-after-free in pfifo_fast_enqueue() [1]
Issue here is that we can not reuse skb after a successful skb_array_produce()
since another cpu might have consumed it already.
I believe a similar problem exists in try_bulk_dequeue_skb_slow()
in case we put an skb into qdisc_enqueue_skb_bad_txq() for lockless qdisc.
[1]
BUG: KASAN: use-after-free in qdisc_pkt_len include/net/sch_generic.h:610 [inline]
BUG: KASAN: use-after-free in qdisc_qstats_cpu_backlog_inc include/net/sch_generic.h:712 [inline]
BUG: KASAN: use-after-free in pfifo_fast_enqueue+0x4bc/0x5e0 net/sched/sch_generic.c:639
Read of size 4 at addr ffff8801cede37e8 by task syzkaller717588/5543
CPU: 1 PID: 5543 Comm: syzkaller717588 Not tainted 4.16.0-rc4+ #265
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:17 [inline]
dump_stack+0x194/0x24d lib/dump_stack.c:53
print_address_description+0x73/0x250 mm/kasan/report.c:256
kasan_report_error mm/kasan/report.c:354 [inline]
kasan_report+0x23c/0x360 mm/kasan/report.c:412
__asan_report_load4_noabort+0x14/0x20 mm/kasan/report.c:432
qdisc_pkt_len include/net/sch_generic.h:610 [inline]
qdisc_qstats_cpu_backlog_inc include/net/sch_generic.h:712 [inline]
pfifo_fast_enqueue+0x4bc/0x5e0 net/sched/sch_generic.c:639
__dev_xmit_skb net/core/dev.c:3216 [inline]
Fixes: c5ad119fb6c0 ("net: sched: pfifo_fast use skb_array")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot+ed43b6903ab968b16f54@syzkaller.appspotmail.com
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
---
net/sched/sch_generic.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 190570f21b208d5a17943360a3a6f85e1c2a2187..7e3fbe9cc936be376b66a5b12bf8957c3b601f2c 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -106,6 +106,14 @@ static inline void qdisc_enqueue_skb_bad_txq(struct Qdisc *q,
__skb_queue_tail(&q->skb_bad_txq, skb);
+ if (qdisc_is_percpu_stats(q)) {
+ qdisc_qstats_cpu_backlog_inc(q, skb);
+ qdisc_qstats_cpu_qlen_inc(q);
+ } else {
+ qdisc_qstats_backlog_inc(q, skb);
+ q->q.qlen++;
+ }
+
if (lock)
spin_unlock(lock);
}
@@ -196,14 +204,6 @@ static void try_bulk_dequeue_skb_slow(struct Qdisc *q,
break;
if (unlikely(skb_get_queue_mapping(nskb) != mapping)) {
qdisc_enqueue_skb_bad_txq(q, nskb);
-
- if (qdisc_is_percpu_stats(q)) {
- qdisc_qstats_cpu_backlog_inc(q, nskb);
- qdisc_qstats_cpu_qlen_inc(q);
- } else {
- qdisc_qstats_backlog_inc(q, nskb);
- q->q.qlen++;
- }
break;
}
skb->next = nskb;
@@ -628,6 +628,7 @@ static int pfifo_fast_enqueue(struct sk_buff *skb, struct Qdisc *qdisc,
int band = prio2band[skb->priority & TC_PRIO_MAX];
struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
struct skb_array *q = band2list(priv, band);
+ unsigned int pkt_len = qdisc_pkt_len(skb);
int err;
err = skb_array_produce(q, skb);
@@ -636,7 +637,10 @@ static int pfifo_fast_enqueue(struct sk_buff *skb, struct Qdisc *qdisc,
return qdisc_drop_cpu(skb, qdisc, to_free);
qdisc_qstats_cpu_qlen_inc(qdisc);
- qdisc_qstats_cpu_backlog_inc(qdisc, skb);
+ /* Note: skb can not be used after skb_array_produce(),
+ * so we better not use qdisc_qstats_cpu_backlog_inc()
+ */
+ this_cpu_add(qdisc->cpu_qstats->backlog, pkt_len);
return NET_XMIT_SUCCESS;
}
--
2.16.2.804.g6dcf76e118-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 net] net: sched: fix uses after free
2018-03-15 1:53 [PATCH v2 net] net: sched: fix uses after free Eric Dumazet
@ 2018-03-15 3:10 ` John Fastabend
2018-03-15 5:19 ` John Fastabend
2018-03-17 21:04 ` David Miller
1 sibling, 1 reply; 4+ messages in thread
From: John Fastabend @ 2018-03-15 3:10 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller; +Cc: netdev, Eric Dumazet, Jamal Hadi Salim
On 03/14/2018 06:53 PM, Eric Dumazet wrote:
> syzbot reported one use-after-free in pfifo_fast_enqueue() [1]
>
> Issue here is that we can not reuse skb after a successful skb_array_produce()
> since another cpu might have consumed it already.
>
> I believe a similar problem exists in try_bulk_dequeue_skb_slow()
> in case we put an skb into qdisc_enqueue_skb_bad_txq() for lockless qdisc.
>
[...]
> Fixes: c5ad119fb6c0 ("net: sched: pfifo_fast use skb_array")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot+ed43b6903ab968b16f54@syzkaller.appspotmail.com
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> ---
> net/sched/sch_generic.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
Thanks!
Acked-by: John Fastabend <john.fastabend@gmail.com>
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 190570f21b208d5a17943360a3a6f85e1c2a2187..7e3fbe9cc936be376b66a5b12bf8957c3b601f2c 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -106,6 +106,14 @@ static inline void qdisc_enqueue_skb_bad_txq(struct Qdisc *q,
>
> __skb_queue_tail(&q->skb_bad_txq, skb);
>
> + if (qdisc_is_percpu_stats(q)) {
> + qdisc_qstats_cpu_backlog_inc(q, skb);
So I guess the skb access above needs to be removed as
well per your comment in the commit description. But that
can be another patch.
> + qdisc_qstats_cpu_qlen_inc(q);
> + } else {
> + qdisc_qstats_backlog_inc(q, skb);
> + q->q.qlen++;
> + }
> +
> if (lock)
> spin_unlock(lock);
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 net] net: sched: fix uses after free
2018-03-15 3:10 ` John Fastabend
@ 2018-03-15 5:19 ` John Fastabend
0 siblings, 0 replies; 4+ messages in thread
From: John Fastabend @ 2018-03-15 5:19 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller; +Cc: netdev, Eric Dumazet, Jamal Hadi Salim
On 03/14/2018 08:10 PM, John Fastabend wrote:
> On 03/14/2018 06:53 PM, Eric Dumazet wrote:
>> syzbot reported one use-after-free in pfifo_fast_enqueue() [1]
>>
>> Issue here is that we can not reuse skb after a successful skb_array_produce()
>> since another cpu might have consumed it already.
>>
>> I believe a similar problem exists in try_bulk_dequeue_skb_slow()
>> in case we put an skb into qdisc_enqueue_skb_bad_txq() for lockless qdisc.
>>
>
> [...]
>
>> Fixes: c5ad119fb6c0 ("net: sched: pfifo_fast use skb_array")
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Reported-by: syzbot+ed43b6903ab968b16f54@syzkaller.appspotmail.com
>> Cc: John Fastabend <john.fastabend@gmail.com>
>> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>> Cc: Cong Wang <xiyou.wangcong@gmail.com>
>> Cc: Jiri Pirko <jiri@resnulli.us>
>> ---
>> net/sched/sch_generic.c | 22 +++++++++++++---------
>> 1 file changed, 13 insertions(+), 9 deletions(-)
>>
>
> Thanks!
>
> Acked-by: John Fastabend <john.fastabend@gmail.com>
>
>> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>> index 190570f21b208d5a17943360a3a6f85e1c2a2187..7e3fbe9cc936be376b66a5b12bf8957c3b601f2c 100644
>> --- a/net/sched/sch_generic.c
>> +++ b/net/sched/sch_generic.c
>> @@ -106,6 +106,14 @@ static inline void qdisc_enqueue_skb_bad_txq(struct Qdisc *q,
>>
>> __skb_queue_tail(&q->skb_bad_txq, skb);
>>
>> + if (qdisc_is_percpu_stats(q)) {
>> + qdisc_qstats_cpu_backlog_inc(q, skb);
>
> So I guess the skb access above needs to be removed as
> well per your comment in the commit description. But that
> can be another patch.
>
Actually this is fine I read this too quickly on first
read.
Sorry for the noise. Looks good to me.
>> + qdisc_qstats_cpu_qlen_inc(q);
>> + } else {
>> + qdisc_qstats_backlog_inc(q, skb);
>> + q->q.qlen++;
>> + }
>> +
>> if (lock)
>> spin_unlock(lock);
>> }
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 net] net: sched: fix uses after free
2018-03-15 1:53 [PATCH v2 net] net: sched: fix uses after free Eric Dumazet
2018-03-15 3:10 ` John Fastabend
@ 2018-03-17 21:04 ` David Miller
1 sibling, 0 replies; 4+ messages in thread
From: David Miller @ 2018-03-17 21:04 UTC (permalink / raw)
To: edumazet; +Cc: netdev, eric.dumazet, john.fastabend, jhs
From: Eric Dumazet <edumazet@google.com>
Date: Wed, 14 Mar 2018 18:53:00 -0700
> syzbot reported one use-after-free in pfifo_fast_enqueue() [1]
>
> Issue here is that we can not reuse skb after a successful skb_array_produce()
> since another cpu might have consumed it already.
>
> I believe a similar problem exists in try_bulk_dequeue_skb_slow()
> in case we put an skb into qdisc_enqueue_skb_bad_txq() for lockless qdisc.
...
> Fixes: c5ad119fb6c0 ("net: sched: pfifo_fast use skb_array")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot+ed43b6903ab968b16f54@syzkaller.appspotmail.com
Applied, thanks a lot Eric.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-03-17 21:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-15 1:53 [PATCH v2 net] net: sched: fix uses after free Eric Dumazet
2018-03-15 3:10 ` John Fastabend
2018-03-15 5:19 ` John Fastabend
2018-03-17 21:04 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.