Commit 5c869194 authored by Paulo Alcantara's avatar Paulo Alcantara Committed by Steve French

smb: client: fix use-after-free in smb2_query_info_compound()

The following UAF was triggered when running fstests generic/072 with
KASAN enabled against Windows Server 2022 and mount options
'multichannel,max_channels=2,vers=3.1.1,mfsymlinks,noperm'

  BUG: KASAN: slab-use-after-free in smb2_query_info_compound+0x423/0x6d0 [cifs]
  Read of size 8 at addr ffff888014941048 by task xfs_io/27534

  CPU: 0 PID: 27534 Comm: xfs_io Not tainted 6.6.0-rc7 #1
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
  rel-1.16.2-3-gd478f380-rebuilt.opensuse.org 04/01/2014
  Call Trace:
   dump_stack_lvl+0x4a/0x80
   print_report+0xcf/0x650
   ? srso_alias_return_thunk+0x5/0x7f
   ? srso_alias_return_thunk+0x5/0x7f
   ? __phys_addr+0x46/0x90
   kasan_report+0xda/0x110
   ? smb2_query_info_compound+0x423/0x6d0 [cifs]
   ? smb2_query_info_compound+0x423/0x6d0 [cifs]
   smb2_query_info_compound+0x423/0x6d0 [cifs]
   ? __pfx_smb2_query_info_compound+0x10/0x10 [cifs]
   ? srso_alias_return_thunk+0x5/0x7f
   ? __stack_depot_save+0x39/0x480
   ? kasan_save_stack+0x33/0x60
   ? kasan_set_track+0x25/0x30
   ? ____kasan_slab_free+0x126/0x170
   smb2_queryfs+0xc2/0x2c0 [cifs]
   ? __pfx_smb2_queryfs+0x10/0x10 [cifs]
   ? __pfx___lock_acquire+0x10/0x10
   smb311_queryfs+0x210/0x220 [cifs]
   ? __pfx_smb311_queryfs+0x10/0x10 [cifs]
   ? srso_alias_return_thunk+0x5/0x7f
   ? __lock_acquire+0x480/0x26c0
   ? lock_release+0x1ed/0x640
   ? srso_alias_return_thunk+0x5/0x7f
   ? do_raw_spin_unlock+0x9b/0x100
   cifs_statfs+0x18c/0x4b0 [cifs]
   statfs_by_dentry+0x9b/0xf0
   fd_statfs+0x4e/0xb0
   __do_sys_fstatfs+0x7f/0xe0
   ? __pfx___do_sys_fstatfs+0x10/0x10
   ? srso_alias_return_thunk+0x5/0x7f
   ? lockdep_hardirqs_on_prepare+0x136/0x200
   ? srso_alias_return_thunk+0x5/0x7f
   do_syscall_64+0x3f/0x90
   entry_SYSCALL_64_after_hwframe+0x6e/0xd8

  Allocated by task 27534:
   kasan_save_stack+0x33/0x60
   kasan_set_track+0x25/0x30
   __kasan_kmalloc+0x8f/0xa0
   open_cached_dir+0x71b/0x1240 [cifs]
   smb2_query_info_compound+0x5c3/0x6d0 [cifs]
   smb2_queryfs+0xc2/0x2c0 [cifs]
   smb311_queryfs+0x210/0x220 [cifs]
   cifs_statfs+0x18c/0x4b0 [cifs]
   statfs_by_dentry+0x9b/0xf0
   fd_statfs+0x4e/0xb0
   __do_sys_fstatfs+0x7f/0xe0
   do_syscall_64+0x3f/0x90
   entry_SYSCALL_64_after_hwframe+0x6e/0xd8

  Freed by task 27534:
   kasan_save_stack+0x33/0x60
   kasan_set_track+0x25/0x30
   kasan_save_free_info+0x2b/0x50
   ____kasan_slab_free+0x126/0x170
   slab_free_freelist_hook+0xd0/0x1e0
   __kmem_cache_free+0x9d/0x1b0
   open_cached_dir+0xff5/0x1240 [cifs]
   smb2_query_info_compound+0x5c3/0x6d0 [cifs]
   smb2_queryfs+0xc2/0x2c0 [cifs]

