All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ralph Boehme <slow@samba.org>
To: linux-cifs@vger.kernel.org
Cc: "Namjae Jeon" <linkinjeon@kernel.org>,
	"Tom Talpey" <tom@talpey.com>,
	"Ronnie Sahlberg" <ronniesahlberg@gmail.com>,
	"Ralph Böhme" <slow@samba.org>,
	"Steve French" <smfrench@gmail.com>,
	"Sergey Senozhatsky" <senozhatsky@chromium.org>,
	"Hyunchul Lee" <hyc.lee@gmail.com>
Subject: [PATCH v5 05/20] ksmbd: check strictly data area in ksmbd_smb2_check_message()
Date: Fri,  1 Oct 2021 14:04:06 +0200	[thread overview]
Message-ID: <20211001120421.327245-6-slow@samba.org> (raw)
In-Reply-To: <20211001120421.327245-1-slow@samba.org>

From: Namjae Jeon <linkinjeon@kernel.org>

When invalid data offset and data length in request,
ksmbd_smb2_check_message check strictly and doesn't allow to process such
requests.

Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Acked-by: Hyunchul Lee <hyc.lee@gmail.com>
Reviewed-by: Ralph Boehme <slow@samba.org>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/smb2misc.c | 98 ++++++++++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 51 deletions(-)

diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
index 9aa46bb3e10d..9edd9c161b27 100644
--- a/fs/ksmbd/smb2misc.c
+++ b/fs/ksmbd/smb2misc.c
@@ -80,18 +80,21 @@ static const bool has_smb2_data_area[NUMBER_OF_SMB2_COMMANDS] = {
 };
 
 /*
- * Returns the pointer to the beginning of the data area. Length of the data
- * area and the offset to it (from the beginning of the smb are also returned.
+ * Set length of the data area and the offset to arguments.
+ * if they are invalid, return error.
  */
-static char *smb2_get_data_area_len(int *off, int *len, struct smb2_hdr *hdr)
+static int smb2_get_data_area_len(unsigned int *off, unsigned int *len,
+				  struct smb2_hdr *hdr)
 {
+	int ret = 0;
+
 	*off = 0;
 	*len = 0;
 
 	/* error reqeusts do not have data area */
 	if (hdr->Status && hdr->Status != STATUS_MORE_PROCESSING_REQUIRED &&
 	    (((struct smb2_err_rsp *)hdr)->StructureSize) == SMB2_ERROR_STRUCTURE_SIZE2_LE)
-		return NULL;
+		return ret;
 
 	/*
 	 * Following commands have data areas so we have to get the location
@@ -165,69 +168,60 @@ static char *smb2_get_data_area_len(int *off, int *len, struct smb2_hdr *hdr)
 	case SMB2_IOCTL:
 		*off = le32_to_cpu(((struct smb2_ioctl_req *)hdr)->InputOffset);
 		*len = le32_to_cpu(((struct smb2_ioctl_req *)hdr)->InputCount);
-
 		break;
 	default:
 		ksmbd_debug(SMB, "no length check for command\n");
 		break;
 	}
 
-	/*
-	 * Invalid length or offset probably means data area is invalid, but
-	 * we have little choice but to ignore the data area in this case.
-	 */
 	if (*off > 4096) {
-		ksmbd_debug(SMB, "offset %d too large, data area ignored\n",
-			    *off);
-		*len = 0;
-		*off = 0;
-	} else if (*off < 0) {
-		ksmbd_debug(SMB,
-			    "negative offset %d to data invalid ignore data area\n",
-			    *off);
-		*off = 0;
-		*len = 0;
-	} else if (*len < 0) {
-		ksmbd_debug(SMB,
-			    "negative data length %d invalid, data area ignored\n",
-			    *len);
-		*len = 0;
-	} else if (*len > 128 * 1024) {
-		ksmbd_debug(SMB, "data area larger than 128K: %d\n", *len);
-		*len = 0;
+		ksmbd_debug(SMB, "offset %d too large\n", *off);
+		ret = -EINVAL;
+	} else if ((u64)*off + *len > MAX_STREAM_PROT_LEN) {
+		ksmbd_debug(SMB, "Request is larger than maximum stream protocol length(%u): %llu\n",
+			    MAX_STREAM_PROT_LEN, (u64)*off + *len);
+		ret = -EINVAL;
 	}
 
-	/* return pointer to beginning of data area, ie offset from SMB start */
-	if ((*off != 0) && (*len != 0))
-		return (char *)hdr + *off;
-	else
-		return NULL;
+	return ret;
 }
 
 /*
  * Calculate the size of the SMB message based on the fixed header
  * portion, the number of word parameters and the data portion of the message.
  */
