fs/smb/server/smb2pdu.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
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
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;
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);
© 2016 - 2026 Red Hat, Inc.