This is a race between open_cached_dir() and cached_dir_lease_break()
where the cache entry for the open directory handle receives a lease
break while creating it.  And before returning from open_cached_dir(),
we put the last reference of the new @cfid because of
!@cfid->has_lease.

Besides the UAF, while running xfstests a lot of missed lease breaks
have been noticed in tests that run several concurrent statfs(2) calls
on those cached fids

  CIFS: VFS: \\w22-root1.gandalf.test No task to wake, unknown frame...
  CIFS: VFS: \\w22-root1.gandalf.test Cmd: 18 Err: 0x0 Flags: 0x1...
  CIFS: VFS: \\w22-root1.gandalf.test smb buf 00000000715bfe83 len 108
  CIFS: VFS: Dump pending requests:
  CIFS: VFS: \\w22-root1.gandalf.test No task to wake, unknown frame...
  CIFS: VFS: \\w22-root1.gandalf.test Cmd: 18 Err: 0x0 Flags: 0x1...
  CIFS: VFS: \\w22-root1.gandalf.test smb buf 000000005aa7316e len 108
  ...

To fix both, in open_cached_dir() ensure that @cfid->has_lease is set
right before sending out compounded request so that any potential
lease break will be get processed by demultiplex thread while we're
still caching @cfid.  And, if open failed for some reason, re-check
@cfid->has_lease to decide whether or not put lease reference.