-static unsigned int smb2_calc_size(void *buf)
+static int smb2_calc_size(void *buf, unsigned int *len)
 {
 	struct smb2_pdu *pdu = (struct smb2_pdu *)buf;
 	struct smb2_hdr *hdr = &pdu->hdr;
-	int offset; /* the offset from the beginning of SMB to data area */
-	int data_length; /* the length of the variable length data area */
+	unsigned int offset; /* the offset from the beginning of SMB to data area */
+	unsigned int data_length; /* the length of the variable length data area */
+	int ret;
+
 	/* Structure Size has already been checked to make sure it is 64 */
-	int len = le16_to_cpu(hdr->StructureSize);
+	*len = le16_to_cpu(hdr->StructureSize);
 
 	/*
 	 * StructureSize2, ie length of fixed parameter area has already
 	 * been checked to make sure it is the correct length.
 	 */
-	len += le16_to_cpu(pdu->StructureSize2);
+	*len += le16_to_cpu(pdu->StructureSize2);
+	/*
+	 * StructureSize2 of smb2_lock pdu is set to 48, indicating
+	 * the size of smb2 lock request with single smb2_lock_element
+	 * regardless of number of locks. Subtract single
+	 * smb2_lock_element for correct buffer size check.
+	 */
+	if (hdr->Command == SMB2_LOCK)
+		*len -= sizeof(struct smb2_lock_element);
 
 	if (has_smb2_data_area[le16_to_cpu(hdr->Command)] == false)
 		goto calc_size_exit;
 
-	smb2_get_data_area_len(&offset, &data_length, hdr);
-	ksmbd_debug(SMB, "SMB2 data length %d offset %d\n", data_length,
+	ret = smb2_get_data_area_len(&offset, &data_length, hdr);
+	if (ret)
+		return ret;
+	ksmbd_debug(SMB, "SMB2 data length %u offset %u\n", data_length,
 		    offset);
 
 	if (data_length > 0) {
@@ -237,16 +231,19 @@ static unsigned int smb2_calc_size(void *buf)
 		 * for some commands, typically those with odd StructureSize,
 		 * so we must add one to the calculation.
 		 */
-		if (offset + 1 < len)
+		if (offset + 1 < *len) {
 			ksmbd_debug(SMB,
-				    "data area offset %d overlaps SMB2 header %d\n",
-				    offset + 1, len);
-		else
-			len = offset + data_length;
+				    "data area offset %d overlaps SMB2 header %u\n",
+				    offset + 1, *len);
+			return -EINVAL;
+		}
+
+		*len = offset + data_length;
 	}
+
 calc_size_exit:
-	ksmbd_debug(SMB, "SMB2 len %d\n", len);
-	return len;
+	ksmbd_debug(SMB, "SMB2 len %u\n", *len);
+	return 0;
 }
 
 static inline int smb2_query_info_req_len(struct smb2_query_info_req *h)
@@ -391,9 +388,11 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
 		return 1;
 	}
 
