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: 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>
Reviewed-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index 8fa780e8efd002b224d180ab653709dc86d2e80d..24f8d58493d00db6ed4671241a447db563a4d712 100644 (file)
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -7592,14 +7592,15 @@ retry:
                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 @@ skip:
                                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,13 +7702,17 @@ out:
                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);
@@ -7714,7 +7722,8 @@ out:
                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: