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@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
>
--
Thanks,
Steve
