| Message ID | 20221118084208.3214951-3-zhangxiaoxu5@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: Fix resource leak when MR allocate failed | expand |
Dave Howells pointed out that around this line (2246 of fs/cifs/smbdirect.c)
list_add_tail(&smbdirect_mr->list, &info->mr_list);
shouldn't there be locking on that?
On Fri, Nov 18, 2022 at 1:37 AM Zhang Xiaoxu <zhangxiaoxu5@huawei.com> wrote:
>
> If the MR allocate failed, the MR recovery work not initialized
> and list not cleared. Then will be warning and UAF when release
> the MR:
>
> WARNING: CPU: 4 PID: 824 at kernel/workqueue.c:3066 __flush_work.isra.0+0xf7/0x110
> CPU: 4 PID: 824 Comm: mount.cifs Not tainted 6.1.0-rc5+ #82
> RIP: 0010:__flush_work.isra.0+0xf7/0x110
> Call Trace:
> <TASK>
> __cancel_work_timer+0x2ba/0x2e0
> smbd_destroy+0x4e1/0x990
> _smbd_get_connection+0x1cbd/0x2110
> smbd_get_connection+0x21/0x40
> cifs_get_tcp_session+0x8ef/0xda0
> mount_get_conns+0x60/0x750
> cifs_mount+0x103/0xd00
> cifs_smb3_do_mount+0x1dd/0xcb0
> smb3_get_tree+0x1d5/0x300
> vfs_get_tree+0x41/0xf0
> path_mount+0x9b3/0xdd0
> __x64_sys_mount+0x190/0x1d0
> do_syscall_64+0x35/0x80
> entry_SYSCALL_64_after_hwframe+0x46/0xb0
>
> BUG: KASAN: use-after-free in smbd_destroy+0x4fc/0x990
> Read of size 8 at addr ffff88810b156a08 by task mount.cifs/824
> CPU: 4 PID: 824 Comm: mount.cifs Tainted: G W 6.1.0-rc5+ #82
> Call Trace:
> dump_stack_lvl+0x34/0x44
> print_report+0x171/0x472
> kasan_report+0xad/0x130
> smbd_destroy+0x4fc/0x990
> _smbd_get_connection+0x1cbd/0x2110
> smbd_get_connection+0x21/0x40
> cifs_get_tcp_session+0x8ef/0xda0
> mount_get_conns+0x60/0x750
> cifs_mount+0x103/0xd00
> cifs_smb3_do_mount+0x1dd/0xcb0
> smb3_get_tree+0x1d5/0x300
> vfs_get_tree+0x41/0xf0
> path_mount+0x9b3/0xdd0
> __x64_sys_mount+0x190/0x1d0
> do_syscall_64+0x35/0x80
> entry_SYSCALL_64_after_hwframe+0x46/0xb0
>
> Allocated by task 824:
> kasan_save_stack+0x1e/0x40
> kasan_set_track+0x21/0x30
> __kasan_kmalloc+0x7a/0x90
> _smbd_get_connection+0x1b6f/0x2110
> smbd_get_connection+0x21/0x40
> cifs_get_tcp_session+0x8ef/0xda0
> mount_get_conns+0x60/0x750
> cifs_mount+0x103/0xd00
> cifs_smb3_do_mount+0x1dd/0xcb0
> smb3_get_tree+0x1d5/0x300
> vfs_get_tree+0x41/0xf0
> path_mount+0x9b3/0xdd0
> __x64_sys_mount+0x190/0x1d0
> do_syscall_64+0x35/0x80
> entry_SYSCALL_64_after_hwframe+0x46/0xb0
>
> Freed by task 824:
> kasan_save_stack+0x1e/0x40
> kasan_set_track+0x21/0x30
> kasan_save_free_info+0x2a/0x40
> ____kasan_slab_free+0x143/0x1b0
> __kmem_cache_free+0xc8/0x330
> _smbd_get_connection+0x1c6a/0x2110
> smbd_get_connection+0x21/0x40
> cifs_get_tcp_session+0x8ef/0xda0
> mount_get_conns+0x60/0x750
> cifs_mount+0x103/0xd00
> cifs_smb3_do_mount+0x1dd/0xcb0
> smb3_get_tree+0x1d5/0x300
> vfs_get_tree+0x41/0xf0
> path_mount+0x9b3/0xdd0
> __x64_sys_mount+0x190/0x1d0
> do_syscall_64+0x35/0x80
> entry_SYSCALL_64_after_hwframe+0x46/0xb0
>
> Let's initialize the MR recovery work before MR allocate to prevent
> the warning, remove the MRs from the list to prevent the UAF.
>
> Fixes: c7398583340a ("CIFS: SMBD: Implement RDMA memory registration")
> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> ---
> fs/cifs/smbdirect.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> index a874c2e1ae41..7013fdb4ea51 100644
> --- a/fs/cifs/smbdirect.c
> +++ b/fs/cifs/smbdirect.c
> @@ -2217,6 +2217,7 @@ static int allocate_mr_list(struct smbd_connection *info)
> atomic_set(&info->mr_ready_count, 0);
> atomic_set(&info->mr_used_count, 0);
> init_waitqueue_head(&info->wait_for_mr_cleanup);
> + INIT_WORK(&info->mr_recovery_work, smbd_mr_recovery_work);
> /* Allocate more MRs (2x) than hardware responder_resources */
> for (i = 0; i < info->responder_resources * 2; i++) {
> smbdirect_mr = kzalloc(sizeof(*smbdirect_mr), GFP_KERNEL);
> @@ -2244,13 +2245,13 @@ static int allocate_mr_list(struct smbd_connection *info)
> list_add_tail(&smbdirect_mr->list, &info->mr_list);
> atomic_inc(&info->mr_ready_count);
> }
> - INIT_WORK(&info->mr_recovery_work, smbd_mr_recovery_work);
> return 0;
>
> out:
> kfree(smbdirect_mr);
>
> list_for_each_entry_safe(smbdirect_mr, tmp, &info->mr_list, list) {
> + list_del(&smbdirect_mr->list);
> ib_dereg_mr(smbdirect_mr->mr);
> kfree(smbdirect_mr->sgl);
> kfree(smbdirect_mr);
> --
> 2.31.1
>
On 2/17/2023 4:15 PM, Steve French wrote: > Dave Howells pointed out that around this line (2246 of fs/cifs/smbdirect.c) > > list_add_tail(&smbdirect_mr->list, &info->mr_list); > > shouldn't there be locking on that? I don't think it's necessary, because neither the smbdirect_mr nor the smbd_connection ("info") have been linked anywhere yet. Regarding the proposed patch: > On Fri, Nov 18, 2022 at 1:37 AM Zhang Xiaoxu <zhangxiaoxu5@huawei.com> wrote: >> >> If the MR allocate failed, the MR recovery work not initialized >> and list not cleared. Then will be warning and UAF when release >> the MR: Reviewed-by: Tom Talpey <tom@talpey.com> >> >> WARNING: CPU: 4 PID: 824 at kernel/workqueue.c:3066 __flush_work.isra.0+0xf7/0x110 >> CPU: 4 PID: 824 Comm: mount.cifs Not tainted 6.1.0-rc5+ #82 >> RIP: 0010:__flush_work.isra.0+0xf7/0x110 >> Call Trace: >> <TASK> >> __cancel_work_timer+0x2ba/0x2e0 >> smbd_destroy+0x4e1/0x990 >> _smbd_get_connection+0x1cbd/0x2110 >> smbd_get_connection+0x21/0x40 >> cifs_get_tcp_session+0x8ef/0xda0 >> mount_get_conns+0x60/0x750 >> cifs_mount+0x103/0xd00 >> cifs_smb3_do_mount+0x1dd/0xcb0 >> smb3_get_tree+0x1d5/0x300 >> vfs_get_tree+0x41/0xf0 >> path_mount+0x9b3/0xdd0 >> __x64_sys_mount+0x190/0x1d0 >> do_syscall_64+0x35/0x80 >> entry_SYSCALL_64_after_hwframe+0x46/0xb0 >> >> BUG: KASAN: use-after-free in smbd_destroy+0x4fc/0x990 >> Read of size 8 at addr ffff88810b156a08 by task mount.cifs/824 >> CPU: 4 PID: 824 Comm: mount.cifs Tainted: G W 6.1.0-rc5+ #82 >> Call Trace: >> dump_stack_lvl+0x34/0x44 >> print_report+0x171/0x472 >> kasan_report+0xad/0x130 >> smbd_destroy+0x4fc/0x990 >> _smbd_get_connection+0x1cbd/0x2110 >> smbd_get_connection+0x21/0x40 >> cifs_get_tcp_session+0x8ef/0xda0 >> mount_get_conns+0x60/0x750 >> cifs_mount+0x103/0xd00 >> cifs_smb3_do_mount+0x1dd/0xcb0 >> smb3_get_tree+0x1d5/0x300 >> vfs_get_tree+0x41/0xf0 >> path_mount+0x9b3/0xdd0 >> __x64_sys_mount+0x190/0x1d0 >> do_syscall_64+0x35/0x80 >> entry_SYSCALL_64_after_hwframe+0x46/0xb0 >> >> Allocated by task 824: >> kasan_save_stack+0x1e/0x40 >> kasan_set_track+0x21/0x30 >> __kasan_kmalloc+0x7a/0x90 >> _smbd_get_connection+0x1b6f/0x2110 >> smbd_get_connection+0x21/0x40 >> cifs_get_tcp_session+0x8ef/0xda0 >> mount_get_conns+0x60/0x750 >> cifs_mount+0x103/0xd00 >> cifs_smb3_do_mount+0x1dd/0xcb0 >> smb3_get_tree+0x1d5/0x300 >> vfs_get_tree+0x41/0xf0 >> path_mount+0x9b3/0xdd0 >> __x64_sys_mount+0x190/0x1d0 >> do_syscall_64+0x35/0x80 >> entry_SYSCALL_64_after_hwframe+0x46/0xb0 >> >> Freed by task 824: >> kasan_save_stack+0x1e/0x40 >> kasan_set_track+0x21/0x30 >> kasan_save_free_info+0x2a/0x40 >> ____kasan_slab_free+0x143/0x1b0 >> __kmem_cache_free+0xc8/0x330 >> _smbd_get_connection+0x1c6a/0x2110 >> smbd_get_connection+0x21/0x40 >> cifs_get_tcp_session+0x8ef/0xda0 >> mount_get_conns+0x60/0x750 >> cifs_mount+0x103/0xd00 >> cifs_smb3_do_mount+0x1dd/0xcb0 >> smb3_get_tree+0x1d5/0x300 >> vfs_get_tree+0x41/0xf0 >> path_mount+0x9b3/0xdd0 >> __x64_sys_mount+0x190/0x1d0 >> do_syscall_64+0x35/0x80 >> entry_SYSCALL_64_after_hwframe+0x46/0xb0 >> >> Let's initialize the MR recovery work before MR allocate to prevent >> the warning, remove the MRs from the list to prevent the UAF. >> >> Fixes: c7398583340a ("CIFS: SMBD: Implement RDMA memory registration") >> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> >> --- >> fs/cifs/smbdirect.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c >> index a874c2e1ae41..7013fdb4ea51 100644 >> --- a/fs/cifs/smbdirect.c >> +++ b/fs/cifs/smbdirect.c >> @@ -2217,6 +2217,7 @@ static int allocate_mr_list(struct smbd_connection *info) >> atomic_set(&info->mr_ready_count, 0); >> atomic_set(&info->mr_used_count, 0); >> init_waitqueue_head(&info->wait_for_mr_cleanup); >> + INIT_WORK(&info->mr_recovery_work, smbd_mr_recovery_work); >> /* Allocate more MRs (2x) than hardware responder_resources */ >> for (i = 0; i < info->responder_resources * 2; i++) { >> smbdirect_mr = kzalloc(sizeof(*smbdirect_mr), GFP_KERNEL); >> @@ -2244,13 +2245,13 @@ static int allocate_mr_list(struct smbd_connection *info) >> list_add_tail(&smbdirect_mr->list, &info->mr_list); >> atomic_inc(&info->mr_ready_count); >> } >> - INIT_WORK(&info->mr_recovery_work, smbd_mr_recovery_work); >> return 0; >> >> out: >> kfree(smbdirect_mr); >> >> list_for_each_entry_safe(smbdirect_mr, tmp, &info->mr_list, list) { >> + list_del(&smbdirect_mr->list); >> ib_dereg_mr(smbdirect_mr->mr); >> kfree(smbdirect_mr->sgl); >> kfree(smbdirect_mr); >> -- >> 2.31.1 >> > >
diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c index a874c2e1ae41..7013fdb4ea51 100644 --- a/fs/cifs/smbdirect.c +++ b/fs/cifs/smbdirect.c @@ -2217,6 +2217,7 @@ static int allocate_mr_list(struct smbd_connection *info) atomic_set(&info->mr_ready_count, 0); atomic_set(&info->mr_used_count, 0); init_waitqueue_head(&info->wait_for_mr_cleanup); + INIT_WORK(&info->mr_recovery_work, smbd_mr_recovery_work); /* Allocate more MRs (2x) than hardware responder_resources */ for (i = 0; i < info->responder_resources * 2; i++) { smbdirect_mr = kzalloc(sizeof(*smbdirect_mr), GFP_KERNEL); @@ -2244,13 +2245,13 @@ static int allocate_mr_list(struct smbd_connection *info) list_add_tail(&smbdirect_mr->list, &info->mr_list); atomic_inc(&info->mr_ready_count); } - INIT_WORK(&info->mr_recovery_work, smbd_mr_recovery_work); return 0; out: kfree(smbdirect_mr); list_for_each_entry_safe(smbdirect_mr, tmp, &info->mr_list, list) { + list_del(&smbdirect_mr->list); ib_dereg_mr(smbdirect_mr->mr); kfree(smbdirect_mr->sgl); kfree(smbdirect_mr);
If the MR allocate failed, the MR recovery work not initialized and list not cleared. Then will be warning and UAF when release the MR: WARNING: CPU: 4 PID: 824 at kernel/workqueue.c:3066 __flush_work.isra.0+0xf7/0x110 CPU: 4 PID: 824 Comm: mount.cifs Not tainted 6.1.0-rc5+ #82 RIP: 0010:__flush_work.isra.0+0xf7/0x110 Call Trace: <TASK> __cancel_work_timer+0x2ba/0x2e0 smbd_destroy+0x4e1/0x990 _smbd_get_connection+0x1cbd/0x2110 smbd_get_connection+0x21/0x40 cifs_get_tcp_session+0x8ef/0xda0 mount_get_conns+0x60/0x750 cifs_mount+0x103/0xd00 cifs_smb3_do_mount+0x1dd/0xcb0 smb3_get_tree+0x1d5/0x300 vfs_get_tree+0x41/0xf0 path_mount+0x9b3/0xdd0 __x64_sys_mount+0x190/0x1d0 do_syscall_64+0x35/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 BUG: KASAN: use-after-free in smbd_destroy+0x4fc/0x990 Read of size 8 at addr ffff88810b156a08 by task mount.cifs/824 CPU: 4 PID: 824 Comm: mount.cifs Tainted: G W 6.1.0-rc5+ #82 Call Trace: dump_stack_lvl+0x34/0x44 print_report+0x171/0x472 kasan_report+0xad/0x130 smbd_destroy+0x4fc/0x990 _smbd_get_connection+0x1cbd/0x2110 smbd_get_connection+0x21/0x40 cifs_get_tcp_session+0x8ef/0xda0 mount_get_conns+0x60/0x750 cifs_mount+0x103/0xd00 cifs_smb3_do_mount+0x1dd/0xcb0 smb3_get_tree+0x1d5/0x300 vfs_get_tree+0x41/0xf0 path_mount+0x9b3/0xdd0 __x64_sys_mount+0x190/0x1d0 do_syscall_64+0x35/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 Allocated by task 824: kasan_save_stack+0x1e/0x40 kasan_set_track+0x21/0x30 __kasan_kmalloc+0x7a/0x90 _smbd_get_connection+0x1b6f/0x2110 smbd_get_connection+0x21/0x40 cifs_get_tcp_session+0x8ef/0xda0 mount_get_conns+0x60/0x750 cifs_mount+0x103/0xd00 cifs_smb3_do_mount+0x1dd/0xcb0 smb3_get_tree+0x1d5/0x300 vfs_get_tree+0x41/0xf0 path_mount+0x9b3/0xdd0 __x64_sys_mount+0x190/0x1d0 do_syscall_64+0x35/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 Freed by task 824: kasan_save_stack+0x1e/0x40 kasan_set_track+0x21/0x30 kasan_save_free_info+0x2a/0x40 ____kasan_slab_free+0x143/0x1b0 __kmem_cache_free+0xc8/0x330 _smbd_get_connection+0x1c6a/0x2110 smbd_get_connection+0x21/0x40 cifs_get_tcp_session+0x8ef/0xda0 mount_get_conns+0x60/0x750 cifs_mount+0x103/0xd00 cifs_smb3_do_mount+0x1dd/0xcb0 smb3_get_tree+0x1d5/0x300 vfs_get_tree+0x41/0xf0 path_mount+0x9b3/0xdd0 __x64_sys_mount+0x190/0x1d0 do_syscall_64+0x35/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 Let's initialize the MR recovery work before MR allocate to prevent the warning, remove the MRs from the list to prevent the UAF. Fixes: c7398583340a ("CIFS: SMBD: Implement RDMA memory registration") Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> --- fs/cifs/smbdirect.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)