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

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

[Werner Kasselman]

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 by freeing smb_lock and flock before goto out in cases 1 and 2,
propagating the correct error code, and adding a NULL check for the
rollback allocation in case 3.

Found via call-graph analysis using sqry.

Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Cc: stable@vger.kernel.org
Signed-off-by: Werner Kasselman <werner@verivus.com>
---
 fs/smb/server/smb2pdu.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index 9f7ff7491e9a..36e281f5924a 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -7583,6 +7583,9 @@ int smb2_lock(struct ksmbd_work *work)
 				ksmbd_debug(SMB, "File unlocked\n");
 			} else if (rc == -ENOENT) {
 				rsp->hdr.Status = STATUS_NOT_LOCKED;
+				locks_free_lock(flock);
+				kfree(smb_lock);
+				err = -ENOENT;
 				goto out;
 			}
 			locks_free_lock(flock);
@@ -7655,6 +7658,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,6 +7691,19 @@ int smb2_lock(struct ksmbd_work *work)
 		struct file_lock *rlock = NULL;
 
 		rlock = smb_flock_init(filp);
+		if (!rlock) {
+			pr_err("rollback unlock alloc failed\n");
+			list_del(&smb_lock->llist);
+			spin_lock(&work->conn->llist_lock);
+			if (!list_empty(&smb_lock->flist))
+				list_del(&smb_lock->flist);
+			list_del(&smb_lock->clist);
+			spin_unlock(&work->conn->llist_lock);
+
+			locks_free_lock(smb_lock->fl);
+			kfree(smb_lock);
+			continue;
+		}
 		rlock->c.flc_type = F_UNLCK;
 		rlock->fl_start = smb_lock->start;
 		rlock->fl_end = smb_lock->end;
-- 
2.43.0

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

[ChenXiaoSong]

Hi Werner,

It might be better to move `locks_free_lock()` and `kfree()` to before 
`if (!rc)` statement.

```
--- 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;
```

Thanks,
ChenXiaoSong <chenxiaosong@kylinos.cn>

On 3/17/26 16:08, Werner Kasselman wrote:
> --- a/fs/smb/server/smb2pdu.c
> +++ b/fs/smb/server/smb2pdu.c
> @@ -7583,6 +7583,9 @@ int smb2_lock(struct ksmbd_work *work)
>   				ksmbd_debug(SMB, "File unlocked\n");
>   			} else if (rc == -ENOENT) {
>   				rsp->hdr.Status = STATUS_NOT_LOCKED;
> +				locks_free_lock(flock);
> +				kfree(smb_lock);
> +				err = -ENOENT;
>   				goto out;
>   			}
>   			locks_free_lock(flock);

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

[ChenXiaoSong]

And it might be better to change it as follows.

```
@@ -7685,13 +7686,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 +7706,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:
```

Thanks,
ChenXiaoSong <chenxiaosong@kylinos.cn>

On 3/17/26 16:08, Werner Kasselman wrote:
> @@ -7685,6 +7691,19 @@ int smb2_lock(struct ksmbd_work *work)
>   		struct file_lock *rlock = NULL;
>   
>   		rlock = smb_flock_init(filp);
> +		if (!rlock) {
> +			pr_err("rollback unlock alloc failed\n");
> +			list_del(&smb_lock->llist);
> +			spin_lock(&work->conn->llist_lock);
> +			if (!list_empty(&smb_lock->flist))
> +				list_del(&smb_lock->flist);
> +			list_del(&smb_lock->clist);
> +			spin_unlock(&work->conn->llist_lock);
> +
> +			locks_free_lock(smb_lock->fl);
> +			kfree(smb_lock);
> +			continue;
> +		}
>   		rlock->c.flc_type = F_UNLCK;
>   		rlock->fl_start = smb_lock->start;
>   		rlock->fl_end = smb_lock->end;

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

[Werner Kasselman]

Thanks ChenXiaoSong,

Both suggestions incorporated in v2:

1. Hoisted locks_free_lock()/kfree() before the if(!rc) check in the UNLOCK branch, all exit paths now share one free site instead of duplicating it in the -ENOENT branch.
2. Replaced the duplicated rollback cleanup with an if(rlock) guard around the VFS unlock and a matching NULL check on locks_free_lock(rlock) in the shared cleanup tail.  This eliminates the code duplication we had in v1 where the !rlock path copied the entire teardown sequence.

v2 sent.

Kind regards,
Werner

-----Original Message-----
From: ChenXiaoSong <chenxiaosong@chenxiaosong.com> 
Sent: Tuesday, 17 March 2026 7:30 PM
To: Werner Kasselman <werner@verivus.ai>; Namjae Jeon <linkinjeon@kernel.org>; Steve French <smfrench@gmail.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>; Tom Talpey <tom@talpey.com>; linux-cifs@vger.kernel.org; linux-kernel@vger.kernel.org; stable@vger.kernel.org
Subject: Re: [PATCH] ksmbd: fix memory leaks and NULL deref in smb2_lock()

And it might be better to change it as follows.

```
@@ -7685,13 +7686,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 +7706,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:
```

Thanks,
ChenXiaoSong <chenxiaosong@kylinos.cn>

On 3/17/26 16:08, Werner Kasselman wrote:
> @@ -7685,6 +7691,19 @@ int smb2_lock(struct ksmbd_work *work)
>   		struct file_lock *rlock = NULL;
>   
>   		rlock = smb_flock_init(filp);
> +		if (!rlock) {
> +			pr_err("rollback unlock alloc failed\n");
> +			list_del(&smb_lock->llist);
> +			spin_lock(&work->conn->llist_lock);
> +			if (!list_empty(&smb_lock->flist))
> +				list_del(&smb_lock->flist);
> +			list_del(&smb_lock->clist);
> +			spin_unlock(&work->conn->llist_lock);
> +
> +			locks_free_lock(smb_lock->fl);
> +			kfree(smb_lock);
> +			continue;
> +		}
>   		rlock->c.flc_type = F_UNLCK;
>   		rlock->fl_start = smb_lock->start;
>   		rlock->fl_end = smb_lock->end;