| Message ID | 20220823142531.9057-1-ematsumiya@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: fix some memory leak when negotiate/session setup fails | expand |
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?
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?
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
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); }
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(-)