* [PATCH] SMB: fix memory leak in smb3_validate_negotiate @ 2017-10-20 10:20 shuwang [not found] ` <20171020102033.22936-1-shuwang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 7+ messages in thread From: shuwang @ 2017-10-20 10:20 UTC (permalink / raw) To: sfrench Cc: linux-cifs, samba-technical, linux-kernel, chuhu, yizhan, Shu Wang 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
[parent not found: <20171020102033.22936-1-shuwang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH] SMB: fix memory leak in smb3_validate_negotiate [not found] ` <20171020102033.22936-1-shuwang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> @ 2017-10-20 12:49 ` David Disseldorp [not found] ` <20171020124938.9913-1-ddiss-l3A5Bk7waGM@public.gmane.org> 0 siblings, 1 reply; 7+ messages in thread From: David Disseldorp @ 2017-10-20 12:49 UTC (permalink / raw) To: Shu Wang, sfrench-eUNUBHrolfbYtjvyW6yDsg, linux-cifs-u79uwXL29TY76Z2rM5mHXA, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <20171020124938.9913-1-ddiss-l3A5Bk7waGM@public.gmane.org>]
* [PATCH 1/2] SMB: fix leak of validate negotiate info response buffer [not found] ` <20171020124938.9913-1-ddiss-l3A5Bk7waGM@public.gmane.org> @ 2017-10-20 12:49 ` David Disseldorp [not found] ` <20171020124938.9913-2-ddiss-l3A5Bk7waGM@public.gmane.org> 2017-10-20 12:49 ` [PATCH 2/2] SMB: fix validate negotiate info uninitialised memory use David Disseldorp 1 sibling, 1 reply; 7+ messages in thread From: David Disseldorp @ 2017-10-20 12:49 UTC (permalink / raw) To: Shu Wang, sfrench-eUNUBHrolfbYtjvyW6yDsg, linux-cifs-u79uwXL29TY76Z2rM5mHXA, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ Cc: 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
[parent not found: <20171020124938.9913-2-ddiss-l3A5Bk7waGM@public.gmane.org>]
* Re: [PATCH 1/2] SMB: fix leak of validate negotiate info response buffer [not found] ` <20171020124938.9913-2-ddiss-l3A5Bk7waGM@public.gmane.org> @ 2017-10-21 2:49 ` Shu Wang [not found] ` <61197467.18033621.1508554198404.JavaMail.zimbra-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 7+ messages in thread From: Shu Wang @ 2017-10-21 2:49 UTC (permalink / raw) To: David Disseldorp Cc: sfrench-eUNUBHrolfbYtjvyW6yDsg, linux-cifs-u79uwXL29TY76Z2rM5mHXA, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <61197467.18033621.1508554198404.JavaMail.zimbra-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH 1/2] SMB: fix leak of validate negotiate info response buffer [not found] ` <61197467.18033621.1508554198404.JavaMail.zimbra-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> @ 2017-10-22 14:16 ` David Disseldorp 0 siblings, 0 replies; 7+ messages in thread From: David Disseldorp @ 2017-10-22 14:16 UTC (permalink / raw) To: Shu Wang Cc: sfrench-eUNUBHrolfbYtjvyW6yDsg, linux-cifs-u79uwXL29TY76Z2rM5mHXA, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] SMB: fix validate negotiate info uninitialised memory use [not found] ` <20171020124938.9913-1-ddiss-l3A5Bk7waGM@public.gmane.org> 2017-10-20 12:49 ` [PATCH 1/2] SMB: fix leak of validate negotiate info response buffer David Disseldorp @ 2017-10-20 12:49 ` David Disseldorp [not found] ` <20171020124938.9913-3-ddiss-l3A5Bk7waGM@public.gmane.org> 1 sibling, 1 reply; 7+ messages in thread From: David Disseldorp @ 2017-10-20 12:49 UTC (permalink / raw) To: Shu Wang, sfrench-eUNUBHrolfbYtjvyW6yDsg, linux-cifs-u79uwXL29TY76Z2rM5mHXA, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ Cc: 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
[parent not found: <20171020124938.9913-3-ddiss-l3A5Bk7waGM@public.gmane.org>]
* Re: [PATCH 2/2] SMB: fix validate negotiate info uninitialised memory use [not found] ` <20171020124938.9913-3-ddiss-l3A5Bk7waGM@public.gmane.org> @ 2017-10-25 18:04 ` Pavel Shilovsky 0 siblings, 0 replies; 7+ messages in thread From: Pavel Shilovsky @ 2017-10-25 18:04 UTC (permalink / raw) To: David Disseldorp; +Cc: Shu Wang, Steve French, linux-cifs, samba-technical 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-10-25 18:04 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-20 10:20 [PATCH] SMB: fix memory leak in smb3_validate_negotiate shuwang [not found] ` <20171020102033.22936-1-shuwang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2017-10-20 12:49 ` David Disseldorp [not found] ` <20171020124938.9913-1-ddiss-l3A5Bk7waGM@public.gmane.org> 2017-10-20 12:49 ` [PATCH 1/2] SMB: fix leak of validate negotiate info response buffer David Disseldorp [not found] ` <20171020124938.9913-2-ddiss-l3A5Bk7waGM@public.gmane.org> 2017-10-21 2:49 ` Shu Wang [not found] ` <61197467.18033621.1508554198404.JavaMail.zimbra-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2017-10-22 14:16 ` David Disseldorp 2017-10-20 12:49 ` [PATCH 2/2] SMB: fix validate negotiate info uninitialised memory use David Disseldorp [not found] ` <20171020124938.9913-3-ddiss-l3A5Bk7waGM@public.gmane.org> 2017-10-25 18:04 ` Pavel Shilovsky
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.