Skip to contentSkip to conversation

SMB: fix memory leak in smb3_validate_negotiate

PATCH In review [email protected] 4 participants 7 messages Oct 20, 2017 – Oct 25, 2017 5 messages not in the archive

Subsystems (2)
SMB client (CIFS) Kernel SMB3 server (ksmbd)
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 SMB: fix memory leak in smb3_validate_negotiate

From: Shu Wang <[email protected]>

Found the issue by kmemleak. The pointer pneg_rsp stores the pointer kmalloced from SMB2_ioctl, and should be release before function return.

unreferenced object 0xffff88018c2b1900 (size 32):
backtrace:

    kmemleak_alloc+0x4a/0xa0
    __kmalloc+0xec/0x220
    SMB2_ioctl+0x27a/0x3c0 [cifs]
    smb3_validate_negotiate+0x135/0x370 [cifs]
    SMB2_tcon+0x308/0xae0 [cifs]
    cifs_put_tcp_session+0x4a5/0x900 [cifs]
    cifs_mount+0x6f4/0x15f0 [cifs]
Signed-off-by Shu Wang <[email protected]>
---
 fs/cifs/smb2pdu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
fs/cifs/smb2pdu.c +5 -1
index 6f0e6343c15e..4785ed8e1589 100644
@@ -727,8 +727,10 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 			 rsplen);
 
 		/* relax check since Mac returns max bufsize allowed on ioctl */
-		if (rsplen > CIFSMaxBufSize)
+		if (rsplen > CIFSMaxBufSize) {
+			kfree(pneg_rsp);
 			return -EIO;
+		}
 	}
 
 	/* check validate negotiate info response matches what we got earlier */

@@ -747,10 +749,12 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 
 	/* validate negotiate successful */
 	cifs_dbg(FYI, "validate negotiate info successful\n");
+	kfree(pneg_rsp);
 	return 0;
 
 vneg_out:
 	cifs_dbg(VFS, "protocol revalidation - security settings mismatch\n");
+	kfree(pneg_rsp);
 	return -EIO;
 }
Signature · 1 line
2.13.5

A message that is not in the archive (sent off-list or not yet received): 20171020102033.22936-1-shuwang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

#2 lore raw
Quoted 5 lines

From: Shu Wang <[email protected]>

Found the issue by kmemleak. The pointer pneg_rsp stores the pointer kmalloced from SMB2_ioctl, and should be release before function return.

Thanks for the patch, Shu Wang. A fix for this memory leak is already queued at https://bugzilla.samba.org/show_bug.cgi?id=13092 , alongside an extra fix for potential use of uninitialised memory. Patches to follow...

Cheers, David

A message that is not in the archive (sent off-list or not yet received): [email protected]

#4 lore raw

PATCH 1/2 SMB: fix leak of validate negotiate info response buffer

Fixes ff1c038addc4 ("Check SMB3 dialects against downgrade attacks")
Signed-off-by David Disseldorp <[email protected]>
---
 fs/cifs/smb2pdu.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
fs/cifs/smb2pdu.c +5 -2
index 6f0e6343c15e..052ab5dee6b6 100644
@@ -648,7 +648,7 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 {
 	int rc = 0;
 	struct validate_negotiate_info_req vneg_inbuf;
-	struct validate_negotiate_info_rsp *pneg_rsp;
+	struct validate_negotiate_info_rsp *pneg_rsp = NULL;
 	u32 rsplen;
 	u32 inbuflen; /* max of 4 dialects */
 

@@ -728,7 +728,7 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 
 		/* relax check since Mac returns max bufsize allowed on ioctl */
 		if (rsplen > CIFSMaxBufSize)
-			return -EIO;
+			goto err_rsp_free;
 	}
 
 	/* check validate negotiate info response matches what we got earlier */

@@ -747,10 +747,13 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 
 	/* validate negotiate successful */
 	cifs_dbg(FYI, "validate negotiate info successful\n");
+	kfree(pneg_rsp);
 	return 0;
 
 vneg_out:
 	cifs_dbg(VFS, "protocol revalidation - security settings mismatch\n");
+err_rsp_free:
+	kfree(pneg_rsp);
 	return -EIO;
 }
Signature · 1 line
2.13.6

A message that is not in the archive (sent off-list or not yet received): [email protected]

#6 lore raw

Re: [PATCH 1/2] SMB: fix leak of validate negotiate info response buffer

Quoted 18 lines

From: "David Disseldorp" <[email protected]>
To: "Shu Wang" <[email protected]>, [email protected], [email protected], samba-technical-w/[email protected]
Cc: "David Disseldorp" <[email protected]>
Sent: Friday, October 20, 2017 8:49:37 PM
Subject: [PATCH 1/2] SMB: fix leak of validate negotiate info response buffer

Fixes ff1c038addc4 ("Check SMB3 dialects against downgrade attacks")
Signed-off-by David Disseldorp <[email protected]>
---
 fs/cifs/smb2pdu.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
