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

[Shirish Pargaonkar]

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

yes, agree.

> other cleanup work to do in this area.
>
> --
> 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/