[PATCH 0/5] cifs: fix some bounds checking problems

[PATCH 0/5] cifs: fix some bounds checking problems

[Jeff Layton]

This patchset fixes a number of problems that David pointed out while
reviewing some backports for RHEL. I think they're appropriate for
2.6.39 and probably the stable series as well.

Jeff Layton (5):
  cifs: change bleft in decode_unicode_ssetup back to signed type
  cifs: check for bytes_remaining going to zero in CIFS_SessSetup
  cifs: sanitize length checking in coalesce_t2
  cifs: refactor mid finding loop in cifs_demultiplex_thread
  cifs: handle errors from coalesce_t2

 fs/cifs/connect.c |  117 +++++++++++++++++++++++++++++-----------------------
 fs/cifs/sess.c    |   19 ++-------
 2 files changed, 69 insertions(+), 67 deletions(-)

-- 
1.7.4.4

[PATCH 1/5] cifs: change bleft in decode_unicode_ssetup back to signed type

[Jeff Layton]

The buffer length checks in this function depend on this value being a
signed data type, but 690c522fa converted it to an unsigned type.

Also, eliminate a problem with the null termination check in the same
function. cifs_strndup_from_ucs handles that situation correctly
already, and the existing check could potentially lead to a buffer
overrun since it increments bleft without checking to see whether it
falls off the end of the buffer.

Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Reported-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/sess.c |   15 +--------------
 1 files changed, 1 insertions(+), 14 deletions(-)

diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index f6728eb..2e2c911 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -276,7 +276,7 @@ static void ascii_ssetup_strings(char **pbcc_area, struct cifsSesInfo *ses,
 }
 
 static void
