linux-kernel - Re: [PATCH] cifs: fix bad buffer length check in coalesce

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20120104112912.139f9578@tlielax.poochiereds.net>
Date:	Wed, 4 Jan 2012 11:29:12 -0500
From:	Jeff Layton <jlayton@...hat.com>
To:	Steve French <smfrench@...il.com>
Cc:	Shirish Pargaonkar <shirishpargaonkar@...il.com>,
	linux-kernel@...r.kernel.org, k.skarlatos@...il.com,
	linux-cifs@...r.kernel.org
Subject: Re: [PATCH] cifs: fix bad buffer length check in coalesce_t2

On Wed, 4 Jan 2012 10:14:25 -0600
Steve French <smfrench@...il.com> wrote:

> FYI - the patch is merged into cifs git tree but want to wait 24 hours
> for linux-next before sending merge request upstream.
> 

I wouldn't wait. The 3.2 release is imminent.

> On Wed, Jan 4, 2012 at 8:09 AM, Jeff Layton <jlayton@...hat.com> wrote:
> > On Tue, 3 Jan 2012 16:46:15 -0600
> > Shirish Pargaonkar <shirishpargaonkar@...il.com> wrote:
> >
> >> On Sun, Jan 1, 2012 at 9:34 AM, Jeff Layton <jlayton@...hat.com> wrote:
> >> > The current check looks to see if the RFC1002 length is larger than
> >> > CIFSMaxBufSize, and fails if it is. The buffer is actually larger than
> >> > that by MAX_CIFS_HDR_SIZE.
> >> >
> >> > This bug has been around for a long time, but the fact that we used to
> >> > cap the clients MaxBufferSize at the same level as the server tended
> >> > to paper over it. Commit c974befa changed that however and caused this
> >> > bug to bite in more cases.
> >> >
> >> > Reported-and-Tested-by: Konstantinos Skarlatos <k.skarlatos@...il.com>
> >> > Signed-off-by: Jeff Layton <jlayton@...hat.com>
> >> > ---
> >> >  fs/cifs/connect.c |    2 +-
> >> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> > index 8cd4b52..27c4f25 100644
> >> > --- a/fs/cifs/connect.c
> >> > +++ b/fs/cifs/connect.c
> >> > @@ -282,7 +282,7 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB)
> >> >        byte_count = be32_to_cpu(pTargetSMB->smb_buf_length);
> >> >        byte_count += total_in_buf2;
> >> >        /* don't allow buffer to overflow */
> >> > -       if (byte_count > CIFSMaxBufSize)
> >> > +       if (byte_count > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4)
> >> >                return -ENOBUFS;
> >> >        pTargetSMB->smb_buf_length = cpu_to_be32(byte_count);
> >> >
> >> > --
> >> > 1.7.7.4
> >> >
> >>
> >> This looks correct.  But the way Windows XP server responds to request
> >>  trans2/find_first2/info level is different than how Windows 2003 Server
> >> and Windows 2008 Server respond.
> >>
> >> So a related concern would be, for a response from Windows XP server,
> >> this check in function check2ndT2 does not make sense.
> >>  if (total_data_size > CIFSMaxBufSize) {
> >>
> >
> > This check looks redundant to me. We could remove it since the check in
> > coalesce_t2 is more accurate...
> >
> >> It is possible to have large number of entries in a directory such that the
> >> response to a  ls  command can exceed CIFSMaxBufSize.
> >
> > It shouldn't be possible. The CIFSFindFirst request sends this:
> >
> >        pSMB->MaxDataCount = cpu_to_le16(CIFSMaxBufSize & 0xFFFFFF00);
> >
> > ...which should ensure that the amount of data in the response is less
> > than CIFSMaxBufSize. I'm not sure what the point of the mask is there
> > however...
> >
> > In addition, we're also limited by this:
> >
> >        pSMB->SearchCount = cpu_to_le16(CIFSMaxBufSize/sizeof(FILE_UNIX_INFO));
> >
> > ...but I think we ought to consider just setting that to 0xffff.
> >
> > Dividing by sizeof(FILE_UNIX_INFO) is clearly wrong for other
> > infolevels. We don't really care how many entries the server sends as
> > long as it doesn't exceed the buffer size.
> >
> > Either way, I believe this patch is correct, though we may have some
> > other cleanup work to do in this area.
> >
> > --
> > Jeff Layton <jlayton@...hat.com>
> 
> 
> 


-- 
Jeff Layton <jlayton@...hat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/