| Message ID | 0e295e62358a68b22c646adece4272a9bd0473f8.1523620752.git.pabeni@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] team: avoid adding twice the same option to the event list | expand |
From: Paolo Abeni <pabeni@redhat.com> Date: Fri, 13 Apr 2018 13:59:25 +0200 > When parsing the options provided by the user space, > team_nl_cmd_options_set() insert them in a temporary list to send > multiple events with a single message. > While each option's attribute is correctly validated, the code does > not check for duplicate entries before inserting into the event > list. > > Exploiting the above, the syzbot was able to trigger the following > splat: ... > This changeset addresses the avoiding list_add() if the current > option is already present in the event list. > > Reported-and-tested-by: syzbot+4d4af685432dc0e56c91@syzkaller.appspotmail.com > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > Fixes: 2fcdb2c9e659 ("team: allow to send multiple set events in one message") Looks good to me. It's too bad that the tmp list entries don't get marked as they are added, or get unlinked by the list processor. Either scheme would make the "already added" test a lot simpler. Jiri, please review before I apply this. Thanks.
On Fri, 2018-04-13 at 14:07 -0400, David Miller wrote: > From: Paolo Abeni <pabeni@redhat.com> > Date: Fri, 13 Apr 2018 13:59:25 +0200 > > > When parsing the options provided by the user space, > > team_nl_cmd_options_set() insert them in a temporary list to send > > multiple events with a single message. > > While each option's attribute is correctly validated, the code does > > not check for duplicate entries before inserting into the event > > list. > > > > Exploiting the above, the syzbot was able to trigger the following > > splat: > ... > > This changeset addresses the avoiding list_add() if the current > > option is already present in the event list. > > > > Reported-and-tested-by: syzbot+4d4af685432dc0e56c91@syzkaller.appspotmail.com > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > Fixes: 2fcdb2c9e659 ("team: allow to send multiple set events in one message") > > Looks good to me. > > It's too bad that the tmp list entries don't get marked as they are > added, or get unlinked by the list processor. Either scheme would > make the "already added" test a lot simpler. Yes, I considered both changes, but than opted for this solution, beliving it would be less invasive and more suitable for -net. Cheers, Paolo
From: Paolo Abeni <pabeni@redhat.com> Date: Fri, 13 Apr 2018 13:59:25 +0200 > When parsing the options provided by the user space, > team_nl_cmd_options_set() insert them in a temporary list to send > multiple events with a single message. > While each option's attribute is correctly validated, the code does > not check for duplicate entries before inserting into the event > list. > > Exploiting the above, the syzbot was able to trigger the following > splat: ... > This changeset addresses the avoiding list_add() if the current > option is already present in the event list. > > Reported-and-tested-by: syzbot+4d4af685432dc0e56c91@syzkaller.appspotmail.com > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > Fixes: 2fcdb2c9e659 ("team: allow to send multiple set events in one message") Applied and queued up for -stable, thanks. Please put the Fixes: tag as the first tag next time. Thank you.
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c index a6c6ce19eeee..acbe84967834 100644 --- a/drivers/net/team/team.c +++ b/drivers/net/team/team.c @@ -261,6 +261,17 @@ static void __team_option_inst_mark_removed_port(struct team *team, } } +static bool __team_option_inst_tmp_find(const struct list_head *opts, + const struct team_option_inst *needle) +{ + struct team_option_inst *opt_inst; + + list_for_each_entry(opt_inst, opts, tmp_list) + if (opt_inst == needle) + return true; + return false; +} + static int __team_options_register(struct team *team, const struct team_option *option, size_t option_count) @@ -2568,6 +2579,14 @@ static int team_nl_cmd_options_set(struct sk_buff *skb, struct genl_info *info) if (err) goto team_put; opt_inst->changed = true; + + /* dumb/evil user-space can send us duplicate opt, + * keep only the last one + */ + if (__team_option_inst_tmp_find(&opt_inst_list, + opt_inst)) + continue; + list_add(&opt_inst->tmp_list, &opt_inst_list); } if (!opt_found) {
When parsing the options provided by the user space, team_nl_cmd_options_set() insert them in a temporary list to send multiple events with a single message. While each option's attribute is correctly validated, the code does not check for duplicate entries before inserting into the event list. Exploiting the above, the syzbot was able to trigger the following splat: kernel BUG at lib/list_debug.c:31! invalid opcode: 0000 [#1] SMP KASAN Dumping ftrace buffer: (ftrace buffer empty) Modules linked in: CPU: 0 PID: 4466 Comm: syzkaller556835 Not tainted 4.16.0+ #17 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:__list_add_valid+0xaa/0xb0 lib/list_debug.c:29 RSP: 0018:ffff8801b04bf248 EFLAGS: 00010286 RAX: 0000000000000058 RBX: ffff8801c8fc7a90 RCX: 0000000000000000 RDX: 0000000000000058 RSI: ffffffff815fbf41 RDI: ffffed0036097e3f RBP: ffff8801b04bf260 R08: ffff8801b0b2a700 R09: ffffed003b604f90 R10: ffffed003b604f90 R11: ffff8801db027c87 R12: ffff8801c8fc7a90 R13: ffff8801c8fc7a90 R14: dffffc0000000000 R15: 0000000000000000 FS: 0000000000b98880(0000) GS:ffff8801db000000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 000000000043fc30 CR3: 00000001afe8e000 CR4: 00000000001406f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: __list_add include/linux/list.h:60 [inline] list_add include/linux/list.h:79 [inline] team_nl_cmd_options_set+0x9ff/0x12b0 drivers/net/team/team.c:2571 genl_family_rcv_msg+0x889/0x1120 net/netlink/genetlink.c:599 genl_rcv_msg+0xc6/0x170 net/netlink/genetlink.c:624 netlink_rcv_skb+0x172/0x440 net/netlink/af_netlink.c:2448 genl_rcv+0x28/0x40 net/netlink/genetlink.c:635 netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline] netlink_unicast+0x58b/0x740 net/netlink/af_netlink.c:1336 netlink_sendmsg+0x9f0/0xfa0 net/netlink/af_netlink.c:1901 sock_sendmsg_nosec net/socket.c:629 [inline] sock_sendmsg+0xd5/0x120 net/socket.c:639 ___sys_sendmsg+0x805/0x940 net/socket.c:2117 __sys_sendmsg+0x115/0x270 net/socket.c:2155 SYSC_sendmsg net/socket.c:2164 [inline] SyS_sendmsg+0x29/0x30 net/socket.c:2162 do_syscall_64+0x29e/0x9d0 arch/x86/entry/common.c:287 entry_SYSCALL_64_after_hwframe+0x42/0xb7 RIP: 0033:0x4458b9 RSP: 002b:00007ffd1d4a7278 EFLAGS: 00000213 ORIG_RAX: 000000000000002e RAX: ffffffffffffffda RBX: 000000000000001b RCX: 00000000004458b9 RDX: 0000000000000010 RSI: 0000000020000d00 RDI: 0000000000000004 RBP: 00000000004a74ed R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000213 R12: 00007ffd1d4a7348 R13: 0000000000402a60 R14: 0000000000000000 R15: 0000000000000000 Code: 75 e8 eb a9 48 89 f7 48 89 75 e8 e8 d1 85 7b fe 48 8b 75 e8 eb bb 48 89 f2 48 89 d9 4c 89 e6 48 c7 c7 a0 84 d8 87 e8 ea 67 28 fe <0f> 0b 0f 1f 40 00 48 b8 00 00 00 00 00 fc ff df 55 48 89 e5 41 RIP: __list_add_valid+0xaa/0xb0 lib/list_debug.c:29 RSP: ffff8801b04bf248 This changeset addresses the avoiding list_add() if the current option is already present in the event list. Reported-and-tested-by: syzbot+4d4af685432dc0e56c91@syzkaller.appspotmail.com Signed-off-by: Paolo Abeni <pabeni@redhat.com> Fixes: 2fcdb2c9e659 ("team: allow to send multiple set events in one message") --- drivers/net/team/team.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)