lkml.org 
[lkml]   [2012]   [Jan]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] cifs: fix bad buffer length check in coalesce_t2
    From
    On Wed, Jan 4, 2012 at 8:09 AM, Jeff Layton <jlayton@redhat.com> wrote:
    > On Tue, 3 Jan 2012 16:46:15 -0600
    > Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
    >
    >> On Sun, Jan 1, 2012 at 9:34 AM, Jeff Layton <jlayton@redhat.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@gmail.com>
    >> > Signed-off-by: Jeff Layton <jlayton@redhat.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

    yes, agree.

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

    \
     
     \ /
      Last update: 2012-01-04 16:11    [W:0.025 / U:185.664 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site