-decode_unicode_ssetup(char **pbcc_area, __u16 bleft, struct cifsSesInfo *ses,
+decode_unicode_ssetup(char **pbcc_area, int bleft, struct cifsSesInfo *ses,
 		      const struct nls_table *nls_cp)
 {
 	int len;
@@ -284,19 +284,6 @@ decode_unicode_ssetup(char **pbcc_area, __u16 bleft, struct cifsSesInfo *ses,
 
 	cFYI(1, "bleft %d", bleft);
 
-	/*
-	 * Windows servers do not always double null terminate their final
-	 * Unicode string. Check to see if there are an uneven number of bytes
-	 * left. If so, then add an extra NULL pad byte to the end of the
-	 * response.
-	 *
-	 * See section 2.7.2 in "Implementing CIFS" for details
-	 */
-	if (bleft % 2) {
-		data[bleft] = 0;
-		++bleft;
-	}
-
 	kfree(ses->serverOS);
 	ses->serverOS = cifs_strndup_from_ucs(data, bleft, true, nls_cp);
 	cFYI(1, "serverOS=%s", ses->serverOS);
-- 
1.7.4.4

[PATCH 2/5] cifs: check for bytes_remaining going to zero in CIFS_SessSetup

[Jeff Layton]

It's possible that when we go to decode the string area in the
SESSION_SETUP response, that bytes_remaining will be 0. Decrementing it at
that point will mean that it can go "negative" and wrap. Check for a
bytes_remaining value of 0, and don't try to decode the string area if
that's the case.

Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Reported-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/sess.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 2e2c911..645114a 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -916,7 +916,9 @@ ssetup_ntlmssp_authenticate:
 	}
 
 	/* BB check if Unicode and decode strings */
-	if (smb_buf->Flags2 & SMBFLG2_UNICODE) {
+	if (bytes_remaining == 0) {
+		/* no string area to decode, do nothing */
+	} else if (smb_buf->Flags2 & SMBFLG2_UNICODE) {
 		/* unicode string area must be word-aligned */
 		if (((unsigned long) bcc_ptr - (unsigned long) smb_buf) % 2) {
 			++bcc_ptr;
-- 
1.7.4.4

[PATCH 3/5] cifs: sanitize length checking in coalesce_t2

[Jeff Layton]

There are a couple of places in this code where these values can wrap or
go negative, and that could potentially end up overflowing the buffer.
Ensure that that doesn't happen. Do all of the length calculation and
checks first, and only perform the memcpy after they pass.

Also, increase some of the field sizes to 32 bits to ensure that they
don't wrap without being detected.

Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Reported-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index db9d55b..a038f29 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -274,7 +274,8 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB)
 	char *data_area_of_target;
 	char *data_area_of_buf2;
 	int remaining;
-	__u16 byte_count, total_data_size, total_in_buf, total_in_buf2;
+	unsigned int byte_count, total_in_buf;
+	__u16 total_data_size, total_in_buf2;
 
 	total_data_size = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
 
@@ -308,20 +309,28 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB)
 	data_area_of_target += total_in_buf;
 
 	/* copy second buffer into end of first buffer */
-	memcpy(data_area_of_target, data_area_of_buf2, total_in_buf2);
 	total_in_buf += total_in_buf2;
+	/* did this field "wrap" ? */
+	if (total_in_buf & ~((1<<16)-1))
+		return -EINVAL;
 	put_unaligned_le16(total_in_buf, &pSMBt->t2_rsp.DataCount);
+
 	byte_count = get_bcc_le(pTargetSMB);
 	byte_count += total_in_buf2;
+	/* did this field "wrap" ? */
+	if (byte_count & ~((1<<16)-1))
+		return -EINVAL;
 	put_bcc_le(byte_count, pTargetSMB);
 
 	byte_count = pTargetSMB->smb_buf_length;
 	byte_count += total_in_buf2;
-
-	/* BB also add check that we are not beyond maximum buffer size */
-
+	/* don't allow buffer to overflow */
+	if (byte_count > CIFSMaxBufSize)
+		return -EINVAL;
 	pTargetSMB->smb_buf_length = byte_count;
 
+	memcpy(data_area_of_target, data_area_of_buf2, total_in_buf2);
+
 	if (remaining == total_in_buf2) {
 		cFYI(1, "found the last secondary response");
 		return 0; /* we are done */
-- 
1.7.4.4

[PATCH 4/5] cifs: refactor mid finding loop in cifs_demultiplex_thread

[Jeff Layton]

...to reduce the extreme indentation. This should introduce no
behavioral changes.

Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |   92 ++++++++++++++++++++++++++--------------------------
 1 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index a038f29..a2cde4c 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -616,59 +616,59 @@ incomplete_rcv:
 		list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
 			mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
 
-			if ((mid_entry->mid == smb_buffer->Mid) &&
-			    (mid_entry->midState == MID_REQUEST_SUBMITTED) &&
-			    (mid_entry->command == smb_buffer->Command)) {
-				if (length == 0 &&
-				   check2ndT2(smb_buffer, server->maxBuf) > 0) {
-					/* We have a multipart transact2 resp */
-					isMultiRsp = true;
-					if (mid_entry->resp_buf) {
-						/* merge response - fix up 1st*/
-						if (coalesce_t2(smb_buffer,
+			if (mid_entry->mid != smb_buffer->Mid ||
+			    mid_entry->midState != MID_REQUEST_SUBMITTED ||
+			    mid_entry->command != smb_buffer->Command) {
+				mid_entry = NULL;
+				continue;
+			}
+
+			if (length == 0 &&
+			    check2ndT2(smb_buffer, server->maxBuf) > 0) {
+				/* We have a multipart transact2 resp */
+				isMultiRsp = true;
+				if (mid_entry->resp_buf) {
+					/* merge response - fix up 1st*/
+					if (coalesce_t2(smb_buffer,
 							mid_entry->resp_buf)) {
-							mid_entry->multiRsp =
-								 true;
-							break;
-						} else {
-							/* all parts received */
-							mid_entry->multiEnd =
-								 true;
-							goto multi_t2_fnd;
-						}
+						mid_entry->multiRsp = true;
+						break;
 					} else {
-						if (!isLargeBuf) {
-							cERROR(1, "1st trans2 resp needs bigbuf");
-					/* BB maybe we can fix this up,  switch
-					   to already allocated large buffer? */
-						} else {
-							/* Have first buffer */
-							mid_entry->resp_buf =
-								 smb_buffer;
-							mid_entry->largeBuf =
-								 true;
-							bigbuf = NULL;
-						}
+						/* all parts received */
+						mid_entry->multiEnd = true;
+						goto multi_t2_fnd;
+					}
+				} else {
+					if (!isLargeBuf) {
+						/*
+						 * FIXME: switch to already
+						 *        allocated largebuf?
+						 */
+						cERROR(1, "1st trans2 resp "
+							  "needs bigbuf");
+					} else {
+						/* Have first buffer */
+						mid_entry->resp_buf =
+							 smb_buffer;
+						mid_entry->largeBuf = true;
+						bigbuf = NULL;
 					}
-					break;
 				}
-				mid_entry->resp_buf = smb_buffer;
-				mid_entry->largeBuf = isLargeBuf;
+				break;
+			}
+			mid_entry->resp_buf = smb_buffer;
+			mid_entry->largeBuf = isLargeBuf;
 multi_t2_fnd:
-				if (length == 0)
-					mid_entry->midState =
-							MID_RESPONSE_RECEIVED;
-				else
-					mid_entry->midState =
-							MID_RESPONSE_MALFORMED;
+			if (length == 0)
+				mid_entry->midState = MID_RESPONSE_RECEIVED;
+			else
+				mid_entry->midState = MID_RESPONSE_MALFORMED;
 #ifdef CONFIG_CIFS_STATS2
-				mid_entry->when_received = jiffies;
+			mid_entry->when_received = jiffies;
 #endif
-				list_del_init(&mid_entry->qhead);
-				mid_entry->callback(mid_entry);
-				break;
-			}
-			mid_entry = NULL;
+			list_del_init(&mid_entry->qhead);
+			mid_entry->callback(mid_entry);
+			break;
 		}
 		spin_unlock(&GlobalMid_Lock);
 
-- 
1.7.4.4

[PATCH 5/5] cifs: handle errors from coalesce_t2

[Jeff Layton]

cifs_demultiplex_thread calls coalesce_t2 to try and merge follow-on t2
responses into the original mid buffer. coalesce_t2 however can return
errors, but the caller doesn't handle that situation properly. Fix the
thread to treat such a case as it would a malformed packet. Mark the
mid as being malformed and issue the callback.

Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index a2cde4c..d025249 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -629,12 +629,16 @@ incomplete_rcv:
 				isMultiRsp = true;
 				if (mid_entry->resp_buf) {
 					/* merge response - fix up 1st*/
-					if (coalesce_t2(smb_buffer,
-							mid_entry->resp_buf)) {
+					length = coalesce_t2(smb_buffer,
+							mid_entry->resp_buf);
+					if (length > 0) {
+						length = 0;
 						mid_entry->multiRsp = true;
 						break;
 					} else {
-						/* all parts received */
+						/* all parts received or
+						 * packet is malformed
+						 */
 						mid_entry->multiEnd = true;
 						goto multi_t2_fnd;
 					}
-- 
1.7.4.4

Re: [PATCH 0/5] cifs: fix some bounds checking problems

[Jeff Layton]

On Tue, 26 Apr 2011 08:03:16 -0400
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> This patchset fixes a number of problems that David pointed out while
> reviewing some backports for RHEL. I think they're appropriate for
> 2.6.39 and probably the stable series as well.
> 
> Jeff Layton (5):
>   cifs: change bleft in decode_unicode_ssetup back to signed type
>   cifs: check for bytes_remaining going to zero in CIFS_SessSetup
>   cifs: sanitize length checking in coalesce_t2
>   cifs: refactor mid finding loop in cifs_demultiplex_thread
>   cifs: handle errors from coalesce_t2
> 
>  fs/cifs/connect.c |  117 +++++++++++++++++++++++++++++-----------------------
>  fs/cifs/sess.c    |   19 ++-------
>  2 files changed, 69 insertions(+), 67 deletions(-)
> 

Note too that some of this patchset has merge conflicts with the patch
to change the BCC to little-endian. I'll plan to respin that patch once
you let me know what you plan to do with this set.

Thanks,
-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH 0/5] cifs: fix some bounds checking problems

[Steve French]

I will try to get the remaining cleanup patches (such as the bcc
little endian patches) remerged over the next few days - that should
make this easier to respin

On Wed, Apr 27, 2011 at 9:17 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Tue, 26 Apr 2011 08:03:16 -0400
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
>> This patchset fixes a number of problems that David pointed out while
>> reviewing some backports for RHEL. I think they're appropriate for
>> 2.6.39 and probably the stable series as well.
>>
>> Jeff Layton (5):
>>   cifs: change bleft in decode_unicode_ssetup back to signed type
>>   cifs: check for bytes_remaining going to zero in CIFS_SessSetup
>>   cifs: sanitize length checking in coalesce_t2
>>   cifs: refactor mid finding loop in cifs_demultiplex_thread
>>   cifs: handle errors from coalesce_t2
>>
>>  fs/cifs/connect.c |  117 +++++++++++++++++++++++++++++-----------------------
>>  fs/cifs/sess.c    |   19 ++-------
>>  2 files changed, 69 insertions(+), 67 deletions(-)
>>
>
> Note too that some of this patchset has merge conflicts with the patch
> to change the BCC to little-endian. I'll plan to respin that patch once
> you let me know what you plan to do with this set.
>
> Thanks,
> --
> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>



-- 
Thanks,

Steve

Re: [PATCH 0/5] cifs: fix some bounds checking problems

[Jeff Layton]

On Wed, 27 Apr 2011 09:59:52 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> I will try to get the remaining cleanup patches (such as the bcc
> little endian patches) remerged over the next few days - that should
> make this easier to respin
> 

Does this mean that you do not intend to push this set for 2.6.39?

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH 0/5] cifs: fix some bounds checking problems

[Steve French]

On Wed, Apr 27, 2011 at 10:04 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Wed, 27 Apr 2011 09:59:52 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> I will try to get the remaining cleanup patches (such as the bcc
>> little endian patches) remerged over the next few days - that should
>> make this easier to respin
>>
>
> Does this mean that you do not intend to push this set for 2.6.39?

the bcc cleanup or the recent bounds checking series?

It could make sense for your recent bounds checking patches
to go in - but I thought that you wanted to wait on those till 2.6.40.




-- 
Thanks,

Steve

Re: [PATCH 0/5] cifs: fix some bounds checking problems

[Steve French]

On Wed, Apr 27, 2011 at 10:06 AM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Apr 27, 2011 at 10:04 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>> On Wed, 27 Apr 2011 09:59:52 -0500
>> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>> I will try to get the remaining cleanup patches (such as the bcc
>>> little endian patches) remerged over the next few days - that should
>>> make this easier to respin
>>>
>>
>> Does this mean that you do not intend to push this set for 2.6.39?
>
> the bcc cleanup or the recent bounds checking series?
>
> It could make sense for your recent bounds checking patches
> to go in - but I thought that you wanted to wait on those till 2.6.40

I had missed the comment about 2.6.39 in your summary.


-- 
Thanks,

Steve

Re: [PATCH 0/5] cifs: fix some bounds checking problems

[Jeff Layton]

On Wed, 27 Apr 2011 10:07:18 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Wed, Apr 27, 2011 at 10:06 AM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Wed, Apr 27, 2011 at 10:04 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> >> On Wed, 27 Apr 2011 09:59:52 -0500
> >> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>
> >>> I will try to get the remaining cleanup patches (such as the bcc
> >>> little endian patches) remerged over the next few days - that should
> >>> make this easier to respin
> >>>
> >>
> >> Does this mean that you do not intend to push this set for 2.6.39?
> >
> > the bcc cleanup or the recent bounds checking series?
> >
> > It could make sense for your recent bounds checking patches
> > to go in - but I thought that you wanted to wait on those till 2.6.40
> 
> I had missed the comment about 2.6.39 in your summary.
> 
> 

Yeah, I think the bounds checking patches are reasonable for 2.6.39.
The patch to convert the BCC to little endian is probably 2.6.40
material at this point.

If you push the bounds checking patches for 2.6.39 though, then the BCC
patch will need to be respun for 2.6.40. It's not a problem, but I need
to know whether you plan to push the bounds checking fixes for 2.6.39
so that I know which one needs to be respun.

Make sense?
-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH 0/5] cifs: fix some bounds checking problems

[Steve French]

I merged the following 3 into cifs-2.6.git (from the versions in your
cifs-2.6.39 branch).  Was curious why the last two weren't in that
branch.

cifs: change bleft in decode_unicode_ssetup back to signed type
cifs: check for bytes_remaining going to zero in CIFS_SessSetup
cifs: sanitize length checking in coalesce_t2

On Tue, Apr 26, 2011 at 7:03 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> This patchset fixes a number of problems that David pointed out while
> reviewing some backports for RHEL. I think they're appropriate for
> 2.6.39 and probably the stable series as well.
>
> Jeff Layton (5):
>  cifs: change bleft in decode_unicode_ssetup back to signed type
>  cifs: check for bytes_remaining going to zero in CIFS_SessSetup
>  cifs: sanitize length checking in coalesce_t2
>  cifs: refactor mid finding loop in cifs_demultiplex_thread
>  cifs: handle errors from coalesce_t2
>
>  fs/cifs/connect.c |  117 +++++++++++++++++++++++++++++-----------------------
>  fs/cifs/sess.c    |   19 ++-------
>  2 files changed, 69 insertions(+), 67 deletions(-)
>
> --
> 1.7.4.4
>
>



-- 
Thanks,

Steve

Re: [PATCH 0/5] cifs: fix some bounds checking problems

[Jeff Layton]

On Fri, 29 Apr 2011 00:05:09 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> I merged the following 3 into cifs-2.6.git (from the versions in your
> cifs-2.6.39 branch).  Was curious why the last two weren't in that
> branch.
> 
> cifs: change bleft in decode_unicode_ssetup back to signed type
> cifs: check for bytes_remaining going to zero in CIFS_SessSetup
> cifs: sanitize length checking in coalesce_t2
> 

That would be because I'm an idiot and pushed the branch to my tree w/o
stg pushing the last two patches onto the stack. They're there now.
Sorry about that...

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

Re: [PATCH 1/5] cifs: change bleft in decode_unicode_ssetup back to signed type

[David Howells]

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

> The buffer length checks in this function depend on this value being a
> signed data type, but 690c522fa converted it to an unsigned type.
> 
> Also, eliminate a problem with the null termination check in the same
> function. cifs_strndup_from_ucs handles that situation correctly
> already, and the existing check could potentially lead to a buffer
> overrun since it increments bleft without checking to see whether it
> falls off the end of the buffer.
> 
> Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> Reported-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Acked-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 2/5] cifs: check for bytes_remaining going to zero in CIFS_SessSetup

[David Howells]

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

> It's possible that when we go to decode the string area in the
> SESSION_SETUP response, that bytes_remaining will be 0. Decrementing it at
> that point will mean that it can go "negative" and wrap. Check for a
> bytes_remaining value of 0, and don't try to decode the string area if
> that's the case.
> 
> Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> Reported-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Acked-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 3/5] cifs: sanitize length checking in coalesce_t2

[David Howells]

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

> -	__u16 byte_count, total_data_size, total_in_buf, total_in_buf2;
> +	unsigned int byte_count, total_in_buf;
> +	__u16 total_data_size, total_in_buf2;

There's no particular need for any of these to be __u16; I'd recommend making
them all unsigned int or size_t.

> +	/* did this field "wrap" ? */
> +	if (total_in_buf & ~((1<<16)-1))
> +		return -EINVAL;

I'd recommend something more like the following:

	/* check the server isn't offering too much data */
	if (total_in_buf > USHRT_MAX)
		return -EINVAL;

rather than calculating a mask.

Also, would EPROTO be a better choice for packet parsing errors than EINVAL?

> +	/* did this field "wrap" ? */
> +	if (byte_count & ~((1<<16)-1))
> +		return -EINVAL;

Ditto.

David

Re: [PATCH 3/5] cifs: sanitize length checking in coalesce_t2

[Jeff Layton]

On Tue, 26 Apr 2011 15:27:32 +0100
David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > -	__u16 byte_count, total_data_size, total_in_buf, total_in_buf2;
> > +	unsigned int byte_count, total_in_buf;
> > +	__u16 total_data_size, total_in_buf2;
> 
> There's no particular need for any of these to be __u16; I'd recommend making
> them all unsigned int or size_t.
> 

Fair enough. For those two, they can be __u16 with no problem, AFAICT.
Is there some reason that an unsigned int would be better here?

> > +	/* did this field "wrap" ? */
> > +	if (total_in_buf & ~((1<<16)-1))
> > +		return -EINVAL;
> 
> I'd recommend something more like the following:
> 
> 	/* check the server isn't offering too much data */
> 	if (total_in_buf > USHRT_MAX)
> 		return -EINVAL;
> 
> rather than calculating a mask.
> 

Ahh, didn't know about USHRT_MAX, I'll change it to use that instead.

> Also, would EPROTO be a better choice for packet parsing errors than EINVAL?
> 
> > +	/* did this field "wrap" ? */
> > +	if (byte_count & ~((1<<16)-1))
> > +		return -EINVAL;
> 
> Ditto.
> 
> David


Probably -- or EIO maybe. This error eventually gets discarded anyway
once we mark the mid as malformed, so it really doesn't matter much for
this. Still, more specific errors are better so maybe I'll change this
when I respin it.

Thanks for the review.

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

[PATCH 3/5] cifs: sanitize length checking in coalesce_t2 (try #2)

[Jeff Layton]

There are a couple of places in this code where these values can wrap or
go negative, and that could potentially end up overflowing the buffer.
Ensure that that doesn't happen. Do all of the length calculation and
checks first, and only perform the memcpy after they pass.

Also, increase some stack variables to 32 bits to ensure that they don't
wrap without being detected.

Finally, change the error codes to be a bit more descriptive of any
problems detected. -EINVAL isn't very accurate.

Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Reported-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index db9d55b..77031a3 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -274,7 +274,8 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB)
 	char *data_area_of_target;
 	char *data_area_of_buf2;
 	int remaining;
-	__u16 byte_count, total_data_size, total_in_buf, total_in_buf2;
+	unsigned int byte_count, total_in_buf;
+	__u16 total_data_size, total_in_buf2;
 
 	total_data_size = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
 
@@ -287,7 +288,7 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB)
 	remaining = total_data_size - total_in_buf;
 
 	if (remaining < 0)
-		return -EINVAL;
+		return -EPROTO;
 
 	if (remaining == 0) /* nothing to do, ignore */
 		return 0;
@@ -308,20 +309,30 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB)
 	data_area_of_target += total_in_buf;
 
 	/* copy second buffer into end of first buffer */
-	memcpy(data_area_of_target, data_area_of_buf2, total_in_buf2);
 	total_in_buf += total_in_buf2;
+
+	/* is the result too big for the field? */
+	if (total_in_buf & USHRT_MAX)
+		return -EPROTO;
 	put_unaligned_le16(total_in_buf, &pSMBt->t2_rsp.DataCount);
+
 	byte_count = get_bcc_le(pTargetSMB);
 	byte_count += total_in_buf2;
+
+	/* is the result too big for the field? */
+	if (byte_count & USHRT_MAX)
+		return -EPROTO;
 	put_bcc_le(byte_count, pTargetSMB);
 
 	byte_count = pTargetSMB->smb_buf_length;
 	byte_count += total_in_buf2;
-
-	/* BB also add check that we are not beyond maximum buffer size */
-
+	/* don't allow buffer to overflow */
+	if (byte_count > CIFSMaxBufSize)
+		return -ENOBUFS;
 	pTargetSMB->smb_buf_length = byte_count;
 
+	memcpy(data_area_of_target, data_area_of_buf2, total_in_buf2);
+
 	if (remaining == total_in_buf2) {
 		cFYI(1, "found the last secondary response");
 		return 0; /* we are done */
-- 
1.7.4.4

Re: [PATCH 3/5] cifs: sanitize length checking in coalesce_t2 (try #2)

[David Howells]

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

> +	/* don't allow buffer to overflow */
> +	if (byte_count > CIFSMaxBufSize)
> +		return -ENOBUFS;

Shouldn't that be EPROTO too?  (ENOBUFS would seem to be wrong anyway).

> +	if (total_in_buf & USHRT_MAX)
> +	if (byte_count & USHRT_MAX)

Use '>' rather than '&'.  '&' is wrong without a '~'.

David

Re: [PATCH 3/5] cifs: sanitize length checking in coalesce_t2 (try #2)

[David Howells]

David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > +	/* don't allow buffer to overflow */
> > +	if (byte_count > CIFSMaxBufSize)
> > +		return -ENOBUFS;
> 
> Shouldn't that be EPROTO too?  (ENOBUFS would seem to be wrong anyway).

No...  It should be ENOBUFS.  It's not a protocol error, but a local
limitation.

David

Re: [PATCH 3/5] cifs: sanitize length checking in coalesce_t2 (try #2)

[Jeff Layton]

On Wed, 27 Apr 2011 17:37:12 +0100
David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > +	/* don't allow buffer to overflow */
> > +	if (byte_count > CIFSMaxBufSize)
> > +		return -ENOBUFS;
> 
> Shouldn't that be EPROTO too?  (ENOBUFS would seem to be wrong anyway).
> 

No, CIFSMaxBufSize is a limitation of this code, and not a protocol
limitation. In this case, there's not enough space in this buffer so it
seems like the correct error.

> > +	if (total_in_buf & USHRT_MAX)
> > +	if (byte_count & USHRT_MAX)
> 
> Use '>' rather than '&'.  '&' is wrong without a '~'.
> 

Doh! Good catch -- will fix...

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

[PATCH] cifs: sanitize length checking in coalesce_t2 (try #3)

[Jeff Layton]

There are a couple of places in this code where these values can wrap or
go negative, and that could potentially end up overflowing the buffer.
Ensure that that doesn't happen. Do all of the length calculation and
checks first, and only perform the memcpy after they pass.

Also, increase some stack variables to 32 bits to ensure that they don't
wrap without being detected.

Finally, change the error codes to be a bit more descriptive of any
problems detected. -EINVAL isn't very accurate.

Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Reported-and-Acked-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 4bc862a..8b75a8ec 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -274,7 +274,8 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB)
 	char *data_area_of_target;
 	char *data_area_of_buf2;
 	int remaining;
-	__u16 byte_count, total_data_size, total_in_buf, total_in_buf2;
+	unsigned int byte_count, total_in_buf;
+	__u16 total_data_size, total_in_buf2;
 
 	total_data_size = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
 
@@ -287,7 +288,7 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB)
 	remaining = total_data_size - total_in_buf;
 
 	if (remaining < 0)
-		return -EINVAL;
+		return -EPROTO;
 
 	if (remaining == 0) /* nothing to do, ignore */
 		return 0;
@@ -308,20 +309,29 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB)
 	data_area_of_target += total_in_buf;
 
 	/* copy second buffer into end of first buffer */
-	memcpy(data_area_of_target, data_area_of_buf2, total_in_buf2);
 	total_in_buf += total_in_buf2;
+	/* is the result too big for the field? */
+	if (total_in_buf > USHRT_MAX)
+		return -EPROTO;
 	put_unaligned_le16(total_in_buf, &pSMBt->t2_rsp.DataCount);
+
+	/* fix up the BCC */
 	byte_count = get_bcc_le(pTargetSMB);
 	byte_count += total_in_buf2;
+	/* is the result too big for the field? */
+	if (byte_count > USHRT_MAX)
+		return -EPROTO;
 	put_bcc_le(byte_count, pTargetSMB);
 
 	byte_count = pTargetSMB->smb_buf_length;
 	byte_count += total_in_buf2;
-
-	/* BB also add check that we are not beyond maximum buffer size */
-
+	/* don't allow buffer to overflow */
+	if (byte_count > CIFSMaxBufSize)
+		return -ENOBUFS;
 	pTargetSMB->smb_buf_length = byte_count;
 
+	memcpy(data_area_of_target, data_area_of_buf2, total_in_buf2);
+
 	if (remaining == total_in_buf2) {
 		cFYI(1, "found the last secondary response");
 		return 0; /* we are done */
-- 
1.7.4.4

Re: [PATCH 4/5] cifs: refactor mid finding loop in cifs_demultiplex_thread

[David Howells]

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

> ...to reduce the extreme indentation. This should introduce no
> behavioral changes.
> 
> Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

In fact, you could go further.  In the code you have:

> +				} else {
> +					if (!isLargeBuf) {
> ...
> +					} else {
> ...
> 					}
>  				}

That could be merged and some more indentation removed:

				} else if (!isLargeBuf) {
...
				} else {
...
				}

Though I acknowledge you may want to keep it for the logical structure.

Anyway:

Acked-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 4/5] cifs: refactor mid finding loop in cifs_demultiplex_thread

[Steve French]

From this series all but the cifs: remove unneeded variable
initialization in cifs_r... (which doesn't need to go in 2.6.39) look
fine and are merged.

On Tue, Apr 26, 2011 at 7:03 AM, Jeff Layton <jlayton@redhat.com> wrote:
> ...to reduce the extreme indentation. This should introduce no
> behavioral changes.
>
> Cc: stable@kernel.org
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/cifs/connect.c |   92 ++++++++++++++++++++++++++--------------------------
>  1 files changed, 46 insertions(+), 46 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index a038f29..a2cde4c 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -616,59 +616,59 @@ incomplete_rcv:
>                list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
>                        mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>
> -                       if ((mid_entry->mid == smb_buffer->Mid) &&
> -                           (mid_entry->midState == MID_REQUEST_SUBMITTED) &&
> -                           (mid_entry->command == smb_buffer->Command)) {
> -                               if (length == 0 &&
> -                                  check2ndT2(smb_buffer, server->maxBuf) > 0) {
> -                                       /* We have a multipart transact2 resp */
> -                                       isMultiRsp = true;
> -                                       if (mid_entry->resp_buf) {
> -                                               /* merge response - fix up 1st*/
> -                                               if (coalesce_t2(smb_buffer,
> +                       if (mid_entry->mid != smb_buffer->Mid ||
> +                           mid_entry->midState != MID_REQUEST_SUBMITTED ||
> +                           mid_entry->command != smb_buffer->Command) {
> +                               mid_entry = NULL;
> +                               continue;
> +                       }
> +
> +                       if (length == 0 &&
> +                           check2ndT2(smb_buffer, server->maxBuf) > 0) {
> +                               /* We have a multipart transact2 resp */
> +                               isMultiRsp = true;
> +                               if (mid_entry->resp_buf) {
> +                                       /* merge response - fix up 1st*/
> +                                       if (coalesce_t2(smb_buffer,
>                                                        mid_entry->resp_buf)) {
> -                                                       mid_entry->multiRsp =
> -                                                                true;
> -                                                       break;
> -                                               } else {
> -                                                       /* all parts received */
> -                                                       mid_entry->multiEnd =
> -                                                                true;
> -                                                       goto multi_t2_fnd;
> -                                               }
> +                                               mid_entry->multiRsp = true;
> +                                               break;
>                                        } else {
> -                                               if (!isLargeBuf) {
> -                                                       cERROR(1, "1st trans2 resp needs bigbuf");
> -                                       /* BB maybe we can fix this up,  switch
> -                                          to already allocated large buffer? */
> -                                               } else {
> -                                                       /* Have first buffer */
> -                                                       mid_entry->resp_buf =
> -                                                                smb_buffer;
> -                                                       mid_entry->largeBuf =
> -                                                                true;
> -                                                       bigbuf = NULL;
> -                                               }
> +                                               /* all parts received */
> +                                               mid_entry->multiEnd = true;
> +                                               goto multi_t2_fnd;
> +                                       }
> +                               } else {
> +                                       if (!isLargeBuf) {
> +                                               /*
> +                                                * FIXME: switch to already
> +                                                *        allocated largebuf?
> +                                                */
> +                                               cERROR(1, "1st trans2 resp "
> +                                                         "needs bigbuf");
> +                                       } else {
> +                                               /* Have first buffer */
> +                                               mid_entry->resp_buf =
> +                                                        smb_buffer;
> +                                               mid_entry->largeBuf = true;
> +                                               bigbuf = NULL;
>                                        }
> -                                       break;
>                                }
> -                               mid_entry->resp_buf = smb_buffer;
> -                               mid_entry->largeBuf = isLargeBuf;
> +                               break;
> +                       }
> +                       mid_entry->resp_buf = smb_buffer;
> +                       mid_entry->largeBuf = isLargeBuf;
>  multi_t2_fnd:
> -                               if (length == 0)
> -                                       mid_entry->midState =
> -                                                       MID_RESPONSE_RECEIVED;
> -                               else
> -                                       mid_entry->midState =
> -                                                       MID_RESPONSE_MALFORMED;
> +                       if (length == 0)
> +                               mid_entry->midState = MID_RESPONSE_RECEIVED;
> +                       else
> +                               mid_entry->midState = MID_RESPONSE_MALFORMED;
>  #ifdef CONFIG_CIFS_STATS2
> -                               mid_entry->when_received = jiffies;
> +                       mid_entry->when_received = jiffies;
>  #endif
> -                               list_del_init(&mid_entry->qhead);
> -                               mid_entry->callback(mid_entry);
> -                               break;
> -                       }
> -                       mid_entry = NULL;
> +                       list_del_init(&mid_entry->qhead);
> +                       mid_entry->callback(mid_entry);
> +                       break;
>                }
>                spin_unlock(&GlobalMid_Lock);
>
> --
> 1.7.4.4
>
>



-- 
Thanks,

Steve

Re: [PATCH 4/5] cifs: refactor mid finding loop in cifs_demultiplex_thread

[Jeff Layton]

On Mon, 2 May 2011 22:44:42 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> From this series all but the cifs: remove unneeded variable
> initialization in cifs_r... (which doesn't need to go in 2.6.39) look
> fine and are merged.
> 

Thanks Steve,

Any idea when you'll update your for-2.6.40 branch? I have some patches
that are based on that branch that will probably need to be respun
now...

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

Re: [PATCH 4/5] cifs: refactor mid finding loop in cifs_demultiplex_thread

[Steve French]

On Tue, May 3, 2011 at 7:09 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Mon, 2 May 2011 22:44:42 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> From this series all but the cifs: remove unneeded variable
>> initialization in cifs_r... (which doesn't need to go in 2.6.39) look
>> fine and are merged.
>>
>
> Thanks Steve,
>
> Any idea when you'll update your for-2.6.40 branch? I have some patches
> that are based on that branch that will probably need to be respun
> now...

I have the current patches for 2.6.40 in "for-next" branch in cifs-2.6.git.



-- 
Thanks,

Steve

Re: [PATCH 5/5] cifs: handle errors from coalesce_t2

[David Howells]

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

> cifs_demultiplex_thread calls coalesce_t2 to try and merge follow-on t2
> responses into the original mid buffer. coalesce_t2 however can return
> errors, but the caller doesn't handle that situation properly. Fix the
> thread to treat such a case as it would a malformed packet. Mark the
> mid as being malformed and issue the callback.
> 
> Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Acked-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>