Re: [PATCH] cifs: fix bad buffer length check in coalesce_t2

[Jeff Layton]

On Wed, 4 Jan 2012 10:14:25 -0600
Steve French <smfrench@xxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > On Tue, 3 Jan 2012 16:46:15 -0600
> > Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> >
> >> On Sun, Jan 1, 2012 at 9:34 AM, Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxx>
> >> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> >> > ---
> >> > Â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@xxxxxxxxxx>
> 
> 
> 


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