* [PATCH net] net: sched: fix possible crash in tcf_action_destroy()
@ 2019-09-18 19:57 Eric Dumazet
2019-09-18 21:37 ` Cong Wang
2019-09-24 14:36 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2019-09-18 19:57 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Eric Dumazet, syzbot, Vlad Buslov, Jiri Pirko
If the allocation done in tcf_exts_init() failed,
we end up with a NULL pointer in exts->actions.
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 8198 Comm: syz-executor.3 Not tainted 5.3.0-rc8+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:tcf_action_destroy+0x71/0x160 net/sched/act_api.c:705
Code: c3 08 44 89 ee e8 4f cb bb fb 41 83 fd 20 0f 84 c9 00 00 00 e8 c0 c9 bb fb 48 89 d8 48 b9 00 00 00 00 00 fc ff df 48 c1 e8 03 <80> 3c 08 00 0f 85 c0 00 00 00 4c 8b 33 4d 85 f6 0f 84 9d 00 00 00
RSP: 0018:ffff888096e16ff0 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000000 RCX: dffffc0000000000
RDX: 0000000000040000 RSI: ffffffff85b6ab30 RDI: 0000000000000000
RBP: ffff888096e17020 R08: ffff8880993f6140 R09: fffffbfff11cae67
R10: fffffbfff11cae66 R11: ffffffff88e57333 R12: 0000000000000000
R13: 0000000000000000 R14: ffff888096e177a0 R15: 0000000000000001
FS: 00007f62bc84a700(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000758040 CR3: 0000000088b64000 CR4: 00000000001426e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
tcf_exts_destroy+0x38/0xb0 net/sched/cls_api.c:3030
tcindex_set_parms+0xf7f/0x1e50 net/sched/cls_tcindex.c:488
tcindex_change+0x230/0x318 net/sched/cls_tcindex.c:519
tc_new_tfilter+0xa4b/0x1c70 net/sched/cls_api.c:2152
rtnetlink_rcv_msg+0x838/0xb00 net/core/rtnetlink.c:5214
netlink_rcv_skb+0x177/0x450 net/netlink/af_netlink.c:2477
rtnetlink_rcv+0x1d/0x30 net/core/rtnetlink.c:5241
netlink_unicast_kernel net/netlink/af_netlink.c:1302 [inline]
netlink_unicast+0x531/0x710 net/netlink/af_netlink.c:1328
netlink_sendmsg+0x8a5/0xd60 net/netlink/af_netlink.c:1917
sock_sendmsg_nosec net/socket.c:637 [inline]
sock_sendmsg+0xd7/0x130 net/socket.c:657
___sys_sendmsg+0x3e2/0x920 net/socket.c:2311
__sys_sendmmsg+0x1bf/0x4d0 net/socket.c:2413
__do_sys_sendmmsg net/socket.c:2442 [inline]
Fixes: 90b73b77d08e ("net: sched: change action API to use array of pointers to actions")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
Cc: Vlad Buslov <vladbu@mellanox.com>
Cc: Jiri Pirko <jiri@mellanox.com>
---
net/sched/cls_api.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index efd3cfb80a2ad775dc8ab3c4900bd73d52c7aaad..9aef93300f1c11791acbb9262dfe77996872eafe 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3027,8 +3027,10 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
void tcf_exts_destroy(struct tcf_exts *exts)
{
#ifdef CONFIG_NET_CLS_ACT
- tcf_action_destroy(exts->actions, TCA_ACT_UNBIND);
- kfree(exts->actions);
+ if (exts->actions) {
+ tcf_action_destroy(exts->actions, TCA_ACT_UNBIND);
+ kfree(exts->actions);
+ }
exts->nr_actions = 0;
#endif
}
--
2.23.0.237.gc6a4ce50a0-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: sched: fix possible crash in tcf_action_destroy()
2019-09-18 19:57 [PATCH net] net: sched: fix possible crash in tcf_action_destroy() Eric Dumazet
@ 2019-09-18 21:37 ` Cong Wang
2019-09-22 2:08 ` Jakub Kicinski
2019-09-24 14:36 ` David Miller
1 sibling, 1 reply; 6+ messages in thread
From: Cong Wang @ 2019-09-18 21:37 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, netdev, Eric Dumazet, syzbot, Vlad Buslov, Jiri Pirko
On Wed, Sep 18, 2019 at 12:57 PM 'Eric Dumazet' via syzkaller
<syzkaller@googlegroups.com> wrote:
>
> If the allocation done in tcf_exts_init() failed,
> we end up with a NULL pointer in exts->actions.
...
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index efd3cfb80a2ad775dc8ab3c4900bd73d52c7aaad..9aef93300f1c11791acbb9262dfe77996872eafe 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3027,8 +3027,10 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
> void tcf_exts_destroy(struct tcf_exts *exts)
> {
> #ifdef CONFIG_NET_CLS_ACT
> - tcf_action_destroy(exts->actions, TCA_ACT_UNBIND);
> - kfree(exts->actions);
> + if (exts->actions) {
I think it is _slightly_ better to check exts->nr_actions!=0 here,
as it would help exts->actions!=NULL&& exts->nr_actions==0
cases too.
What do you think?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: sched: fix possible crash in tcf_action_destroy()
2019-09-18 21:37 ` Cong Wang
@ 2019-09-22 2:08 ` Jakub Kicinski
2019-09-23 15:44 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2019-09-22 2:08 UTC (permalink / raw)
To: Cong Wang, Eric Dumazet
Cc: Eric Dumazet, David S . Miller, netdev, syzbot, Vlad Buslov, Jiri Pirko
On Wed, 18 Sep 2019 14:37:21 -0700, Cong Wang wrote:
> On Wed, Sep 18, 2019 at 12:57 PM 'Eric Dumazet' via syzkaller
> <syzkaller@googlegroups.com> wrote:
> >
> > If the allocation done in tcf_exts_init() failed,
> > we end up with a NULL pointer in exts->actions.
> ...
> > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> > index efd3cfb80a2ad775dc8ab3c4900bd73d52c7aaad..9aef93300f1c11791acbb9262dfe77996872eafe 100644
> > --- a/net/sched/cls_api.c
> > +++ b/net/sched/cls_api.c
> > @@ -3027,8 +3027,10 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
> > void tcf_exts_destroy(struct tcf_exts *exts)
> > {
> > #ifdef CONFIG_NET_CLS_ACT
> > - tcf_action_destroy(exts->actions, TCA_ACT_UNBIND);
> > - kfree(exts->actions);
> > + if (exts->actions) {
>
> I think it is _slightly_ better to check exts->nr_actions!=0 here,
> as it would help exts->actions!=NULL&& exts->nr_actions==0
> cases too.
>
> What do you think?
Alternatively, since tcf_exts_destroy() now takes NULL, and so
obviously does kfree() - perhaps tcf_action_destroy() should
return early if actions are NULL?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: sched: fix possible crash in tcf_action_destroy()
2019-09-22 2:08 ` Jakub Kicinski
@ 2019-09-23 15:44 ` Eric Dumazet
2019-09-23 21:10 ` Cong Wang
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2019-09-23 15:44 UTC (permalink / raw)
To: Jakub Kicinski, Cong Wang, Eric Dumazet
Cc: Eric Dumazet, David S . Miller, netdev, syzbot, Vlad Buslov, Jiri Pirko
On 9/21/19 7:08 PM, Jakub Kicinski wrote:
> On Wed, 18 Sep 2019 14:37:21 -0700, Cong Wang wrote:
>> On Wed, Sep 18, 2019 at 12:57 PM 'Eric Dumazet' via syzkaller
>> <syzkaller@googlegroups.com> wrote:
>>>
>>> If the allocation done in tcf_exts_init() failed,
>>> we end up with a NULL pointer in exts->actions.
>> ...
>>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>>> index efd3cfb80a2ad775dc8ab3c4900bd73d52c7aaad..9aef93300f1c11791acbb9262dfe77996872eafe 100644
>>> --- a/net/sched/cls_api.c
>>> +++ b/net/sched/cls_api.c
>>> @@ -3027,8 +3027,10 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
>>> void tcf_exts_destroy(struct tcf_exts *exts)
>>> {
>>> #ifdef CONFIG_NET_CLS_ACT
>>> - tcf_action_destroy(exts->actions, TCA_ACT_UNBIND);
>>> - kfree(exts->actions);
>>> + if (exts->actions) {
>>
>> I think it is _slightly_ better to check exts->nr_actions!=0 here,
>> as it would help exts->actions!=NULL&& exts->nr_actions==0
>> cases too.
>>
>> What do you think?
>
> Alternatively, since tcf_exts_destroy() now takes NULL, and so
> obviously does kfree() - perhaps tcf_action_destroy() should
> return early if actions are NULL?
>
I do not have any preference really, this is slow path and was trying to
fix a crash.
tcf_action_destroy() makes me nervous, since it seems to be able to break its loop
in case __tcf_idr_release() returns an error. This means that some actions will
never be release.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: sched: fix possible crash in tcf_action_destroy()
2019-09-23 15:44 ` Eric Dumazet
@ 2019-09-23 21:10 ` Cong Wang
0 siblings, 0 replies; 6+ messages in thread
From: Cong Wang @ 2019-09-23 21:10 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jakub Kicinski, Eric Dumazet, David S . Miller, netdev, syzbot,
Vlad Buslov, Jiri Pirko
On Mon, Sep 23, 2019 at 8:44 AM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 9/21/19 7:08 PM, Jakub Kicinski wrote:
> > On Wed, 18 Sep 2019 14:37:21 -0700, Cong Wang wrote:
> >> On Wed, Sep 18, 2019 at 12:57 PM 'Eric Dumazet' via syzkaller
> >> <syzkaller@googlegroups.com> wrote:
> >>>
> >>> If the allocation done in tcf_exts_init() failed,
> >>> we end up with a NULL pointer in exts->actions.
> >> ...
> >>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> >>> index efd3cfb80a2ad775dc8ab3c4900bd73d52c7aaad..9aef93300f1c11791acbb9262dfe77996872eafe 100644
> >>> --- a/net/sched/cls_api.c
> >>> +++ b/net/sched/cls_api.c
> >>> @@ -3027,8 +3027,10 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
> >>> void tcf_exts_destroy(struct tcf_exts *exts)
> >>> {
> >>> #ifdef CONFIG_NET_CLS_ACT
> >>> - tcf_action_destroy(exts->actions, TCA_ACT_UNBIND);
> >>> - kfree(exts->actions);
> >>> + if (exts->actions) {
> >>
> >> I think it is _slightly_ better to check exts->nr_actions!=0 here,
> >> as it would help exts->actions!=NULL&& exts->nr_actions==0
> >> cases too.
> >>
> >> What do you think?
> >
> > Alternatively, since tcf_exts_destroy() now takes NULL, and so
> > obviously does kfree() - perhaps tcf_action_destroy() should
> > return early if actions are NULL?
> >
>
> I do not have any preference really, this is slow path and was trying to
> fix a crash.
>
> tcf_action_destroy() makes me nervous, since it seems to be able to break its loop
> in case __tcf_idr_release() returns an error. This means that some actions will
> never be release.
Good point. Seems we can just continue the loop even when
-EPERM is returned, there is in fact no harm to leave those still
bound to filters there until the filers release them. Not sure if we
should still propagate -EPERM to users in this partially failure
case.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: sched: fix possible crash in tcf_action_destroy()
2019-09-18 19:57 [PATCH net] net: sched: fix possible crash in tcf_action_destroy() Eric Dumazet
2019-09-18 21:37 ` Cong Wang
@ 2019-09-24 14:36 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2019-09-24 14:36 UTC (permalink / raw)
To: edumazet; +Cc: netdev, eric.dumazet, syzkaller, vladbu, jiri
From: Eric Dumazet <edumazet@google.com>
Date: Wed, 18 Sep 2019 12:57:04 -0700
> If the allocation done in tcf_exts_init() failed,
> we end up with a NULL pointer in exts->actions.
...
> Fixes: 90b73b77d08e ("net: sched: change action API to use array of pointers to actions")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
Applied and queued up for -stable.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-09-24 14:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-18 19:57 [PATCH net] net: sched: fix possible crash in tcf_action_destroy() Eric Dumazet
2019-09-18 21:37 ` Cong Wang
2019-09-22 2:08 ` Jakub Kicinski
2019-09-23 15:44 ` Eric Dumazet
2019-09-23 21:10 ` Cong Wang
2019-09-24 14:36 ` 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).