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

[Werner Kasselman]

Hi Steve,

The Sashiko review confirms the patch addresses its stated goals.
The one question it raises:

Whether the asymmetry between UNLOCK and non-UNLOCK error handling aligns with the SMB2 specification.

This asymmetry is pre-existing behavior — non-ENOENT unlock errors   are silently skipped in the current code. This patch preserves that behavior; it only fixes the resource leaks and NULL deref on the error paths. The UNLOCK/non-UNLOCK difference would be a separate discussion if it needs changing.

Werner

-----Original Message-----
From: Steve French <smfrench@xxxxxxxxx> 
Sent: Wednesday, 18 March 2026 7:22 AM
To: ChenXiaoSong <chenxiaosong@xxxxxxxxxxxxxxxx>
Cc: Werner Kasselman <werner@xxxxxxxxxx>; Namjae Jeon <linkinjeon@xxxxxxxxxx>; Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>; Tom Talpey <tom@xxxxxxxxxx>; linux-cifs@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx
Subject: Re: [PATCH v2] ksmbd: fix memory leaks and NULL deref in smb2_lock()

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@xxxxxxxxxxxxxxxx> wrote:
>
> Looks good. Feel free to add:
> Reviewed-by: ChenXiaoSong <chenxiaosong@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
> > Suggested-by: ChenXiaoSong<chenxiaosong@xxxxxxxxxx>
> > Signed-off-by: Werner Kasselman<werner@xxxxxxxxxxx>
> > ---
> >   fs/smb/server/smb2pdu.c | 27 ++++++++++++++++++---------
> >   1 file changed, 18 insertions(+), 9 deletions(-)
>


--
Thanks,

Steve