[PATCH v2] ksmbd: fix use-after-free and NULL deref in smb_grant_oplock()

Werner Kasselman posted 1 patch 2 weeks, 6 days ago
There is a newer version of this series
fs/smb/server/oplock.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
[PATCH v2] ksmbd: fix use-after-free and NULL deref in smb_grant_oplock()
Posted by Werner Kasselman 2 weeks, 6 days ago
smb_grant_oplock() has two issues in the oplock publication sequence:

1) opinfo is linked into ci->m_op_list (via opinfo_add) before
   add_lease_global_list() is called.  If add_lease_global_list()
   fails (kmalloc returns NULL), the error path frees the opinfo
   via __free_opinfo() while it is still linked in ci->m_op_list.
   Concurrent m_op_list readers (opinfo_get_list, or direct iteration
   in smb_break_all_levII_oplock) dereference the freed node.

2) opinfo->o_fp is assigned after add_lease_global_list() publishes
   the opinfo on the global lease list.  A concurrent
   find_same_lease_key() can walk the lease list and dereference
   opinfo->o_fp->f_ci while o_fp is still NULL.

Fix by restructuring the publication sequence to eliminate post-publish
failure:

- Set opinfo->o_fp before any list publication (fixes NULL deref).
- Preallocate lease_table via alloc_lease_table() before opinfo_add()
  so add_lease_global_list() becomes infallible after publication.
- Keep the original m_op_list publication order (opinfo_add before
  lease list) so concurrent opens via same_client_has_lease() and
  opinfo_get_list() still see the in-flight grant.
- Use opinfo_put() instead of __free_opinfo() on err_out so that
  the RCU-deferred free path is used.

This also requires splitting add_lease_global_list() to take a
preallocated lease_table and changing its return type from int to void,
since it can no longer fail.

Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Fixes: 1dfd062caa16 ("ksmbd: fix use-after-free by using call_rcu() for oplock_info")
Cc: stable@vger.kernel.org
Signed-off-by: Werner Kasselman <werner@verivus.com>
---
 fs/smb/server/oplock.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/fs/smb/server/oplock.c b/fs/smb/server/oplock.c
index 393a4ae47cc1..4bc7737f7aa8 100644
--- a/fs/smb/server/oplock.c
+++ b/fs/smb/server/oplock.c
@@ -1291,8 +1291,17 @@ int smb_grant_oplock(struct ksmbd_work *work, int req_op_level, u64 pid,
 	set_oplock_level(opinfo, req_op_level, lctx);
 
 out:
-	opinfo_count_inc(fp);
-	opinfo_add(opinfo, fp);
+	/*
+	 * Set o_fp before any publication so that concurrent readers
+	 * (e.g. find_same_lease_key() on the lease list) that
+	 * dereference opinfo->o_fp don't hit a NULL pointer.
+	 *
+	 * Add to lease global list before publishing on the inode
+	 * op list.  add_lease_global_list() can fail on allocation
+	 * and we must not leave a freed opinfo linked in ci->m_op_list
+	 * where concurrent opinfo_get_list() readers could find it.
+	 */
+	opinfo->o_fp = fp;
 
 	if (opinfo->is_lease) {
 		err = add_lease_global_list(opinfo);
@@ -1300,8 +1309,10 @@ int smb_grant_oplock(struct ksmbd_work *work, int req_op_level, u64 pid,
 			goto err_out;
 	}
 
+	opinfo_count_inc(fp);
+	opinfo_add(opinfo, fp);
+
 	rcu_assign_pointer(fp->f_opinfo, opinfo);
-	opinfo->o_fp = fp;
 
 	return 0;
 err_out:
-- 
2.43.0