Re: [PATCH v4] cifs: fix use-after-free caused by invalid pointer `hostname`

[Shyam Prasad N]

On Fri, Oct 28, 2022 at 11:29 AM Steve French <smfrench@xxxxxxxxx> wrote:
>
> merged into cifs-2.6 for-next
>
> thx
>
> On Thu, Oct 27, 2022 at 7:49 AM Zeng Heng <zengheng4@xxxxxxxxxx> wrote:
> >
> > `hostname` needs to be set as null-pointer after free in
> > `cifs_put_tcp_session` function, or when `cifsd` thread attempts
> > to resolve hostname and reconnect the host, the thread would deref
> > the invalid pointer.
> >
> > Here is one of practical backtrace examples as reference:
> >
> > Task 477
> > ---------------------------
> >  do_mount
> >   path_mount
> >    do_new_mount
> >     vfs_get_tree
> >      smb3_get_tree
> >       smb3_get_tree_common
> >        cifs_smb3_do_mount
> >         cifs_mount
> >          mount_put_conns
> >           cifs_put_tcp_session
> >           --> kfree(server->hostname)
> >
> > cifsd
> > ---------------------------
> >  kthread
> >   cifs_demultiplex_thread
> >    cifs_reconnect
> >     reconn_set_ipaddr_from_hostname
> >     --> if (!server->hostname)
> >     --> if (server->hostname[0] == '\0')  // !! UAF fault here
> >
> > CIFS: VFS: cifs_mount failed w/return code = -112
> > mount error(112): Host is down
> > BUG: KASAN: use-after-free in reconn_set_ipaddr_from_hostname+0x2ba/0x310
> > Read of size 1 at addr ffff888108f35380 by task cifsd/480
> > CPU: 2 PID: 480 Comm: cifsd Not tainted 6.1.0-rc2-00106-gf705792f89dd-dirty #25
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
> > Call Trace:
> >  <TASK>
> >  dump_stack_lvl+0x68/0x85
> >  print_report+0x16c/0x4a3
> >  kasan_report+0x95/0x190
> >  reconn_set_ipaddr_from_hostname+0x2ba/0x310
> >  __cifs_reconnect.part.0+0x241/0x800
> >  cifs_reconnect+0x65f/0xb60
> >  cifs_demultiplex_thread+0x1570/0x2570
> >  kthread+0x2c5/0x380
> >  ret_from_fork+0x22/0x30
> >  </TASK>
> > Allocated by task 477:
> >  kasan_save_stack+0x1e/0x40
> >  kasan_set_track+0x21/0x30
> >  __kasan_kmalloc+0x7e/0x90
> >  __kmalloc_node_track_caller+0x52/0x1b0
> >  kstrdup+0x3b/0x70
> >  cifs_get_tcp_session+0xbc/0x19b0
> >  mount_get_conns+0xa9/0x10c0
> >  cifs_mount+0xdf/0x1970
> >  cifs_smb3_do_mount+0x295/0x1660
> >  smb3_get_tree+0x352/0x5e0
> >  vfs_get_tree+0x8e/0x2e0
> >  path_mount+0xf8c/0x1990
> >  do_mount+0xee/0x110
> >  __x64_sys_mount+0x14b/0x1f0
> >  do_syscall_64+0x3b/0x90
> >  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > Freed by task 477:
> >  kasan_save_stack+0x1e/0x40
> >  kasan_set_track+0x21/0x30
> >  kasan_save_free_info+0x2a/0x50
> >  __kasan_slab_free+0x10a/0x190
> >  __kmem_cache_free+0xca/0x3f0
> >  cifs_put_tcp_session+0x30c/0x450
> >  cifs_mount+0xf95/0x1970
> >  cifs_smb3_do_mount+0x295/0x1660
> >  smb3_get_tree+0x352/0x5e0
> >  vfs_get_tree+0x8e/0x2e0
> >  path_mount+0xf8c/0x1990
> >  do_mount+0xee/0x110
> >  __x64_sys_mount+0x14b/0x1f0
> >  do_syscall_64+0x3b/0x90
> >  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > The buggy address belongs to the object at ffff888108f35380
> >  which belongs to the cache kmalloc-16 of size 16
> > The buggy address is located 0 bytes inside of
> >  16-byte region [ffff888108f35380, ffff888108f35390)
> > The buggy address belongs to the physical page:
> > page:00000000333f8e58 refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff888108f350e0 pfn:0x108f35
> > flags: 0x200000000000200(slab|node=0|zone=2)
> > raw: 0200000000000200 0000000000000000 dead000000000122 ffff8881000423c0
> > raw: ffff888108f350e0 000000008080007a 00000001ffffffff 0000000000000000
> > page dumped because: kasan: bad access detected
> > Memory state around the buggy address:
> >  ffff888108f35280: fa fb fc fc fa fb fc fc fa fb fc fc fa fb fc fc
> >  ffff888108f35300: fa fb fc fc fa fb fc fc fa fb fc fc fa fb fc fc
> > >ffff888108f35380: fa fb fc fc fa fb fc fc fa fb fc fc fa fb fc fc
> >                    ^
> >  ffff888108f35400: fa fb fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >  ffff888108f35480: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >
> > Fixes: 7be3248f3139 ("cifs: To match file servers, make sure the server hostname matches")
> > Signed-off-by: Zeng Heng <zengheng4@xxxxxxxxxx>
> > Reviewed-by: Paulo Alcantara (SUSE) <pc@xxxxxx>
> > ---
> > changes in v4:
> >  - correct fix tag
> >  - add reviewed-by
> > ---
> >  fs/cifs/connect.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index ffb291579bb9..1cc47dd3b4d6 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -1584,6 +1584,7 @@ cifs_put_tcp_session(struct TCP_Server_Info *server, int from_reconnect)
> >         server->session_key.response = NULL;
> >         server->session_key.len = 0;
> >         kfree(server->hostname);
> > +       server->hostname = NULL;
> >
> >         task = xchg(&server->tsk, NULL);
> >         if (task)
> > --
> > 2.25.1
> >
>
>
> --
> Thanks,
>
> Steve

What I mean is something like this:
https://github.com/sprasad-microsoft/smb3-kernel-client/commit/07abfbeb01d3cb8d55d76c1937bd4cde46063e5d.patch

-- 
Regards,
Shyam