fs/cifs/smb2pdu.c +0 -0
index 6f0e6343c15e..052ab5dee6b6 100644

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, · line 654

Quoted 8 lines
fs/cifs/smb2pdu.c +1 -1
@@ -648,7 +648,7 @@ int smb3_validate_negotiate(const unsigned int xid,
 struct cifs_tcon *tcon)
 {
 	int rc = 0;
 	struct validate_negotiate_info_req vneg_inbuf;
-	struct validate_negotiate_info_rsp *pneg_rsp;
+	struct validate_negotiate_info_rsp *pneg_rsp = NULL;
 	u32 rsplen;
 	u32 inbuflen; /* max of 4 dialects */

SMB2_ioctl will set pneg_rsp pointer to NULL, so it won't really cause any issue. Anyway, looks good to me.

1879 SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid,

1880 >---   u64 volatile_fid, u32 opcode, bool is_fsctl, bool use_ipc,
1881 >---   char *in_data, u32 indatalen,
1882 >---   char **out_data, u32 *plen /* returned data len */)

1883 {
........
1897 >---if (out_data != NULL)
1898 >--->---*out_data = NULL;
1899

A message that is not in the archive (sent off-list or not yet received): 61197467.18033621.1508554198404.JavaMail.zimbra-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

#8 lore raw

Re: [PATCH 1/2] SMB: fix leak of validate negotiate info response buffer

Hi Shu Wang,

Quoted 7 lines · Shu Wang
Quoted 7 lines
fs/cifs/smb2pdu.c +0 -0
@@ -648,7 +648,7 @@ int smb3_validate_negotiate(const unsigned int xid,
 struct cifs_tcon *tcon)
 {
 	int rc = 0;
 	struct validate_negotiate_info_req vneg_inbuf;
Quoted 8 lines

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 653

Quoted 5 lines
fs/cifs/smb2pdu.c +1 -1
-	struct validate_negotiate_info_rsp *pneg_rsp;
+	struct validate_negotiate_info_rsp *pneg_rsp = NULL;
 	u32 rsplen;
 	u32 inbuflen; /* max of 4 dialects */

SMB2_ioctl will set pneg_rsp pointer to NULL, so it won't really cause any issue. Anyway, looks good to me.

Yeah, this hunk is unnecessary, but thought it might be helpful if someone in future wants to jump to the error path prior to the SMB2_ioctl() call. @Steve: feel free to drop it if you prefer.

Cheers, David

#9 lore raw

PATCH 2/2 SMB: fix validate negotiate info uninitialised memory use

In reply to a message not in the archive

An undersize validate negotiate info server response causes the client to use uninitialised memory for struct validate_negotiate_info_rsp comparisons of Dialect, SecurityMode and/or Capabilities members.

Link https://bugzilla.samba.org/show_bug.cgi?id=13092
Fixes 7db0a6efdc3e ("SMB3: Work around mount failure when using SMB3 dialect to Macs")
Signed-off-by David Disseldorp <[email protected]>
---
 fs/cifs/smb2pdu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
fs/cifs/smb2pdu.c +2 -1
index 052ab5dee6b6..c836de2f79b2 100644
@@ -727,7 +727,8 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 			 rsplen);
 
 		/* relax check since Mac returns max bufsize allowed on ioctl */
-		if (rsplen > CIFSMaxBufSize)
+		if ((rsplen > CIFSMaxBufSize)
+		     || (rsplen < sizeof(struct validate_negotiate_info_rsp)))
 			goto err_rsp_free;
 	}
Signature · 1 line
2.13.6

A message that is not in the archive (sent off-list or not yet received): [email protected]

Acked-by
#11 lore raw

Re: [PATCH 2/2] SMB: fix validate negotiate info uninitialised memory use

Looks good.

Acked-by Pavel Shilovsky <[email protected]>

Best regards,
Pavel Shilovskiy

2017-10-20 5:49 GMT-07:00 David Disseldorp via samba-technical <samba-technical-w/[email protected]>:

Quoted 29 lines

An undersize validate negotiate info server response causes the client to use uninitialised memory for struct validate_negotiate_info_rsp comparisons of Dialect, SecurityMode and/or Capabilities members.

Link https://bugzilla.samba.org/show_bug.cgi?id=13092
Fixes 7db0a6efdc3e ("SMB3: Work around mount failure when using SMB3 dialect to Macs")
Signed-off-by David Disseldorp <[email protected]>
---
 fs/cifs/smb2pdu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
fs/cifs/smb2pdu.c +2 -2
index 052ab5dee6b6..c836de2f79b2 100644
@@ -727,7 +727,8 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
                         rsplen);
 
                /* relax check since Mac returns max bufsize allowed on ioctl */
-               if (rsplen > CIFSMaxBufSize)
+               if ((rsplen > CIFSMaxBufSize)
+                    || (rsplen < sizeof(struct validate_negotiate_info_rsp)))
                        goto err_rsp_free;
        }
 
--

2.13.6