Cc: stable@vger.kernel.org
Signed-off-by: default avatarPaulo Alcantara (SUSE) <pc@manguebit.com>
Signed-off-by: default avatarSteve French <stfrench@microsoft.com>
parent c37ed2d7
...@@ -32,7 +32,7 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids, ...@@ -32,7 +32,7 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
* fully cached or it may be in the process of * fully cached or it may be in the process of
* being deleted due to a lease break. * being deleted due to a lease break.
*/ */
if (!cfid->has_lease) { if (!cfid->time || !cfid->has_lease) {
spin_unlock(&cfids->cfid_list_lock); spin_unlock(&cfids->cfid_list_lock);
return NULL; return NULL;
} }
...@@ -193,10 +193,20 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, ...@@ -193,10 +193,20 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
npath = path_no_prefix(cifs_sb, path); npath = path_no_prefix(cifs_sb, path);
if (IS_ERR(npath)) { if (IS_ERR(npath)) {
rc = PTR_ERR(npath); rc = PTR_ERR(npath);
kfree(utf16_path); goto out;
return rc;
} }
if (!npath[0]) {
dentry = dget(cifs_sb->root);
} else {
dentry = path_to_dentry(cifs_sb, npath);
if (IS_ERR(dentry)) {
rc = -ENOENT;
goto out;
}
}
cfid->dentry = dentry;
/* /*
* We do not hold the lock for the open because in case * We do not hold the lock for the open because in case
* SMB2_open needs to reconnect. * SMB2_open needs to reconnect.
...@@ -249,6 +259,15 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, ...@@ -249,6 +259,15 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
smb2_set_related(&rqst[1]); smb2_set_related(&rqst[1]);
/*
* Set @cfid->has_lease to true before sending out compounded request so
* its lease reference can be put in cached_dir_lease_break() due to a
* potential lease break right after the request is sent or while @cfid
* is still being cached. Concurrent processes won't be to use it yet
* due to @cfid->time being zero.
*/
cfid->has_lease = true;
rc = compound_send_recv(xid, ses, server, rc = compound_send_recv(xid, ses, server,
flags, 2, rqst, flags, 2, rqst,
resp_buftype, rsp_iov); resp_buftype, rsp_iov);
...@@ -263,6 +282,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, ...@@ -263,6 +282,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
cfid->tcon = tcon; cfid->tcon = tcon;
cfid->is_open = true; cfid->is_open = true;
spin_lock(&cfids->cfid_list_lock);
o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base; o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
oparms.fid->persistent_fid = o_rsp->PersistentFileId; oparms.fid->persistent_fid = o_rsp->PersistentFileId;
oparms.fid->volatile_fid = o_rsp->VolatileFileId; oparms.fid->volatile_fid = o_rsp->VolatileFileId;
...@@ -270,18 +291,25 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, ...@@ -270,18 +291,25 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
oparms.fid->mid = le64_to_cpu(o_rsp->hdr.MessageId); oparms.fid->mid = le64_to_cpu(o_rsp->hdr.MessageId);
#endif /* CIFS_DEBUG2 */ #endif /* CIFS_DEBUG2 */
if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) rc = -EINVAL;
if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) {
spin_unlock(&cfids->cfid_list_lock);
goto oshr_free; goto oshr_free;
}
smb2_parse_contexts(server, o_rsp, smb2_parse_contexts(server, o_rsp,
&oparms.fid->epoch, &oparms.fid->epoch,
oparms.fid->lease_key, &oplock, oparms.fid->lease_key, &oplock,
NULL, NULL); NULL, NULL);
if (!(oplock & SMB2_LEASE_READ_CACHING_HE)) if (!(oplock & SMB2_LEASE_READ_CACHING_HE)) {
spin_unlock(&cfids->cfid_list_lock);
goto oshr_free; goto oshr_free;
}
qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base; qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info)) if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info)) {
spin_unlock(&cfids->cfid_list_lock);
goto oshr_free; goto oshr_free;
}
if (!smb2_validate_and_copy_iov( if (!smb2_validate_and_copy_iov(
le16_to_cpu(qi_rsp->OutputBufferOffset), le16_to_cpu(qi_rsp->OutputBufferOffset),
sizeof(struct smb2_file_all_info), sizeof(struct smb2_file_all_info),
...@@ -289,37 +317,24 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, ...@@ -289,37 +317,24 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
(char *)&cfid->file_all_info)) (char *)&cfid->file_all_info))
cfid->file_all_info_is_valid = true; cfid->file_all_info_is_valid = true;
if (!npath[0])
dentry = dget(cifs_sb->root);
else {
dentry = path_to_dentry(cifs_sb, npath);
if (IS_ERR(dentry)) {
rc = -ENOENT;
goto oshr_free;
}
}
spin_lock(&cfids->cfid_list_lock);
cfid->dentry = dentry;
cfid->time = jiffies; cfid->time = jiffies;
cfid->has_lease = true;
spin_unlock(&cfids->cfid_list_lock); spin_unlock(&cfids->cfid_list_lock);
/* At this point the directory handle is fully cached */
rc = 0;
oshr_free: oshr_free:
kfree(utf16_path);
SMB2_open_free(&rqst[0]); SMB2_open_free(&rqst[0]);
SMB2_query_info_free(&rqst[1]); SMB2_query_info_free(&rqst[1]);
free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base); free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base); free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
spin_lock(&cfids->cfid_list_lock);
if (!cfid->has_lease) {
if (rc) { if (rc) {
spin_lock(&cfids->cfid_list_lock);
if (cfid->on_list) { if (cfid->on_list) {
list_del(&cfid->entry); list_del(&cfid->entry);
cfid->on_list = false; cfid->on_list = false;
cfids->num_entries--; cfids->num_entries--;
} }
rc = -ENOENT; if (cfid->has_lease) {
} else {
/* /*
* We are guaranteed to have two references at this * We are guaranteed to have two references at this
* point. One for the caller and one for a potential * point. One for the caller and one for a potential
...@@ -327,25 +342,24 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, ...@@ -327,25 +342,24 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
* will be closed when the caller closes the cached * will be closed when the caller closes the cached
* handle. * handle.
*/ */
cfid->has_lease = false;
spin_unlock(&cfids->cfid_list_lock); spin_unlock(&cfids->cfid_list_lock);
kref_put(&cfid->refcount, smb2_close_cached_fid); kref_put(&cfid->refcount, smb2_close_cached_fid);
goto out; goto out;
} }
}
spin_unlock(&cfids->cfid_list_lock); spin_unlock(&cfids->cfid_list_lock);
}
out:
if (rc) { if (rc) {
if (cfid->is_open) if (cfid->is_open)
SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid, SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
cfid->fid.volatile_fid); cfid->fid.volatile_fid);
free_cached_dir(cfid); free_cached_dir(cfid);
cfid = NULL; } else {
}
out:
if (rc == 0) {
*ret_cfid = cfid; *ret_cfid = cfid;
atomic_inc(&tcon->num_remote_opens); atomic_inc(&tcon->num_remote_opens);
} }
kfree(utf16_path);
return rc; return rc;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment