[PATCH] SMB: fix memory leak in smb3_validate_negotiate

[PATCH] SMB: fix memory leak in smb3_validate_negotiate

[shuwang]

From: Shu Wang <shuwang@redhat.com>

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 <shuwang@redhat.com>
---
 fs/cifs/smb2pdu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 6f0e6343c15e..4785ed8e1589 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -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;
 }
 
-- 
2.13.5

Re: [PATCH] SMB: fix memory leak in smb3_validate_negotiate

[David Disseldorp]

> From: Shu Wang <shuwang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> 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

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

[David Disseldorp]

Fixes: ff1c038addc4 ("Check SMB3 dialects against downgrade attacks")
Signed-off-by: David Disseldorp <ddiss-l3A5Bk7waGM@public.gmane.org>
---
 fs/cifs/smb2pdu.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 6f0e6343c15e..052ab5dee6b6 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -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;
 }
 
-- 
2.13.6

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

[David Disseldorp]

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 <ddiss-l3A5Bk7waGM@public.gmane.org>
---
 fs/cifs/smb2pdu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 052ab5dee6b6..c836de2f79b2 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -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

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

[Shu Wang]

> From: "David Disseldorp" <ddiss-l3A5Bk7waGM@public.gmane.org>
> To: "Shu Wang" <shuwang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, sfrench-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ@public.gmane.org
> Cc: "David Disseldorp" <ddiss-l3A5Bk7waGM@public.gmane.org>
> 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 <ddiss-l3A5Bk7waGM@public.gmane.org>
> ---
>  fs/cifs/smb2pdu.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 6f0e6343c15e..052ab5dee6b6 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -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 

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

[David Disseldorp]

Hi Shu Wang,

On Fri, 20 Oct 2017 22:49:58 -0400 (EDT), Shu Wang wrote:

> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -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.

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

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

[Pavel Shilovsky]

Looks good.

Acked-by: Pavel Shilovsky <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>


Best regards,
Pavel Shilovskiy


2017-10-20 5:49 GMT-07:00 David Disseldorp via samba-technical
<samba-technical-w/Ol4Ecudpl8XjKLYN78aQ@public.gmane.org>:
> 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 <ddiss-l3A5Bk7waGM@public.gmane.org>
> ---
>  fs/cifs/smb2pdu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 052ab5dee6b6..c836de2f79b2 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -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
>
>