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

[PATCH v2] ksmbd: fix memory leaks and NULL deref in smb2_lock()

Posted by Werner Kasselman 2 weeks, 6 days ago

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: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Cc: stable@vger.kernel.org
Suggested-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
Signed-off-by: Werner Kasselman <werner@verivus.com>
---
 fs/smb/server/smb2pdu.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index 9f7ff7491e9a..0485187e5156 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -7579,14 +7579,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;
@@ -7655,6 +7656,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;
 			}
 		}
@@ -7685,13 +7689,17 @@ int smb2_lock(struct ksmbd_work *work)
 		struct file_lock *rlock = NULL;
 
 		rlock = smb_flock_init(filp);
-		rlock->c.flc_type = F_UNLCK;
-		rlock->fl_start = smb_lock->start;
-		rlock->fl_end = smb_lock->end;
+		if (rlock) {
+			rlock->c.flc_type = F_UNLCK;
+			rlock->fl_start = smb_lock->start;
+			rlock->fl_end = smb_lock->end;
 
-		rc = vfs_lock_file(filp, F_SETLK, rlock, NULL);
-		if (rc)
-			pr_err("rollback unlock fail : %d\n", rc);
+			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);
@@ -7701,7 +7709,8 @@ int smb2_lock(struct ksmbd_work *work)
 		spin_unlock(&work->conn->llist_lock);
 
 		locks_free_lock(smb_lock->fl);
-		locks_free_lock(rlock);
+		if (rlock)
+			locks_free_lock(rlock);
 		kfree(smb_lock);
 	}
 out2:
-- 
2.43.0

Re: [PATCH v2] ksmbd: fix memory leaks and NULL deref in smb2_lock()

Posted by Namjae Jeon 2 weeks, 5 days ago

On Tue, Mar 17, 2026 at 6:53 PM Werner Kasselman <werner@verivus.ai> wrote:
>
> 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: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
> Cc: stable@vger.kernel.org
> Suggested-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
> Signed-off-by: Werner Kasselman <werner@verivus.com>
Applied it to #ksmbd-for-next-next.
Thanks!

Re: [PATCH v2] ksmbd: fix memory leaks and NULL deref in smb2_lock()

Posted by ChenXiaoSong 2 weeks, 6 days ago

Looks good. Feel free to add:
Reviewed-by: ChenXiaoSong <chenxiaosong@kylinos.cn>

On 3/17/26 17:46, Werner Kasselman wrote:
> 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: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
> Cc:stable@vger.kernel.org
> Suggested-by: ChenXiaoSong<chenxiaosong@kylinos.cn>
> Signed-off-by: Werner Kasselman<werner@verivus.com>
> ---
>   fs/smb/server/smb2pdu.c | 27 ++++++++++++++++++---------
>   1 file changed, 18 insertions(+), 9 deletions(-)

Re: [PATCH v2] ksmbd: fix memory leaks and NULL deref in smb2_lock()

Posted by Steve French 2 weeks, 6 days ago

I see that Sashiko had AI review comments on the patch:

https://sashiko.dev/#/patchset/20260317094653.2236624-1-werner%40verivus.com

On Tue, Mar 17, 2026 at 6:10 AM ChenXiaoSong
<chenxiaosong@chenxiaosong.com> wrote:
>
> Looks good. Feel free to add:
> Reviewed-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
>
> On 3/17/26 17:46, Werner Kasselman wrote:
> > 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: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
> > Cc:stable@vger.kernel.org
> > Suggested-by: ChenXiaoSong<chenxiaosong@kylinos.cn>
> > Signed-off-by: Werner Kasselman<werner@verivus.com>
> > ---
> >   fs/smb/server/smb2pdu.c | 27 ++++++++++++++++++---------
> >   1 file changed, 18 insertions(+), 9 deletions(-)
>


-- 
Thanks,

Steve