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

Message ID 20171020124938.9913-2-ddiss@suse.de
State New
Headers show
Series
  • [1/2] SMB: fix leak of validate negotiate info response buffer
Related show

Commit Message

David Disseldorp Oct. 20, 2017, 12:49 p.m.
Fixes: ff1c038addc4 ("Check SMB3 dialects against downgrade attacks")
Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 fs/cifs/smb2pdu.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Shu Wang Oct. 21, 2017, 2:49 a.m. | #1
> From: "David Disseldorp" <ddiss@suse.de>
> To: "Shu Wang" <shuwang@redhat.com>, sfrench@samba.org, linux-cifs@vger.kernel.org, samba-technical@lists.samba.org
> Cc: "David Disseldorp" <ddiss@suse.de>
> 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@suse.de>
> ---
>  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 
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Disseldorp Oct. 22, 2017, 2:16 p.m. | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;
 }