[PATCH 1/2] cifs: fix length vs. total_read confusion in cifs_demultiplex_thread (try #2)

[PATCH 1/2] cifs: fix length vs. total_read confusion in cifs_demultiplex_thread (try #2)

[Jeff Layton]

length at this point is the length returned by the last kernel_recvmsg
call. total_read is the length of all of the data read so far. length
is more or less meaningless at this point, so use total_read for
everything.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
---
 fs/cifs/connect.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 47034af..41a0ba0 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -581,12 +581,12 @@ incomplete_rcv:
 		else if (reconnect == 1)
 			continue;
 
-		length += 4; /* account for rfc1002 hdr */
+		total_read += 4; /* account for rfc1002 hdr */
 
-
-		dump_smb(smb_buffer, length);
-		if (checkSMB(smb_buffer, smb_buffer->Mid, total_read+4)) {
-			cifs_dump_mem("Bad SMB: ", smb_buffer, 48);
+		dump_smb(smb_buffer, total_read);
+		if (checkSMB(smb_buffer, smb_buffer->Mid, total_read)) {
+			cifs_dump_mem("Bad SMB: ", smb_buffer,
+					total_read < 48 ? total_read : 48);
 			continue;
 		}
 
-- 
1.7.3.4

[PATCH 2/2] cifs: don't always drop malformed replies on the floor (try #2)

[Jeff Layton]

After receiving a packet, we currently check the header. If it's no
good, then we toss it out and continue the loop, leaving the caller
waiting on that response. This is problematic now that the client waits
indefinitely for responses.

Check first to see if the packet is big enough for us to read the Mid.
If it's not, then discard it since we can't do anything with it anyway.

If it is big enough, then go ahead and do the checkSMB checks. Don't
immediately discard the packet however if they fail. Instead, find the
matching mid_q_entry, mark it as having a malformed response and issue
the callback.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsglob.h  |    2 +-
 fs/cifs/connect.c   |   27 ++++++++++++++++++++++-----
 fs/cifs/transport.c |    3 +++
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 921b884..8034da1 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -652,7 +652,7 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
 #define   MID_REQUEST_SUBMITTED 2
 #define   MID_RESPONSE_RECEIVED 4
 #define   MID_RETRY_NEEDED      8 /* session closed while this request out */
-#define   MID_NO_RESP_NEEDED 0x10
+#define   MID_RESPONSE_MALFORMED 0x10
 
 /* Types of response buffer returned from SendReceive2 */
 #define   CIFS_NO_BUFFER        0    /* Response buffer not returned */
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 41a0ba0..79c8195 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -584,11 +584,19 @@ incomplete_rcv:
 		total_read += 4; /* account for rfc1002 hdr */
 
 		dump_smb(smb_buffer, total_read);
-		if (checkSMB(smb_buffer, smb_buffer->Mid, total_read)) {
+
+		/*
+		 * We know that we received enough to get to the MID as we
+		 * checked the pdu_length earlier. Now check to see
+		 * if the rest of the header is OK. We borrow the length
+		 * var for the rest of the loop to avoid a new stack var.
+		 *
+		 * FIXME: why 48 bytes?
+		 */
+		length = checkSMB(smb_buffer, smb_buffer->Mid, total_read);
+		if (length != 0)
 			cifs_dump_mem("Bad SMB: ", smb_buffer,
 					total_read < 48 ? total_read : 48);
-			continue;
-		}
 
 		mid_entry = NULL;
 		server->lstrp = jiffies;
@@ -600,7 +608,8 @@ incomplete_rcv:
 			if ((mid_entry->mid == smb_buffer->Mid) &&
 			    (mid_entry->midState == MID_REQUEST_SUBMITTED) &&
 			    (mid_entry->command == smb_buffer->Command)) {
-				if (check2ndT2(smb_buffer,server->maxBuf) > 0) {
+				if (length == 0 &&
+				    check2ndT2(smb_buffer,server->maxBuf) > 0) {
 					/* We have a multipart transact2 resp */
 					isMultiRsp = true;
 					if (mid_entry->resp_buf) {
@@ -635,7 +644,12 @@ incomplete_rcv:
 				mid_entry->resp_buf = smb_buffer;
 				mid_entry->largeBuf = isLargeBuf;
 multi_t2_fnd:
-				mid_entry->midState = MID_RESPONSE_RECEIVED;
+				if (length == 0)
+					mid_entry->midState =
+							MID_RESPONSE_RECEIVED;
+				else
+					mid_entry->midState =
+							MID_RESPONSE_MALFORMED;
 				list_del_init(&mid_entry->qhead);
 				mid_entry->callback(mid_entry);
 #ifdef CONFIG_CIFS_STATS2
@@ -656,6 +670,9 @@ multi_t2_fnd:
 				else
 					smallbuf = NULL;
 			}
+		} else if (length != 0) {
+			/* response sanity checks failed */
+			continue;
 		} else if (!is_valid_oplock_break(smb_buffer, server) &&
 			   !isMultiRsp) {
 			cERROR(1, "No task to wake, unknown frame received! "
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 748b3b8..14312e0 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -453,6 +453,9 @@ sync_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
 	case MID_RETRY_NEEDED:
 		rc = -EAGAIN;
 		break;
+	case MID_RESPONSE_MALFORMED:
+		rc = -EIO;
+		break;
 	default:
 		cERROR(1, "%s: invalid mid state mid=%d state=%d", __func__,
 			mid->mid, mid->midState);
-- 
1.7.3.4

Re: [PATCH 1/2] cifs: fix length vs. total_read confusion in cifs_demultiplex_thread (try #2)

[Shirish Pargaonkar]

On Thu, Jan 27, 2011 at 10:41 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> length at this point is the length returned by the last kernel_recvmsg
> call. total_read is the length of all of the data read so far. length
> is more or less meaningless at this point, so use total_read for
> everything.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
>  fs/cifs/connect.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 47034af..41a0ba0 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -581,12 +581,12 @@ incomplete_rcv:
>                else if (reconnect == 1)
>                        continue;
>
> -               length += 4; /* account for rfc1002 hdr */
> +               total_read += 4; /* account for rfc1002 hdr */
>
> -
> -               dump_smb(smb_buffer, length);
> -               if (checkSMB(smb_buffer, smb_buffer->Mid, total_read+4)) {
> -                       cifs_dump_mem("Bad SMB: ", smb_buffer, 48);
> +               dump_smb(smb_buffer, total_read);
> +               if (checkSMB(smb_buffer, smb_buffer->Mid, total_read)) {
> +                       cifs_dump_mem("Bad SMB: ", smb_buffer,
> +                                       total_read < 48 ? total_read : 48);
>                        continue;

Looks correct except that (and this is not a change in this patch) but
would be nice to know what is 48.

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

>                }
>
> --
> 1.7.3.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 2/2] cifs: don't always drop malformed replies on the floor (try #2)

[Shirish Pargaonkar]

On Thu, Jan 27, 2011 at 10:41 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> After receiving a packet, we currently check the header. If it's no
> good, then we toss it out and continue the loop, leaving the caller
> waiting on that response. This is problematic now that the client waits
> indefinitely for responses.
>
> Check first to see if the packet is big enough for us to read the Mid.
> If it's not, then discard it since we can't do anything with it anyway.
>
> If it is big enough, then go ahead and do the checkSMB checks. Don't
> immediately discard the packet however if they fail. Instead, find the
> matching mid_q_entry, mark it as having a malformed response and issue
> the callback.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifsglob.h  |    2 +-
>  fs/cifs/connect.c   |   27 ++++++++++++++++++++++-----
>  fs/cifs/transport.c |    3 +++
>  3 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 921b884..8034da1 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -652,7 +652,7 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
>  #define   MID_REQUEST_SUBMITTED 2
>  #define   MID_RESPONSE_RECEIVED 4
>  #define   MID_RETRY_NEEDED      8 /* session closed while this request out */
> -#define   MID_NO_RESP_NEEDED 0x10
> +#define   MID_RESPONSE_MALFORMED 0x10
>
>  /* Types of response buffer returned from SendReceive2 */
>  #define   CIFS_NO_BUFFER        0    /* Response buffer not returned */
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 41a0ba0..79c8195 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -584,11 +584,19 @@ incomplete_rcv:
>                total_read += 4; /* account for rfc1002 hdr */
>
>                dump_smb(smb_buffer, total_read);
> -               if (checkSMB(smb_buffer, smb_buffer->Mid, total_read)) {
> +
> +               /*
> +                * We know that we received enough to get to the MID as we
> +                * checked the pdu_length earlier. Now check to see
> +                * if the rest of the header is OK. We borrow the length
> +                * var for the rest of the loop to avoid a new stack var.
> +                *
> +                * FIXME: why 48 bytes?
> +                */
> +               length = checkSMB(smb_buffer, smb_buffer->Mid, total_read);
> +               if (length != 0)
>                        cifs_dump_mem("Bad SMB: ", smb_buffer,
>                                        total_read < 48 ? total_read : 48);
> -                       continue;
> -               }
>
>                mid_entry = NULL;
>                server->lstrp = jiffies;
> @@ -600,7 +608,8 @@ incomplete_rcv:
>                        if ((mid_entry->mid == smb_buffer->Mid) &&
>                            (mid_entry->midState == MID_REQUEST_SUBMITTED) &&
>                            (mid_entry->command == smb_buffer->Command)) {
> -                               if (check2ndT2(smb_buffer,server->maxBuf) > 0) {
> +                               if (length == 0 &&
> +                                   check2ndT2(smb_buffer,server->maxBuf) > 0) {
>                                        /* We have a multipart transact2 resp */
>                                        isMultiRsp = true;
>                                        if (mid_entry->resp_buf) {
> @@ -635,7 +644,12 @@ incomplete_rcv:
>                                mid_entry->resp_buf = smb_buffer;
>                                mid_entry->largeBuf = isLargeBuf;
>  multi_t2_fnd:
> -                               mid_entry->midState = MID_RESPONSE_RECEIVED;
> +                               if (length == 0)
> +                                       mid_entry->midState =
> +                                                       MID_RESPONSE_RECEIVED;
> +                               else
> +                                       mid_entry->midState =
> +                                                       MID_RESPONSE_MALFORMED;

Should the decision to assign a value to midState be based on length
as well return value of function check2ndT2() or basing it on just the
value of the length is sufficient?

>                                list_del_init(&mid_entry->qhead);
>                                mid_entry->callback(mid_entry);
>  #ifdef CONFIG_CIFS_STATS2
> @@ -656,6 +670,9 @@ multi_t2_fnd:
>                                else
>                                        smallbuf = NULL;
>                        }
> +               } else if (length != 0) {
> +                       /* response sanity checks failed */
> +                       continue;
>                } else if (!is_valid_oplock_break(smb_buffer, server) &&
>                           !isMultiRsp) {
>                        cERROR(1, "No task to wake, unknown frame received! "
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 748b3b8..14312e0 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -453,6 +453,9 @@ sync_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
>        case MID_RETRY_NEEDED:
>                rc = -EAGAIN;
>                break;
> +       case MID_RESPONSE_MALFORMED:
> +               rc = -EIO;
> +               break;
>        default:
>                cERROR(1, "%s: invalid mid state mid=%d state=%d", __func__,
>                        mid->mid, mid->midState);
> --
> 1.7.3.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>