-	clc_len = smb2_calc_size(hdr);
+	if (smb2_calc_size(hdr, &clc_len))
+		return 1;
+
 	if (len != clc_len) {
-		/* server can return one byte more due to implied bcc[0] */
+		/* client can return one byte more due to implied bcc[0] */
 		if (clc_len == len + 1)
 			return 0;
 
@@ -418,9 +417,6 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
 			return 0;
 		}
 
-		if (command == SMB2_LOCK_HE && len == 88)
-			return 0;
-
 		ksmbd_debug(SMB,
 			    "cli req too short, len %d not %d. cmd:%d mid:%llu\n",
 			    len, clc_len, command,
-- 
2.31.1


  parent reply	other threads:[~2021-10-01 12:25 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-01 12:04 [PATCH v5 00/20] Buffer validation patches Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 01/20] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 02/20] ksmbd: add validation in smb2_ioctl Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 03/20] ksmbd: use correct basic info level in set_file_basic_info() Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 04/20] ksmbd: add request buffer validation in smb2_set_info Ralph Boehme
2021-10-01 12:04 ` Ralph Boehme [this message]
2021-10-01 12:04 ` [PATCH v5 06/20] ksmbd: add validation in smb2 negotiate Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 07/20] ksmbd: add buffer validation for SMB2_CREATE_CONTEXT Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 08/20] ksmbd: remove the leftover of smb2.0 dialect support Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 09/20] ksmbd: remove NTLMv1 authentication Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 10/20] ksmbd: fix transform header validation Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 11/20] ksmbd: use ksmbd_req_buf_next() in ksmbd_smb2_check_message() Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 12/20] ksmbd: use ksmbd_req_buf_next() in ksmbd_verify_smb_message() Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 13/20] ksmbd: remove ksmbd_verify_smb_message() Ralph Boehme
2021-10-02  5:46   ` Namjae Jeon
2021-10-02 12:05     ` Ralph Boehme
2021-10-03 23:37     ` Jeremy Allison
2021-10-04  0:47       ` Namjae Jeon
2021-10-01 12:04 ` [PATCH v5 14/20] ksmbd: add ksmbd_smb2_cur_pdu_buflen() Ralph Boehme
2021-10-02  5:49   ` Namjae Jeon
2021-10-02 11:55     ` Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 15/20] ksmbd: use ksmbd_smb2_cur_pdu_buflen() in ksmbd_smb2_check_message() Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 16/20] ksmbd: check PDU len is at least header plus body size " Ralph Boehme
2021-10-02  5:55   ` Namjae Jeon
2021-10-02 11:54     ` Ralph Boehme
2021-10-02 12:45   ` Hyunchul Lee
2021-10-02 12:49     ` Ralph Boehme
2021-10-03  1:25       ` Namjae Jeon
2021-10-01 12:04 ` [PATCH v5 17/20] ksmdb: use cmd helper variable in smb2_get_ksmbd_tcon() Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 18/20] ksmdb: make smb2_get_ksmbd_tcon() callable with chained PDUs Ralph Boehme
2021-10-02  6:00   ` Namjae Jeon
2021-10-02 12:08     ` Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 19/20] ksmbd: make smb2_check_user_session() callabe for compound PDUs Ralph Boehme
2021-10-02  6:01   ` Namjae Jeon
2021-10-02 12:08     ` Ralph Boehme
2021-10-01 12:04 ` [PATCH v5 20/20] ksmdb: move session and tcon validation to ksmbd_smb2_check_message() Ralph Boehme
2021-10-02  6:05 ` [PATCH v5 00/20] Buffer validation patches Namjae Jeon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211001120421.327245-6-slow@samba.org \
    --to=slow@samba.org \
    --cc=hyc.lee@gmail.com \
    --cc=linkinjeon@kernel.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=ronniesahlberg@gmail.com \
    --cc=senozhatsky@chromium.org \
    --cc=smfrench@gmail.com \
    --cc=tom@talpey.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.