[PATCH] cifs: fix some memory leak when negotiate/session setup fails

[PATCH] cifs: fix some memory leak when negotiate/session setup fails

[Enzo Matsumiya]

Fix memory leaks from some ses fields when cifs_negotiate_protocol() or
cifs_setup_session() fails in cifs_get_smb_ses().

A leak from ses->domainName has also been identified in setup_ntlmv2_rsp()
when session setup fails.

These has been reported by kmemleak.

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
 fs/cifs/cifsencrypt.c |  5 +++++
 fs/cifs/connect.c     | 22 ++++++++++++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 8f7835ccbca1..6b681b7084f5 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -680,6 +680,11 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct nls_table *nls_cp)
 unlock:
 	cifs_server_unlock(ses->server);
 setup_ntlmv2_rsp_ret:
+	if (rc && ses->domainName) {
+		kfree(ses->domainName);
+		ses->domainName = NULL;
+	}
+
 	kfree(tiblob);
 
 	return rc;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 3da5da9f16b0..49162973ca30 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2215,7 +2215,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 	cifs_dbg(FYI, "Existing smb sess not found\n");
 	ses = sesInfoAlloc();
 	if (ses == NULL)
-		goto get_ses_fail;
+		goto out;
 
 	/* new SMB session uses our server ref */
 	ses->server = server;
@@ -2227,19 +2227,19 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 	if (ctx->username) {
 		ses->user_name = kstrdup(ctx->username, GFP_KERNEL);
 		if (!ses->user_name)
-			goto get_ses_fail;
+			goto out_free_ses;
 	}
 
 	/* ctx->password freed at unmount */
 	if (ctx->password) {
 		ses->password = kstrdup(ctx->password, GFP_KERNEL);
 		if (!ses->password)
-			goto get_ses_fail;
+			goto out_free_username;
 	}
 	if (ctx->domainname) {
 		ses->domainName = kstrdup(ctx->domainname, GFP_KERNEL);
 		if (!ses->domainName)
-			goto get_ses_fail;
+			goto out_free_pw;
 	}
 
 	strscpy(ses->workstation_name, ctx->workstation_name, sizeof(ses->workstation_name));
@@ -2273,7 +2273,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 	spin_unlock(&ses->chan_lock);
 
 	if (rc)
-		goto get_ses_fail;
+		goto out_free_domain;
 
 	/*
 	 * success, put it on the list and add it as first channel
@@ -2290,8 +2290,18 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 
 	return ses;
 
-get_ses_fail:
+out_free_domain:
+	kfree(ses->domainName);
+	ses->domainName = NULL;
+out_free_pw:
+	kfree(ses->password);
+	ses->password = NULL;
+out_free_username:
+	kfree(ses->user_name);
+	ses->user_name = NULL;
+out_free_ses:
 	sesInfoFree(ses);
+out:
 	free_xid(xid);
 	return ERR_PTR(rc);
 }
-- 
2.35.3

Re: [PATCH] cifs: fix some memory leak when negotiate/session setup fails

[Paulo Alcantara]

Enzo Matsumiya <ematsumiya@suse.de> writes:

>
> Fix memory leaks from some ses fields when cifs_negotiate_protocol() or
> cifs_setup_session() fails in cifs_get_smb_ses().
>
> A leak from ses->domainName has also been identified in setup_ntlmv2_rsp()
> when session setup fails.

Those fields are already freed by sesInfoFree().

> These has been reported by kmemleak.

Could you please include the report in commit message?

Re: [PATCH] cifs: fix some memory leak when negotiate/session setup fails

[Steve French]

It does look like the first of these may fix a real leak

setup_ntlmv2_rsp when called from CIFS_SessSetup doesn't seem to free
ses->domainName

sesInfoFree does free it but it is not clear whether in all paths
sesInfoFree is called, but of course if it isn't called we have a much
worse leak.

Can you doublecheck about the memleak details.

On Tue, Aug 23, 2022 at 12:45 PM Paulo Alcantara <pc@cjr.nz> wrote:
>
> Enzo Matsumiya <ematsumiya@suse.de> writes:
>
> >
> > Fix memory leaks from some ses fields when cifs_negotiate_protocol() or
> > cifs_setup_session() fails in cifs_get_smb_ses().
> >
> > A leak from ses->domainName has also been identified in setup_ntlmv2_rsp()
> > when session setup fails.
>
> Those fields are already freed by sesInfoFree().
>
> > These has been reported by kmemleak.
>
> Could you please include the report in commit message?



-- 
Thanks,

Steve

Re: [PATCH] cifs: fix some memory leak when negotiate/session setup fails

[Enzo Matsumiya]

Hi Paulo, Steve,

On 08/24, Steve French wrote:
>It does look like the first of these may fix a real leak
>
>setup_ntlmv2_rsp when called from CIFS_SessSetup doesn't seem to free
>ses->domainName
>
>sesInfoFree does free it but it is not clear whether in all paths
>sesInfoFree is called, but of course if it isn't called we have a much
>worse leak.
>
>Can you doublecheck about the memleak details.
>
>On Tue, Aug 23, 2022 at 12:45 PM Paulo Alcantara <pc@cjr.nz> wrote:
>>
>> Enzo Matsumiya <ematsumiya@suse.de> writes:
>>
>> >
>> > Fix memory leaks from some ses fields when cifs_negotiate_protocol() or
>> > cifs_setup_session() fails in cifs_get_smb_ses().
>> >
>> > A leak from ses->domainName has also been identified in setup_ntlmv2_rsp()
>> > when session setup fails.
>>
>> Those fields are already freed by sesInfoFree().
>>
>> > These has been reported by kmemleak.
>>
>> Could you please include the report in commit message?

Indeed, I was hitting some uncommon path.

This was on a particular branch of mine (i.e. not for-next), and I think
I might have fixed it now(*).

This one can be dropped, I think.


(*) - my branch didn't directly touch that get ses -> negotiate -> setup
path, but only failing on tree connect. So, yes, there might be a bigger
leak involved and I'll try to reproduce it again, and be sure to save
the kmemleak report (which I didn't originally).


Cheers,

Enzo