Re: [PATCH 1/7] cifs: Fix use-after-free bug in cifs_reconnect()

[Steve French <smfrench@gmail.com>]

merged into cifs-2.6.git for-next pending testing and more reviews

On Fri, Nov 22, 2019 at 9:31 AM Paulo Alcantara (SUSE) <pc@cjr.nz> wrote:
>
> Ensure we grab an active reference in cifs superblock while doing
> failover to prevent automounts (DFS links) of expiring and then
> destroying the superblock pointer.
>
> This patch fixes the following KASAN report:
>
> [  464.301462] BUG: KASAN: use-after-free in
> cifs_reconnect+0x6ab/0x1350
> [  464.303052] Read of size 8 at addr ffff888155e580d0 by task
> cifsd/1107
>
> [  464.304682] CPU: 3 PID: 1107 Comm: cifsd Not tainted 5.4.0-rc4+ #13
> [  464.305552] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
> BIOS rel-1.12.1-0-ga5cab58-rebuilt.opensuse.org 04/01/2014
> [  464.307146] Call Trace:
> [  464.307875]  dump_stack+0x5b/0x90
> [  464.308631]  print_address_description.constprop.0+0x16/0x200
> [  464.309478]  ? cifs_reconnect+0x6ab/0x1350
> [  464.310253]  ? cifs_reconnect+0x6ab/0x1350
> [  464.311040]  __kasan_report.cold+0x1a/0x41
> [  464.311811]  ? cifs_reconnect+0x6ab/0x1350
> [  464.312563]  kasan_report+0xe/0x20
> [  464.313300]  cifs_reconnect+0x6ab/0x1350
> [  464.314062]  ? extract_hostname.part.0+0x90/0x90
> [  464.314829]  ? printk+0xad/0xde
> [  464.315525]  ? _raw_spin_lock+0x7c/0xd0
> [  464.316252]  ? _raw_read_lock_irq+0x40/0x40
> [  464.316961]  ? ___ratelimit+0xed/0x182
> [  464.317655]  cifs_readv_from_socket+0x289/0x3b0
> [  464.318386]  cifs_read_from_socket+0x98/0xd0
> [  464.319078]  ? cifs_readv_from_socket+0x3b0/0x3b0
> [  464.319782]  ? try_to_wake_up+0x43c/0xa90
> [  464.320463]  ? cifs_small_buf_get+0x4b/0x60
> [  464.321173]  ? allocate_buffers+0x98/0x1a0
> [  464.321856]  cifs_demultiplex_thread+0x218/0x14a0
> [  464.322558]  ? cifs_handle_standard+0x270/0x270
> [  464.323237]  ? __switch_to_asm+0x40/0x70
> [  464.323893]  ? __switch_to_asm+0x34/0x70
> [  464.324554]  ? __switch_to_asm+0x40/0x70
> [  464.325226]  ? __switch_to_asm+0x40/0x70
> [  464.325863]  ? __switch_to_asm+0x34/0x70
> [  464.326505]  ? __switch_to_asm+0x40/0x70
> [  464.327161]  ? __switch_to_asm+0x34/0x70
> [  464.327784]  ? finish_task_switch+0xa1/0x330
> [  464.328414]  ? __switch_to+0x363/0x640
> [  464.329044]  ? __schedule+0x575/0xaf0
> [  464.329655]  ? _raw_spin_lock_irqsave+0x82/0xe0
> [  464.330301]  kthread+0x1a3/0x1f0
> [  464.330884]  ? cifs_handle_standard+0x270/0x270
> [  464.331624]  ? kthread_create_on_node+0xd0/0xd0
> [  464.332347]  ret_from_fork+0x35/0x40
>
> [  464.333577] Allocated by task 1110:
> [  464.334381]  save_stack+0x1b/0x80
> [  464.335123]  __kasan_kmalloc.constprop.0+0xc2/0xd0
> [  464.335848]  cifs_smb3_do_mount+0xd4/0xb00
> [  464.336619]  legacy_get_tree+0x6b/0xa0
> [  464.337235]  vfs_get_tree+0x41/0x110
> [  464.337975]  fc_mount+0xa/0x40
> [  464.338557]  vfs_kern_mount.part.0+0x6c/0x80
> [  464.339227]  cifs_dfs_d_automount+0x336/0xd29
> [  464.339846]  follow_managed+0x1b1/0x450
> [  464.340449]  lookup_fast+0x231/0x4a0
> [  464.341039]  path_openat+0x240/0x1fd0
> [  464.341634]  do_filp_open+0x126/0x1c0
> [  464.342277]  do_sys_open+0x1eb/0x2c0
> [  464.342957]  do_syscall_64+0x5e/0x190
> [  464.343555]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> [  464.344772] Freed by task 0:
> [  464.345347]  save_stack+0x1b/0x80
> [  464.345966]  __kasan_slab_free+0x12c/0x170
> [  464.346576]  kfree+0xa6/0x270
> [  464.347211]  rcu_core+0x39c/0xc80
> [  464.347800]  __do_softirq+0x10d/0x3da
>
> [  464.348919] The buggy address belongs to the object at
> ffff888155e58000
>                 which belongs to the cache kmalloc-256 of size 256
> [  464.350222] The buggy address is located 208 bytes inside of
>                 256-byte region [ffff888155e58000, ffff888155e58100)
> [  464.351575] The buggy address belongs to the page:
> [  464.352333] page:ffffea0005579600 refcount:1 mapcount:0
> mapping:ffff88815a803400 index:0x0 compound_mapcount: 0
> [  464.353583] flags: 0x200000000010200(slab|head)
> [  464.354209] raw: 0200000000010200 ffffea0005576200 0000000400000004
> ffff88815a803400
> [  464.355353] raw: 0000000000000000 0000000080100010 00000001ffffffff
> 0000000000000000
> [  464.356458] page dumped because: kasan: bad access detected
>
> [  464.367005] Memory state around the buggy address:
> [  464.367787]  ffff888155e57f80: fc fc fc fc fc fc fc fc fc fc fc fc
> fc fc fc fc
> [  464.368877]  ffff888155e58000: fb fb fb fb fb fb fb fb fb fb fb fb
> fb fb fb fb
> [  464.369967] >ffff888155e58080: fb fb fb fb fb fb fb fb fb fb fb fb
> fb fb fb fb
> [  464.371111]                                                  ^
> [  464.371775]  ffff888155e58100: fc fc fc fc fc fc fc fc fc fc fc fc
> fc fc fc fc
> [  464.372893]  ffff888155e58180: fc fc fc fc fc fc fc fc fc fc fc fc
> fc fc fc fc
> [  464.373983] ==================================================================
>
> Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> ---
>  fs/cifs/connect.c | 46 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index fce71a6cc3f4..668d477cc9c7 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -391,7 +391,7 @@ static inline int reconn_set_ipaddr(struct TCP_Server_Info *server)
>  #ifdef CONFIG_CIFS_DFS_UPCALL
>  struct super_cb_data {
>         struct TCP_Server_Info *server;
> -       struct cifs_sb_info *cifs_sb;
> +       struct super_block *sb;
>  };
>
>  /* These functions must be called with server->srv_mutex held */
> @@ -402,25 +402,39 @@ static void super_cb(struct super_block *sb, void *arg)
>         struct cifs_sb_info *cifs_sb;
>         struct cifs_tcon *tcon;
>
> -       if (d->cifs_sb)
> +       if (d->sb)
>                 return;
>
>         cifs_sb = CIFS_SB(sb);
>         tcon = cifs_sb_master_tcon(cifs_sb);
>         if (tcon->ses->server == d->server)
> -               d->cifs_sb = cifs_sb;
> +               d->sb = sb;
>  }
>
> -static inline struct cifs_sb_info *
> -find_super_by_tcp(struct TCP_Server_Info *server)
> +static struct super_block *get_tcp_super(struct TCP_Server_Info *server)
>  {
>         struct super_cb_data d = {
>                 .server = server,
> -               .cifs_sb = NULL,
> +               .sb = NULL,
>         };
>
>         iterate_supers_type(&cifs_fs_type, super_cb, &d);
> -       return d.cifs_sb ? d.cifs_sb : ERR_PTR(-ENOENT);
> +
> +       if (unlikely(!d.sb))
> +               return ERR_PTR(-ENOENT);
> +       /*
> +        * Grab an active reference in order to prevent automounts (DFS links)
> +        * of expiring and then freeing up our cifs superblock pointer while
> +        * we're doing failover.
> +        */
> +       cifs_sb_active(d.sb);
> +       return d.sb;
> +}
> +
> +static inline void put_tcp_super(struct super_block *sb)
> +{
> +       if (!IS_ERR_OR_NULL(sb))
> +               cifs_sb_deactive(sb);
>  }
>
>  static void reconn_inval_dfs_target(struct TCP_Server_Info *server,
> @@ -484,6 +498,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>         struct mid_q_entry *mid_entry;
>         struct list_head retry_list;
>  #ifdef CONFIG_CIFS_DFS_UPCALL
> +       struct super_block *sb = NULL;
>         struct cifs_sb_info *cifs_sb = NULL;
>         struct dfs_cache_tgt_list tgt_list = {0};
>         struct dfs_cache_tgt_iterator *tgt_it = NULL;
> @@ -493,13 +508,15 @@ cifs_reconnect(struct TCP_Server_Info *server)
>         server->nr_targets = 1;
>  #ifdef CONFIG_CIFS_DFS_UPCALL
>         spin_unlock(&GlobalMid_Lock);
> -       cifs_sb = find_super_by_tcp(server);
> -       if (IS_ERR(cifs_sb)) {
> -               rc = PTR_ERR(cifs_sb);
> +       sb = get_tcp_super(server);
> +       if (IS_ERR(sb)) {
> +               rc = PTR_ERR(sb);
>                 cifs_dbg(FYI, "%s: will not do DFS failover: rc = %d\n",
>                          __func__, rc);
> -               cifs_sb = NULL;
> +               sb = NULL;
>         } else {
> +               cifs_sb = CIFS_SB(sb);
> +
>                 rc = reconn_setup_dfs_targets(cifs_sb, &tgt_list, &tgt_it);
>                 if (rc && (rc != -EOPNOTSUPP)) {
>                         cifs_server_dbg(VFS, "%s: no target servers for DFS failover\n",
> @@ -516,6 +533,10 @@ cifs_reconnect(struct TCP_Server_Info *server)
>                 /* the demux thread will exit normally
>                 next time through the loop */
>                 spin_unlock(&GlobalMid_Lock);
> +#ifdef CONFIG_CIFS_DFS_UPCALL
> +               dfs_cache_free_tgts(&tgt_list);
> +               put_tcp_super(sb);
> +#endif
>                 return rc;
>         } else
>                 server->tcpStatus = CifsNeedReconnect;
> @@ -642,7 +663,10 @@ cifs_reconnect(struct TCP_Server_Info *server)
>                                  __func__, rc);
>                 }
>                 dfs_cache_free_tgts(&tgt_list);
> +
>         }
> +
> +       put_tcp_super(sb);
>  #endif
>         if (server->tcpStatus == CifsNeedNegotiate)
>                 mod_delayed_work(cifsiod_wq, &server->echo, 0);
> --
> 2.24.0
>


-- 
Thanks,

Steve