[1/7] cifs: Fix use-after-free bug in cifs_reconnect()
diff mbox series

Message ID 20191122153057.6608-2-pc@cjr.nz
State New
Headers show
Series
  • DFS fixes
Related show

Commit Message

Paulo Alcantara (SUSE) Nov. 22, 2019, 3:30 p.m. UTC
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(-)

Comments

Steve French Nov. 25, 2019, 7:34 a.m. UTC | #1
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
>
Aurelien Aptel Nov. 25, 2019, 11:36 a.m. UTC | #2
"Paulo Alcantara (SUSE)" <pc@cjr.nz> writes:
> 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.

Reviewed-by: Aurelien Aptel <aaptel@suse.com>

Patch
diff mbox series

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);