smb: client: fix memory leak during error handling for POSIX mkdir

smb: client: fix memory leak during error handling for POSIX mkdir

The response buffer for the CREATE request handled by smb311_posix_mkdir()
is leaked on the error path (goto err_free_rsp_buf) because the structure
pointer *rsp passed to free_rsp_buf() is not assigned until *after* the
error condition is checked.

As *rsp is initialised to NULL, free_rsp_buf() becomes a no-op and the leak
is instead reported by __kmem_cache_shutdown() upon subsequent rmmod of
cifs.ko if (and only if) the error path has been hit.

Pass rsp_iov.iov_base to free_rsp_buf() instead, similar to the code in
other functions in smb2pdu.c for which *rsp is assigned late.

Signed-off-by: Jethro Donaldson <devel@jro.nz>
---

Follow up on "smb: client: fix zero length for mkdir POSIX create context"

Am tempted to change all the other calls to free_rsp_buf() in smb2pdu.c
to pass rsp_iov.iov_base, even though none of the other cases where *rsp is
passed seem to exhibit the above problem. Reasoning:

 a) more robust to re-ordering during future change, 
 b) easier to follow (acquire/release via same pointer), and
 c) more consistent

If that sounds like a good idea, please advise if a separate patch is
preferred or a v2 of this one.



base-commit: e2d3e1fdb530198317501eb7ded4f3a5fb6c881c
--
2.49.0

> Am tempted to change all the other calls to free_rsp_buf() in smb2pdu.c
> to pass rsp_iov.iov_base, even though none of the other cases where *rsp is
> passed seem to exhibit the above problem.
<>
> If that sounds like a good idea, please advise if a separate patch is preferred or a v2 of this one.

The larger change is too late to go in 6.15 so larger patch would be
best to do separately

On Wed, May 14, 2025 at 8:37 AM Jethro Donaldson <devel@jro.nz> wrote:
>
> smb: client: fix memory leak during error handling for POSIX mkdir
>
> The response buffer for the CREATE request handled by smb311_posix_mkdir()
> is leaked on the error path (goto err_free_rsp_buf) because the structure
> pointer *rsp passed to free_rsp_buf() is not assigned until *after* the
> error condition is checked.
>
> As *rsp is initialised to NULL, free_rsp_buf() becomes a no-op and the leak
> is instead reported by __kmem_cache_shutdown() upon subsequent rmmod of
> cifs.ko if (and only if) the error path has been hit.
>
> Pass rsp_iov.iov_base to free_rsp_buf() instead, similar to the code in
> other functions in smb2pdu.c for which *rsp is assigned late.
>
> Signed-off-by: Jethro Donaldson <devel@jro.nz>
> ---
>
> Follow up on "smb: client: fix zero length for mkdir POSIX create context"
>
> Am tempted to change all the other calls to free_rsp_buf() in smb2pdu.c
> to pass rsp_iov.iov_base, even though none of the other cases where *rsp is
> passed seem to exhibit the above problem. Reasoning:
>
>  a) more robust to re-ordering during future change,
>  b) easier to follow (acquire/release via same pointer), and
>  c) more consistent
>
> If that sounds like a good idea, please advise if a separate patch is
> preferred or a v2 of this one.
>
> diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> index e7118501fdcc..ed3ffcb80aef 100644
> --- a/fs/smb/client/smb2pdu.c
> +++ b/fs/smb/client/smb2pdu.c
> @@ -2967,7 +2967,7 @@ int smb311_posix_mkdir(const unsigned int xid, struct inode *inode,
>         /* Eventually save off posix specific response info and timestamps */
>
>  err_free_rsp_buf:
> -       free_rsp_buf(resp_buftype, rsp);
> +       free_rsp_buf(resp_buftype, rsp_iov.iov_base);
>         kfree(pc_buf);
>  err_free_req:
>         cifs_small_buf_release(req);
>
>
> base-commit: e2d3e1fdb530198317501eb7ded4f3a5fb6c881c
> --
> 2.49.0
>

Good catch. Merged into cifs-2.6.git for-next

On Wed, May 14, 2025 at 8:37 AM Jethro Donaldson <devel@jro.nz> wrote:
>
> smb: client: fix memory leak during error handling for POSIX mkdir
>
> The response buffer for the CREATE request handled by smb311_posix_mkdir()
> is leaked on the error path (goto err_free_rsp_buf) because the structure
> pointer *rsp passed to free_rsp_buf() is not assigned until *after* the
> error condition is checked.
>
> As *rsp is initialised to NULL, free_rsp_buf() becomes a no-op and the leak
> is instead reported by __kmem_cache_shutdown() upon subsequent rmmod of
> cifs.ko if (and only if) the error path has been hit.
>
> Pass rsp_iov.iov_base to free_rsp_buf() instead, similar to the code in
> other functions in smb2pdu.c for which *rsp is assigned late.
>
> Signed-off-by: Jethro Donaldson <devel@jro.nz>
> ---
>
> Follow up on "smb: client: fix zero length for mkdir POSIX create context"
>
> Am tempted to change all the other calls to free_rsp_buf() in smb2pdu.c
> to pass rsp_iov.iov_base, even though none of the other cases where *rsp is
> passed seem to exhibit the above problem. Reasoning:
>
>  a) more robust to re-ordering during future change,
>  b) easier to follow (acquire/release via same pointer), and
>  c) more consistent
>
> If that sounds like a good idea, please advise if a separate patch is
> preferred or a v2 of this one.
>
> diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> index e7118501fdcc..ed3ffcb80aef 100644
> --- a/fs/smb/client/smb2pdu.c
> +++ b/fs/smb/client/smb2pdu.c
> @@ -2967,7 +2967,7 @@ int smb311_posix_mkdir(const unsigned int xid, struct inode *inode,
>         /* Eventually save off posix specific response info and timestamps */
>
>  err_free_rsp_buf:
> -       free_rsp_buf(resp_buftype, rsp);
> +       free_rsp_buf(resp_buftype, rsp_iov.iov_base);
>         kfree(pc_buf);
>  err_free_req:
>         cifs_small_buf_release(req);
>
>
> base-commit: e2d3e1fdb530198317501eb7ded4f3a5fb6c881c
> --
> 2.49.0
>