Commit 309b44ed authored by Werner Kasselman's avatar Werner Kasselman Committed by Steve French
Browse files

ksmbd: fix memory leaks and NULL deref in smb2_lock() 



smb2_lock() has three error handling issues after list_del() detaches
smb_lock from lock_list at no_check_cl:

1) If vfs_lock_file() returns an unexpected error in the non-UNLOCK
   path, goto out leaks smb_lock and its flock because the out:
   handler only iterates lock_list and rollback_list, neither of
   which contains the detached smb_lock.

2) If vfs_lock_file() returns -ENOENT in the UNLOCK path, goto out
   leaks smb_lock and flock for the same reason.  The error code
   returned to the dispatcher is also stale.

3) In the rollback path, smb_flock_init() can return NULL on
   allocation failure.  The result is dereferenced unconditionally,
   causing a kernel NULL pointer dereference.  Add a NULL check to
   prevent the crash and clean up the bookkeeping; the VFS lock
   itself cannot be rolled back without the allocation and will be
   released at file or connection teardown.

Fix cases 1 and 2 by hoisting the locks_free_lock()/kfree() to before
the if(!rc) check in the UNLOCK branch so all exit paths share one
free site, and by freeing smb_lock and flock before goto out in the
non-UNLOCK branch.  Propagate the correct error code in both cases.
Fix case 3 by wrapping the VFS unlock in an if(rlock) guard and adding
a NULL check for locks_free_lock(rlock) in the shared cleanup.

Found via call-graph analysis using sqry.

Fixes: e2f34481 ("cifsd: add server-side procedures for SMB3")
Cc: stable@vger.kernel.org
Suggested-by: default avatarChenXiaoSong <chenxiaosong@kylinos.cn>
Signed-off-by: default avatarWerner Kasselman <werner@verivus.com>
Reviewed-by: default avatarChenXiaoSong <chenxiaosong@kylinos.cn>
Acked-by: default avatarNamjae Jeon <linkinjeon@kernel.org>
Signed-off-by: default avatarSteve French <stfrench@microsoft.com>
parent 48623ec3

fs/smb/server/smb2pdu.c

+18 −9
Original line number Diff line number Diff line
@@ -7592,14 +7592,15 @@ int smb2_lock(struct ksmbd_work *work) 
		rc = vfs_lock_file(filp, smb_lock->cmd, flock, NULL);

skip:

		if (smb_lock->flags & SMB2_LOCKFLAG_UNLOCK) {

			locks_free_lock(flock);

			kfree(smb_lock);

			if (!rc) {

				ksmbd_debug(SMB, "File unlocked\n");

			} else if (rc == -ENOENT) {

				rsp->hdr.Status = STATUS_NOT_LOCKED;

				err = rc;

				goto out;

			}

			locks_free_lock(flock);

			kfree(smb_lock);

		} else {

			if (rc == FILE_LOCK_DEFERRED) {

				void **argv;
@@ -7668,6 +7669,9 @@ int smb2_lock(struct ksmbd_work *work) 
				spin_unlock(&work->conn->llist_lock);

				ksmbd_debug(SMB, "successful in taking lock\n");

			} else {

				locks_free_lock(flock);

				kfree(smb_lock);

				err = rc;

				goto out;

			}

		}
@@ -7698,6 +7702,7 @@ int smb2_lock(struct ksmbd_work *work) 
		struct file_lock *rlock = NULL;



		rlock = smb_flock_init(filp);

		if (rlock) {

			rlock->c.flc_type = F_UNLCK;

			rlock->fl_start = smb_lock->start;

			rlock->fl_end = smb_lock->end;
@@ -7705,6 +7710,9 @@ int smb2_lock(struct ksmbd_work *work) 
			rc = vfs_lock_file(filp, F_SETLK, rlock, NULL);

			if (rc)

				pr_err("rollback unlock fail : %d\n", rc);

		} else {

			pr_err("rollback unlock alloc failed\n");

		}



		list_del(&smb_lock->llist);

		spin_lock(&work->conn->llist_lock);
@@ -7714,6 +7722,7 @@ int smb2_lock(struct ksmbd_work *work) 
		spin_unlock(&work->conn->llist_lock);



		locks_free_lock(smb_lock->fl);

		if (rlock)

			locks_free_lock(rlock);

		kfree(smb_lock);

	}