From: Roi Dayan <roid@mellanox.com>
To: Vlad Buslov <vladbu@mellanox.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"dcaratti@redhat.com" <dcaratti@redhat.com>
Cc: "jhs@mojatatu.com" <jhs@mojatatu.com>,
"xiyou.wangcong@gmail.com" <xiyou.wangcong@gmail.com>,
"jiri@resnulli.us" <jiri@resnulli.us>,
"davem@davemloft.net" <davem@davemloft.net>,
"wenxu@ucloud.cn" <wenxu@ucloud.cn>
Subject: Re: [PATCH net-next v2] net: sched: act_tunnel_key: fix metadata handling
Date: Wed, 27 Feb 2019 08:14:24 +0000 [thread overview]
Message-ID: <c0b30b6d-4647-2087-5684-0033c2ff6539@mellanox.com> (raw)
In-Reply-To: <20190225153014.8885-1-vladbu@mellanox.com>
On 25/02/2019 17:30, Vlad Buslov wrote:
> Tunnel key action params->tcft_enc_metadata is only set when action is
> TCA_TUNNEL_KEY_ACT_SET. However, metadata pointer is incorrectly
> dereferenced during tunnel key init and release without verifying that
> action is if correct type, which causes NULL pointer dereference. Metadata
> tunnel dst_cache is also leaked on action overwrite.
>
> Fix metadata handling:
> - Verify that metadata pointer is not NULL before dereferencing it in
> tunnel_key_init error handling code.
> - Move dst_cache destroy code into tunnel_key_release_params() function
> that is called in both action overwrite and release cases (fixes resource
> leak) and verifies that actions has correct type before dereferencing
> metadata pointer (fixes NULL pointer dereference).
>
> Oops with KASAN enabled during tdc tests execution:
>
> [ 261.080482] ==================================================================
> [ 261.088049] BUG: KASAN: null-ptr-deref in dst_cache_destroy+0x21/0xa0
> [ 261.094613] Read of size 8 at addr 00000000000000b0 by task tc/2976
> [ 261.102524] CPU: 14 PID: 2976 Comm: tc Not tainted 5.0.0-rc7+ #157
> [ 261.108844] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017
> [ 261.116726] Call Trace:
> [ 261.119234] dump_stack+0x9a/0xeb
> [ 261.122625] ? dst_cache_destroy+0x21/0xa0
> [ 261.126818] ? dst_cache_destroy+0x21/0xa0
> [ 261.131004] kasan_report+0x176/0x192
> [ 261.134752] ? idr_get_next+0xd0/0x120
> [ 261.138578] ? dst_cache_destroy+0x21/0xa0
> [ 261.142768] dst_cache_destroy+0x21/0xa0
> [ 261.146799] tunnel_key_release+0x3a/0x50 [act_tunnel_key]
> [ 261.152392] tcf_action_cleanup+0x2c/0xc0
> [ 261.156490] tcf_generic_walker+0x4c2/0x5c0
> [ 261.160794] ? tcf_action_dump_1+0x390/0x390
> [ 261.165163] ? tunnel_key_walker+0x5/0x1a0 [act_tunnel_key]
> [ 261.170865] ? tunnel_key_walker+0xe9/0x1a0 [act_tunnel_key]
> [ 261.176641] tca_action_gd+0x600/0xa40
> [ 261.180482] ? tca_get_fill.constprop.17+0x200/0x200
> [ 261.185548] ? __lock_acquire+0x588/0x1d20
> [ 261.189741] ? __lock_acquire+0x588/0x1d20
> [ 261.193922] ? mark_held_locks+0x90/0x90
> [ 261.197944] ? mark_held_locks+0x90/0x90
> [ 261.202018] ? __nla_parse+0xfe/0x190
> [ 261.205774] tc_ctl_action+0x218/0x230
> [ 261.209614] ? tcf_action_add+0x230/0x230
> [ 261.213726] rtnetlink_rcv_msg+0x3a5/0x600
> [ 261.217910] ? lock_downgrade+0x2d0/0x2d0
> [ 261.222006] ? validate_linkmsg+0x400/0x400
> [ 261.226278] ? find_held_lock+0x6d/0xd0
> [ 261.230200] ? match_held_lock+0x1b/0x210
> [ 261.234296] ? validate_linkmsg+0x400/0x400
> [ 261.238567] netlink_rcv_skb+0xc7/0x1f0
> [ 261.242489] ? netlink_ack+0x470/0x470
> [ 261.246319] ? netlink_deliver_tap+0x1f3/0x5a0
> [ 261.250874] netlink_unicast+0x2ae/0x350
> [ 261.254884] ? netlink_attachskb+0x340/0x340
> [ 261.261647] ? _copy_from_iter_full+0xdd/0x380
> [ 261.268576] ? __virt_addr_valid+0xb6/0xf0
> [ 261.275227] ? __check_object_size+0x159/0x240
> [ 261.282184] netlink_sendmsg+0x4d3/0x630
> [ 261.288572] ? netlink_unicast+0x350/0x350
> [ 261.295132] ? netlink_unicast+0x350/0x350
> [ 261.301608] sock_sendmsg+0x6d/0x80
> [ 261.307467] ___sys_sendmsg+0x48e/0x540
> [ 261.313633] ? copy_msghdr_from_user+0x210/0x210
> [ 261.320545] ? save_stack+0x89/0xb0
> [ 261.326289] ? __lock_acquire+0x588/0x1d20
> [ 261.332605] ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 261.340063] ? mark_held_locks+0x90/0x90
> [ 261.346162] ? do_filp_open+0x138/0x1d0
> [ 261.352108] ? may_open_dev+0x50/0x50
> [ 261.357897] ? match_held_lock+0x1b/0x210
> [ 261.364016] ? __fget_light+0xa6/0xe0
> [ 261.369840] ? __sys_sendmsg+0xd2/0x150
> [ 261.375814] __sys_sendmsg+0xd2/0x150
> [ 261.381610] ? __ia32_sys_shutdown+0x30/0x30
> [ 261.388026] ? lock_downgrade+0x2d0/0x2d0
> [ 261.394182] ? mark_held_locks+0x1c/0x90
> [ 261.400230] ? do_syscall_64+0x1e/0x280
> [ 261.406172] do_syscall_64+0x78/0x280
> [ 261.411932] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 261.419103] RIP: 0033:0x7f28e91a8b87
> [ 261.424791] Code: 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 80 00 00 00 00 8b 05 6a 2b 2c 00 48 63 d2 48 63 ff 85 c0 75 18 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 59 f3 c3 0f 1f 80 00 00 00 00 53 48 89 f3 48
> [ 261.448226] RSP: 002b:00007ffdc5c4e2d8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> [ 261.458183] RAX: ffffffffffffffda RBX: 000000005c73c202 RCX: 00007f28e91a8b87
> [ 261.467728] RDX: 0000000000000000 RSI: 00007ffdc5c4e340 RDI: 0000000000000003
> [ 261.477342] RBP: 0000000000000000 R08: 0000000000000001 R09: 000000000000000c
> [ 261.486970] R10: 000000000000000c R11: 0000000000000246 R12: 0000000000000001
> [ 261.496599] R13: 000000000067b4e0 R14: 00007ffdc5c5248c R15: 00007ffdc5c52480
> [ 261.506281] ==================================================================
> [ 261.516076] Disabling lock debugging due to kernel taint
> [ 261.523979] BUG: unable to handle kernel NULL pointer dereference at 00000000000000b0
> [ 261.534413] #PF error: [normal kernel read fault]
> [ 261.541730] PGD 8000000317400067 P4D 8000000317400067 PUD 316878067 PMD 0
> [ 261.551294] Oops: 0000 [#1] SMP KASAN PTI
> [ 261.557985] CPU: 14 PID: 2976 Comm: tc Tainted: G B 5.0.0-rc7+ #157
> [ 261.568306] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017
> [ 261.578874] RIP: 0010:dst_cache_destroy+0x21/0xa0
> [ 261.586413] Code: f4 ff ff ff eb f6 0f 1f 00 0f 1f 44 00 00 41 56 41 55 49 c7 c6 60 fe 35 af 41 54 55 49 89 fc 53 bd ff ff ff ff e8 ef 98 73 ff <49> 83 3c 24 00 75 35 eb 6c 4c 63 ed e8 de 98 73 ff 4a 8d 3c ed 40
> [ 261.611247] RSP: 0018:ffff888316447160 EFLAGS: 00010282
> [ 261.619564] RAX: 0000000000000000 RBX: ffff88835b3e2f00 RCX: ffffffffad1c5071
> [ 261.629862] RDX: 0000000000000003 RSI: dffffc0000000000 RDI: 0000000000000297
> [ 261.640149] RBP: 00000000ffffffff R08: fffffbfff5dd4e89 R09: fffffbfff5dd4e89
> [ 261.650467] R10: 0000000000000001 R11: fffffbfff5dd4e88 R12: 00000000000000b0
> [ 261.660785] R13: ffff8883267a10c0 R14: ffffffffaf35fe60 R15: 0000000000000001
> [ 261.671110] FS: 00007f28ea3e6400(0000) GS:ffff888364200000(0000) knlGS:0000000000000000
> [ 261.682447] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 261.691491] CR2: 00000000000000b0 CR3: 00000003178ae004 CR4: 00000000001606e0
> [ 261.701283] Call Trace:
> [ 261.706374] tunnel_key_release+0x3a/0x50 [act_tunnel_key]
> [ 261.714522] tcf_action_cleanup+0x2c/0xc0
> [ 261.721208] tcf_generic_walker+0x4c2/0x5c0
> [ 261.728074] ? tcf_action_dump_1+0x390/0x390
> [ 261.734996] ? tunnel_key_walker+0x5/0x1a0 [act_tunnel_key]
> [ 261.743247] ? tunnel_key_walker+0xe9/0x1a0 [act_tunnel_key]
> [ 261.751557] tca_action_gd+0x600/0xa40
> [ 261.757991] ? tca_get_fill.constprop.17+0x200/0x200
> [ 261.765644] ? __lock_acquire+0x588/0x1d20
> [ 261.772461] ? __lock_acquire+0x588/0x1d20
> [ 261.779266] ? mark_held_locks+0x90/0x90
> [ 261.785880] ? mark_held_locks+0x90/0x90
> [ 261.792470] ? __nla_parse+0xfe/0x190
> [ 261.798738] tc_ctl_action+0x218/0x230
> [ 261.805145] ? tcf_action_add+0x230/0x230
> [ 261.811760] rtnetlink_rcv_msg+0x3a5/0x600
> [ 261.818564] ? lock_downgrade+0x2d0/0x2d0
> [ 261.825433] ? validate_linkmsg+0x400/0x400
> [ 261.832256] ? find_held_lock+0x6d/0xd0
> [ 261.838624] ? match_held_lock+0x1b/0x210
> [ 261.845142] ? validate_linkmsg+0x400/0x400
> [ 261.851729] netlink_rcv_skb+0xc7/0x1f0
> [ 261.857976] ? netlink_ack+0x470/0x470
> [ 261.864132] ? netlink_deliver_tap+0x1f3/0x5a0
> [ 261.870969] netlink_unicast+0x2ae/0x350
> [ 261.877294] ? netlink_attachskb+0x340/0x340
> [ 261.883962] ? _copy_from_iter_full+0xdd/0x380
> [ 261.890750] ? __virt_addr_valid+0xb6/0xf0
> [ 261.897188] ? __check_object_size+0x159/0x240
> [ 261.903928] netlink_sendmsg+0x4d3/0x630
> [ 261.910112] ? netlink_unicast+0x350/0x350
> [ 261.916410] ? netlink_unicast+0x350/0x350
> [ 261.922656] sock_sendmsg+0x6d/0x80
> [ 261.928257] ___sys_sendmsg+0x48e/0x540
> [ 261.934183] ? copy_msghdr_from_user+0x210/0x210
> [ 261.940865] ? save_stack+0x89/0xb0
> [ 261.946355] ? __lock_acquire+0x588/0x1d20
> [ 261.952358] ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 261.959468] ? mark_held_locks+0x90/0x90
> [ 261.965248] ? do_filp_open+0x138/0x1d0
> [ 261.970910] ? may_open_dev+0x50/0x50
> [ 261.976386] ? match_held_lock+0x1b/0x210
> [ 261.982210] ? __fget_light+0xa6/0xe0
> [ 261.987648] ? __sys_sendmsg+0xd2/0x150
> [ 261.993263] __sys_sendmsg+0xd2/0x150
> [ 261.998613] ? __ia32_sys_shutdown+0x30/0x30
> [ 262.004555] ? lock_downgrade+0x2d0/0x2d0
> [ 262.010236] ? mark_held_locks+0x1c/0x90
> [ 262.015758] ? do_syscall_64+0x1e/0x280
> [ 262.021234] do_syscall_64+0x78/0x280
> [ 262.026500] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 262.033207] RIP: 0033:0x7f28e91a8b87
> [ 262.038421] Code: 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 80 00 00 00 00 8b 05 6a 2b 2c 00 48 63 d2 48 63 ff 85 c0 75 18 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 59 f3 c3 0f 1f 80 00 00 00 00 53 48 89 f3 48
> [ 262.060708] RSP: 002b:00007ffdc5c4e2d8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> [ 262.070112] RAX: ffffffffffffffda RBX: 000000005c73c202 RCX: 00007f28e91a8b87
> [ 262.079087] RDX: 0000000000000000 RSI: 00007ffdc5c4e340 RDI: 0000000000000003
> [ 262.088122] RBP: 0000000000000000 R08: 0000000000000001 R09: 000000000000000c
> [ 262.097157] R10: 000000000000000c R11: 0000000000000246 R12: 0000000000000001
> [ 262.106207] R13: 000000000067b4e0 R14: 00007ffdc5c5248c R15: 00007ffdc5c52480
> [ 262.115271] Modules linked in: act_tunnel_key act_skbmod act_simple act_connmark nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 act_csum libcrc32c act_meta_skbtcindex act_meta_skbprio act_meta_mark act_ife ife act_police act_sample psample act_gact veth nfsv3 nfs_acl nfs lockd grace fscache bridge stp llc intel_rapl sb_edac mlx5_ib x86_pkg_temp_thermal sunrpc intel_powerclamp coretemp ib_uverbs kvm_intel ib_core kvm irqbypass mlx5_core crct10dif_pclmul crc32_pclmul crc32c_intel igb ghash_clmulni_intel intel_cstate mlxfw iTCO_wdt devlink intel_uncore iTCO_vendor_support ipmi_ssif ptp mei_me intel_rapl_perf ioatdma joydev pps_core ses mei i2c_i801 pcspkr enclosure lpc_ich dca wmi ipmi_si ipmi_devintf ipmi_msghandler acpi_pad acpi_power_meter pcc_cpufreq ast i2c_algo_bit drm_kms_helper ttm drm mpt3sas raid_class scsi_transport_sas
> [ 262.204393] CR2: 00000000000000b0
> [ 262.210390] ---[ end trace 2e41d786f2c7901a ]---
> [ 262.226790] RIP: 0010:dst_cache_destroy+0x21/0xa0
> [ 262.234083] Code: f4 ff ff ff eb f6 0f 1f 00 0f 1f 44 00 00 41 56 41 55 49 c7 c6 60 fe 35 af 41 54 55 49 89 fc 53 bd ff ff ff ff e8 ef 98 73 ff <49> 83 3c 24 00 75 35 eb 6c 4c 63 ed e8 de 98 73 ff 4a 8d 3c ed 40
> [ 262.258311] RSP: 0018:ffff888316447160 EFLAGS: 00010282
> [ 262.266304] RAX: 0000000000000000 RBX: ffff88835b3e2f00 RCX: ffffffffad1c5071
> [ 262.276251] RDX: 0000000000000003 RSI: dffffc0000000000 RDI: 0000000000000297
> [ 262.286208] RBP: 00000000ffffffff R08: fffffbfff5dd4e89 R09: fffffbfff5dd4e89
> [ 262.296183] R10: 0000000000000001 R11: fffffbfff5dd4e88 R12: 00000000000000b0
> [ 262.306157] R13: ffff8883267a10c0 R14: ffffffffaf35fe60 R15: 0000000000000001
> [ 262.316139] FS: 00007f28ea3e6400(0000) GS:ffff888364200000(0000) knlGS:0000000000000000
> [ 262.327146] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 262.335815] CR2: 00000000000000b0 CR3: 00000003178ae004 CR4: 00000000001606e0
>
> Fixes: 41411e2fd6b8 ("net/sched: act_tunnel_key: Add dst_cache support")
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> ---
> Changes from V1 to V2:
> - Extract metadata->dst error handler fix into standalone patch that targets
> net.
>
> net/sched/act_tunnel_key.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
> index 3404af72b4c1..fc2b884b170c 100644
> --- a/net/sched/act_tunnel_key.c
> +++ b/net/sched/act_tunnel_key.c
> @@ -201,8 +201,14 @@ static void tunnel_key_release_params(struct tcf_tunnel_key_params *p)
> {
> if (!p)
> return;
> - if (p->tcft_action == TCA_TUNNEL_KEY_ACT_SET)
> + if (p->tcft_action == TCA_TUNNEL_KEY_ACT_SET) {
> +#ifdef CONFIG_DST_CACHE
> + struct ip_tunnel_info *info = &p->tcft_enc_metadata->u.tun_info;
> +
> + dst_cache_destroy(&info->dst_cache);
> +#endif
> dst_release(&p->tcft_enc_metadata->dst);
> + }
> kfree_rcu(p, rcu);
> }
>
> @@ -384,7 +390,8 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
>
> release_dst_cache:
> #ifdef CONFIG_DST_CACHE
> - dst_cache_destroy(&metadata->u.tun_info.dst_cache);
> + if (metadata)
> + dst_cache_destroy(&metadata->u.tun_info.dst_cache);
> #endif
> release_tun_meta:
> dst_release(&metadata->dst);
> @@ -401,15 +408,8 @@ static void tunnel_key_release(struct tc_action *a)
> {
> struct tcf_tunnel_key *t = to_tunnel_key(a);
> struct tcf_tunnel_key_params *params;
> -#ifdef CONFIG_DST_CACHE
> - struct ip_tunnel_info *info;
> -#endif
>
> params = rcu_dereference_protected(t->params, 1);
> -#ifdef CONFIG_DST_CACHE
> - info = ¶ms->tcft_enc_metadata->u.tun_info;
> - dst_cache_destroy(&info->dst_cache);
> -#endif
> tunnel_key_release_params(params);
> }
>
>
Reviewed-by: Roi Dayan <roid@mellanox.com>
next prev parent reply other threads:[~2019-02-27 8:14 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-25 12:21 [PATCH net-next] net: sched: act_tunnel_key: fix metadata handling Vlad Buslov
2019-02-25 14:04 ` Davide Caratti
2019-02-25 15:12 ` Vlad Buslov
2019-02-25 15:28 ` [PATCH net] net: sched: act_tunnel_key: fix NULL pointer dereference during init Vlad Buslov
2019-02-25 15:36 ` Davide Caratti
2019-02-25 18:16 ` David Miller
2019-02-25 15:30 ` [PATCH net-next v2] net: sched: act_tunnel_key: fix metadata handling Vlad Buslov
2019-02-27 8:14 ` Roi Dayan [this message]
2019-02-28 5:36 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c0b30b6d-4647-2087-5684-0033c2ff6539@mellanox.com \
--to=roid@mellanox.com \
--cc=davem@davemloft.net \
--cc=dcaratti@redhat.com \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=netdev@vger.kernel.org \
--cc=vladbu@mellanox.com \
--cc=wenxu@ucloud.cn \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).