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

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

[Jeff Layton]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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

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

[Jeff Layton]

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

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

[Jeff Layton]

On Sun, 1 Jan 2012 17:35:18 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Any easy repro?

Not for me. It seems to depend on a variety of factors including the
infolevel used, the length of the filenames, and possibly their order
in the response.

> On Jan 1, 2012 9:34 AM, "Jeff Layton" <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  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
> >
> >


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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

[Jeff Layton]

On Sun, 1 Jan 2012 17:35:18 -0600
Steve French <smfrench@gmail.com> wrote:

> Any easy repro?

Not for me. It seems to depend on a variety of factors including the
infolevel used, the length of the filenames, and possibly their order
in the response.

> On Jan 1, 2012 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
> >
> >


-- 
Jeff Layton <jlayton@redhat.com>

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

[Shirish Pargaonkar]

On Sun, Jan 1, 2012 at 9:34 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  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
>

Tested-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Tested cifs module built with this patch against a Windows XP server
where the failure was noticed.
But even without this patch, there are no errors against Windows 2003 server
and Windows 2008 server.

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

[Shirish Pargaonkar]

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
>

Tested-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>

Tested cifs module built with this patch against a Windows XP server
where the failure was noticed.
But even without this patch, there are no errors against Windows 2003 server
and Windows 2008 server.

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

[Shirish Pargaonkar]

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) {

It is possible to have large number of entries in a directory such that the
response to a  ls  command can exceed CIFSMaxBufSize.

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

[Jeff Layton]

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

-- 
Jeff Layton <jlayton@redhat.com>

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@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>

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

[Steve French]

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

On Wed, Jan 4, 2012 at 8:09 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, 3 Jan 2012 16:46:15 -0600
> Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Sun, Jan 1, 2012 at 9:34 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > ---
>> >  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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>



-- 
Thanks,

Steve

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

[Steve French]

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

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
> other cleanup work to do in this area.
>
> --
> Jeff Layton <jlayton@redhat.com>



-- 
Thanks,

Steve

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Tue, 3 Jan 2012 16:46:15 -0600
> > Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> On Sun, Jan 1, 2012 at 9:34 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> > ---
> >> >  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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> 
> 


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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@gmail.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@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
> > other cleanup work to do in this area.
> >
> > --
> > Jeff Layton <jlayton@redhat.com>
> 
> 
> 


-- 
Jeff Layton <jlayton@redhat.com>