[PATCH v7 0/9] Buffer validation and compound handling patches

[PATCH v7 0/9] Buffer validation and compound handling patches

[Ralph Boehme]

v2:
  - update comments of smb2_get_data_area_len().
  - fix wrong buffer size check in fsctl_query_iface_info_ioctl().
  - fix 32bit overflow in smb2_set_info.

v3:
  - add buffer check for ByteCount of smb negotiate request.
  - Moved buffer check of to the top of loop to avoid unneeded behavior when
    out_buf_len is smaller than network_interface_info_ioctl_rsp.
  - get correct out_buf_len which doesn't exceed max stream protocol length.
  - subtract single smb2_lock_element for correct buffer size check in
    ksmbd_smb2_check_message().

v4: 
  - use work->response_sz for out_buf_len calculation in smb2_ioctl.
  - move smb2_neg size check to above to validate NegotiateContextOffset
    field.
  - remove unneeded dialect checks in smb2_sess_setup() and
    smb2_handle_negotiate().
  - split smb2_set_info patch into two patches(declaring
    smb2_file_basic_info and buffer check) 

v5:
  - remove PDU size validation from ksmbd_conn_handler_loop()
  - add PDU size validation to ksmbd_smb2_check_message()
  - fix compound non-related request handling

v6:
  - check we can access ProtocolId in ksmbd_verify_smb_message()
  - optimize tcon and session check functions for compound related PDUs
  - drop patch that broke SMB1 negprot
  - check credits after fully validating PDU size

v7:
  - drop header size check in ksmbd_verify_smb_message()
  - fix invalid read when accessing StructureSize2 in
    ksmbd_smb2_check_message()
  - validate credit charge after validating SMB2 PDU body size

Ralph Boehme (9):
  ksmbd: use ksmbd_req_buf_next() in ksmbd_verify_smb_message()
  ksmbd: use ksmbd_req_buf_next() in ksmbd_smb2_check_message()
  ksmbd: add and use ksmbd_smb2_cur_pdu_buflen() in
    ksmbd_smb2_check_message()
  ksmbd: check buffer is big enough to access the SMB2 PUD body size
    field
  ksmdb: validate credit charge after validating SMB2 PDU body size
  ksmdb: use cmd helper variable in smb2_get_ksmbd_tcon()
  ksmdb: make smb2_get_ksmbd_tcon() callable with chained PDUs
  ksmbd: make smb2_check_user_session() callable for compound PDUs
  ksmdb: move session and tcon validation to __process_request()

 fs/ksmbd/ksmbd_work.h |  1 +
 fs/ksmbd/server.c     | 46 +++++++++++++++++++++-------------
 fs/ksmbd/smb2misc.c   | 58 +++++++++++++++++++++++++++----------------
 fs/ksmbd/smb2pdu.c    | 39 +++++++++++++++++++++++------
 fs/ksmbd/smb2pdu.h    |  1 +
 fs/ksmbd/smb_common.c |  2 +-
 6 files changed, 101 insertions(+), 46 deletions(-)

-- 
2.31.1

[PATCH v7 1/9] ksmbd: use ksmbd_req_buf_next() in ksmbd_verify_smb_message()

[Ralph Boehme]

No change in behaviour.

Signed-off-by: Ralph Boehme <slow@samba.org>
---
 fs/ksmbd/smb_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index 707490ab1f4c..e1e5a071678e 100644
--- a/fs/ksmbd/smb_common.c
+++ b/fs/ksmbd/smb_common.c
@@ -132,7 +132,7 @@ int ksmbd_lookup_protocol_idx(char *str)
  */
 int ksmbd_verify_smb_message(struct ksmbd_work *work)
 {
-	struct smb2_hdr *smb2_hdr = work->request_buf + work->next_smb2_rcv_hdr_off;
+	struct smb2_hdr *smb2_hdr = ksmbd_req_buf_next(work);
 	struct smb_hdr *hdr;
 
 	if (smb2_hdr->ProtocolId == SMB2_PROTO_NUMBER)
-- 
2.31.1

[PATCH v7 2/9] ksmbd: use ksmbd_req_buf_next() in ksmbd_smb2_check_message()

[Ralph Boehme]

No change in behaviour.

Cc: Namjae Jeon <linkinjeon@kernel.org>
Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Steve French <smfrench@gmail.com>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Ralph Boehme <slow@samba.org>
---
 fs/ksmbd/smb2misc.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
index 9edd9c161b27..2cc031c39514 100644
--- a/fs/ksmbd/smb2misc.c
+++ b/fs/ksmbd/smb2misc.c
@@ -329,17 +329,12 @@ static int smb2_validate_credit_charge(struct smb2_hdr *hdr)
 
 int ksmbd_smb2_check_message(struct ksmbd_work *work)
 {
-	struct smb2_pdu *pdu = work->request_buf;
+	struct smb2_pdu *pdu = ksmbd_req_buf_next(work);
 	struct smb2_hdr *hdr = &pdu->hdr;
 	int command;
 	__u32 clc_len;  /* calculated length */
 	__u32 len = get_rfc1002_len(pdu);
 
-	if (work->next_smb2_rcv_hdr_off) {
-		pdu = ksmbd_req_buf_next(work);
-		hdr = &pdu->hdr;
-	}
-
 	if (le32_to_cpu(hdr->NextCommand) > 0) {
 		len = le32_to_cpu(hdr->NextCommand);
 	} else if (work->next_smb2_rcv_hdr_off) {
-- 
2.31.1

[PATCH v7 3/9] ksmbd: add and use ksmbd_smb2_cur_pdu_buflen() in ksmbd_smb2_check_message()

[Ralph Boehme]

No change in behaviour.

Cc: Namjae Jeon <linkinjeon@kernel.org>
Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Steve French <smfrench@gmail.com>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Ralph Boehme <slow@samba.org>
---
 fs/ksmbd/smb2misc.c | 36 +++++++++++++++++++++++++++---------
 fs/ksmbd/smb2pdu.h  |  1 +
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
index 2cc031c39514..7ed266eb6c5e 100644
--- a/fs/ksmbd/smb2misc.c
+++ b/fs/ksmbd/smb2misc.c
@@ -333,14 +333,7 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
 	struct smb2_hdr *hdr = &pdu->hdr;
 	int command;
 	__u32 clc_len;  /* calculated length */
-	__u32 len = get_rfc1002_len(pdu);
-
-	if (le32_to_cpu(hdr->NextCommand) > 0) {
-		len = le32_to_cpu(hdr->NextCommand);
-	} else if (work->next_smb2_rcv_hdr_off) {
-		len -= work->next_smb2_rcv_hdr_off;
-		len = round_up(len, 8);
-	}
+	__u32 len = ksmbd_smb2_cur_pdu_buflen(work);
 
 	if (check_smb2_hdr(hdr))
 		return 1;
@@ -395,7 +388,7 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
 		 * Some windows servers (win2016) will pad also the final
 		 * PDU in a compound to 8 bytes.
 		 */
-		if (ALIGN(clc_len, 8) == len)
+		if (ALIGN(clc_len, 8) == ALIGN(len, 8))
 			return 0;
 
 		/*
@@ -427,3 +420,28 @@ int smb2_negotiate_request(struct ksmbd_work *work)
 {
 	return ksmbd_smb_negotiate_common(work, SMB2_NEGOTIATE_HE);
 }
+
+/**
+ * ksmbd_smb2_cur_pdu_buflen() - Get len of current SMB2 PDU buffer
+ * This returns the lenght including any possible padding.
+ * @work: smb work containing request buffer
+ */
+unsigned int ksmbd_smb2_cur_pdu_buflen(struct ksmbd_work *work)
+{
+	struct smb2_hdr *hdr = ksmbd_req_buf_next(work);
+	unsigned int buf_len;
+	unsigned int pdu_len;
+
+	if (hdr->NextCommand != 0) {
+		/*
+		 * hdr->NextCommand has already been validated by
+		 * init_chained_smb2_rsp().
+		 */
+		return __le32_to_cpu(hdr->NextCommand);
+	}
+
+	buf_len = get_rfc1002_len(work->request_buf);
+	pdu_len = buf_len - work->next_smb2_rcv_hdr_off;
+	return pdu_len;
+}
+
diff --git a/fs/ksmbd/smb2pdu.h b/fs/ksmbd/smb2pdu.h
index a6dec5ec6a54..c5fa8256b0bb 100644
--- a/fs/ksmbd/smb2pdu.h
+++ b/fs/ksmbd/smb2pdu.h
@@ -1680,6 +1680,7 @@ int smb2_set_rsp_credits(struct ksmbd_work *work);
 
 /* smb2 misc functions */
 int ksmbd_smb2_check_message(struct ksmbd_work *work);
+unsigned int ksmbd_smb2_cur_pdu_buflen(struct ksmbd_work *work);
 
 /* smb2 command handlers */
 int smb2_handle_negotiate(struct ksmbd_work *work);
-- 
2.31.1

[PATCH v7 4/9] ksmbd: check buffer is big enough to access the SMB2 PUD body size field

[Ralph Boehme]

Cc: Namjae Jeon <linkinjeon@kernel.org>
Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Steve French <smfrench@gmail.com>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Ralph Boehme <slow@samba.org>
---
 fs/ksmbd/smb2misc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
index 7ed266eb6c5e..50521b5a50b5 100644
--- a/fs/ksmbd/smb2misc.c
+++ b/fs/ksmbd/smb2misc.c
@@ -350,6 +350,9 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
 		return 1;
 	}
 
+	if (len < sizeof(struct smb2_pdu) - 4)
+		return 1;
+
 	if (smb2_req_struct_sizes[command] != pdu->StructureSize2) {
 		if (command != SMB2_OPLOCK_BREAK_HE &&
 		    (hdr->Status == 0 || pdu->StructureSize2 != SMB2_ERROR_STRUCTURE_SIZE2_LE)) {
-- 
2.31.1

[PATCH v7 5/9] ksmdb: validate credit charge after validating SMB2 PDU body size

[Ralph Boehme]

smb2_validate_credit_charge() accesses fields in the SMB2 PDU body, but until
smb2_calc_size() is called the PDU has not yet been verified to be large enough
to access the PDU dynamic part length field.

Signed-off-by: Ralph Boehme <slow@samba.org>
---
 fs/ksmbd/smb2misc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
index 50521b5a50b5..1f14120a0e48 100644
--- a/fs/ksmbd/smb2misc.c
+++ b/fs/ksmbd/smb2misc.c
@@ -373,12 +373,6 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
 		}
 	}
 
-	if ((work->conn->vals->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU) &&
-	    smb2_validate_credit_charge(hdr)) {
-		work->conn->ops->set_rsp_status(work, STATUS_INVALID_PARAMETER);
-		return 1;
-	}
-
 	if (smb2_calc_size(hdr, &clc_len))
 		return 1;
 
@@ -416,6 +410,12 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
 		return 1;
 	}
 
+	if ((work->conn->vals->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU) &&
+	    smb2_validate_credit_charge(hdr)) {
+		work->conn->ops->set_rsp_status(work, STATUS_INVALID_PARAMETER);
+		return 1;
+	}
+
 	return 0;
 }
 
-- 
2.31.1

[PATCH v7 6/9] ksmdb: use cmd helper variable in smb2_get_ksmbd_tcon()

[Ralph Boehme]

No change in behaviour.

Cc: Namjae Jeon <linkinjeon@kernel.org>
Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Steve French <smfrench@gmail.com>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Ralph Boehme <slow@samba.org>
---
 fs/ksmbd/smb2pdu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index ed8324f9c2bd..e10ddc1fce09 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -94,12 +94,13 @@ struct channel *lookup_chann_list(struct ksmbd_session *sess, struct ksmbd_conn
 int smb2_get_ksmbd_tcon(struct ksmbd_work *work)
 {
 	struct smb2_hdr *req_hdr = work->request_buf;
+	unsigned int cmd = le16_to_cpu(req_hdr->Command);
 	int tree_id;
 
 	work->tcon = NULL;
-	if (work->conn->ops->get_cmd_val(work) == SMB2_TREE_CONNECT_HE ||
-	    work->conn->ops->get_cmd_val(work) ==  SMB2_CANCEL_HE ||
-	    work->conn->ops->get_cmd_val(work) ==  SMB2_LOGOFF_HE) {
+	if (cmd == SMB2_TREE_CONNECT_HE ||
+	    cmd ==  SMB2_CANCEL_HE ||
+	    cmd ==  SMB2_LOGOFF_HE) {
 		ksmbd_debug(SMB, "skip to check tree connect request\n");
 		return 0;
 	}
-- 
2.31.1

[PATCH v7 7/9] ksmdb: make smb2_get_ksmbd_tcon() callable with chained PDUs

[Ralph Boehme]

Also track the tcon id of compound requests.

Cc: Namjae Jeon <linkinjeon@kernel.org>
Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Steve French <smfrench@gmail.com>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Ralph Boehme <slow@samba.org>
---
 fs/ksmbd/ksmbd_work.h |  1 +
 fs/ksmbd/smb2pdu.c    | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/ksmbd/ksmbd_work.h b/fs/ksmbd/ksmbd_work.h
index f7156bc50049..91363d508909 100644
--- a/fs/ksmbd/ksmbd_work.h
+++ b/fs/ksmbd/ksmbd_work.h
@@ -46,6 +46,7 @@ struct ksmbd_work {
 	u64				compound_fid;
 	u64				compound_pfid;
 	u64				compound_sid;
+	u32				compound_tid;
 
 	const struct cred		*saved_cred;
 
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index e10ddc1fce09..1755a524beb3 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -97,7 +97,6 @@ int smb2_get_ksmbd_tcon(struct ksmbd_work *work)
 	unsigned int cmd = le16_to_cpu(req_hdr->Command);
 	int tree_id;
 
-	work->tcon = NULL;
 	if (cmd == SMB2_TREE_CONNECT_HE ||
 	    cmd ==  SMB2_CANCEL_HE ||
 	    cmd ==  SMB2_LOGOFF_HE) {
@@ -110,13 +109,26 @@ int smb2_get_ksmbd_tcon(struct ksmbd_work *work)
 		return -ENOENT;
 	}
 
+	if (req_hdr->Flags & SMB2_FLAGS_RELATED_OPERATIONS) {
+		if (!work->tcon) {
+			pr_err("Missing tcon\n");
+			return -EINVAL;
+		}
+		return 1;
+	}
+
+	work->tcon = NULL;
+	work->compound_tid = 0;
+
 	tree_id = le32_to_cpu(req_hdr->Id.SyncId.TreeId);
+
 	work->tcon = ksmbd_tree_conn_lookup(work->sess, tree_id);
 	if (!work->tcon) {
 		pr_err("Invalid tid %d\n", tree_id);
 		return -EINVAL;
 	}
 
+	work->compound_tid = tree_id;
 	return 1;
 }
 
-- 
2.31.1

[PATCH v7 8/9] ksmbd: make smb2_check_user_session() callable for compound PDUs

[Ralph Boehme]

Cc: Namjae Jeon <linkinjeon@kernel.org>
Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Steve French <smfrench@gmail.com>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Ralph Boehme <slow@samba.org>
---
 fs/ksmbd/smb2pdu.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 1755a524beb3..c137c1a94b99 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -416,7 +416,6 @@ static void init_chained_smb2_rsp(struct ksmbd_work *work)
 		work->compound_pfid =
 			le64_to_cpu(((struct smb2_create_rsp *)rsp)->
 				PersistentFileId);
-		work->compound_sid = le64_to_cpu(rsp->SessionId);
 	}
 
 	len = get_rfc1002_len(work->response_buf) - work->next_smb2_rsp_hdr_off;
@@ -596,7 +595,6 @@ int smb2_check_user_session(struct ksmbd_work *work)
 	unsigned int cmd = conn->ops->get_cmd_val(work);
 	unsigned long long sess_id;
 
-	work->sess = NULL;
 	/*
 	 * SMB2_ECHO, SMB2_NEGOTIATE, SMB2_SESSION_SETUP command do not
 	 * require a session id, so no need to validate user session's for
@@ -609,11 +607,25 @@ int smb2_check_user_session(struct ksmbd_work *work)
 	if (!ksmbd_conn_good(work))
 		return -EINVAL;
 
+	if (req_hdr->Flags & SMB2_FLAGS_RELATED_OPERATIONS) {
+		if (work->sess) {
+			pr_err("Missing session\n");
+			return -EINVAL;
+		}
+		return 1;
+	}
+
+	work->sess = NULL;
+	work->compound_sid = 0;
+
 	sess_id = le64_to_cpu(req_hdr->SessionId);
+
 	/* Check for validity of user session */
 	work->sess = ksmbd_session_lookup_all(conn, sess_id);
-	if (work->sess)
+	if (work->sess) {
+		work->compound_sid = sess_id;
 		return 1;
+	}
 	ksmbd_debug(SMB, "Invalid user session, Uid %llu\n", sess_id);
 	return -EINVAL;
 }
-- 
2.31.1

[PATCH v7 9/9] ksmdb: move session and tcon validation to __process_request()

[Ralph Boehme]

For compound non-related operations session id and tree id must be taken from
earch PDU.

Cc: Namjae Jeon <linkinjeon@kernel.org>
Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Steve French <smfrench@gmail.com>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Ralph Boehme <slow@samba.org>
---
 fs/ksmbd/server.c | 46 +++++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 17 deletions(-)

diff --git a/fs/ksmbd/server.c b/fs/ksmbd/server.c
index 2a2b2135bfde..5d1ef277653f 100644
--- a/fs/ksmbd/server.c
+++ b/fs/ksmbd/server.c
@@ -101,6 +101,32 @@ static inline int check_conn_state(struct ksmbd_work *work)
 	return 0;
 }
 
+static int check_session_and_tcon(struct ksmbd_work *work)
+{
+	int rc;
+
+	if (work->conn->ops->check_user_session == NULL)
+		return 0;
+
+	rc = work->conn->ops->check_user_session(work);
+	if (rc < 0) {
+		work->conn->ops->set_rsp_status(work,
+						STATUS_USER_SESSION_DELETED);
+		return 1;
+	}
+	if (rc == 0)
+		return 0;
+
+	rc = work->conn->ops->get_ksmbd_tcon(work);
+	if (rc < 0) {
+		work->conn->ops->set_rsp_status(work,
+						STATUS_NETWORK_NAME_DELETED);
+		return 1;
+	}
+
+	return 0;
+}
+
 #define SERVER_HANDLER_CONTINUE		0
 #define SERVER_HANDLER_ABORT		1
 
@@ -117,6 +143,9 @@ static int __process_request(struct ksmbd_work *work, struct ksmbd_conn *conn,
 	if (ksmbd_verify_smb_message(work))
 		return SERVER_HANDLER_ABORT;
 
+	if (check_session_and_tcon(work))
+		return SERVER_HANDLER_ABORT;
+
 	command = conn->ops->get_cmd_val(work);
 	*cmd = command;
 
@@ -184,23 +213,6 @@ static void __handle_ksmbd_work(struct ksmbd_work *work,
 		goto send;
 	}
 
-	if (conn->ops->check_user_session) {
-		rc = conn->ops->check_user_session(work);
-		if (rc < 0) {
-			command = conn->ops->get_cmd_val(work);
-			conn->ops->set_rsp_status(work,
-					STATUS_USER_SESSION_DELETED);
-			goto send;
-		} else if (rc > 0) {
-			rc = conn->ops->get_ksmbd_tcon(work);
-			if (rc < 0) {
-				conn->ops->set_rsp_status(work,
-					STATUS_NETWORK_NAME_DELETED);
-				goto send;
-			}
-		}
-	}
-
 	do {
 		rc = __process_request(work, conn, &command);
 		if (rc == SERVER_HANDLER_ABORT)
-- 
2.31.1

Re: [PATCH v7 1/9] ksmbd: use ksmbd_req_buf_next() in ksmbd_verify_smb_message()

[Namjae Jeon]

2021-10-05 14:03 GMT+09:00, Ralph Boehme <slow@samba.org>:
> No change in behaviour.
>
> Signed-off-by: Ralph Boehme <slow@samba.org>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>

Thanks for your patch!

Re: [PATCH v7 2/9] ksmbd: use ksmbd_req_buf_next() in ksmbd_smb2_check_message()

[Namjae Jeon]

2021-10-05 14:03 GMT+09:00, Ralph Boehme <slow@samba.org>:
> No change in behaviour.
>
> Cc: Namjae Jeon <linkinjeon@kernel.org>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Steve French <smfrench@gmail.com>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Signed-off-by: Ralph Boehme <slow@samba.org>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>

Thanks for your patch!

Re: [PATCH v7 3/9] ksmbd: add and use ksmbd_smb2_cur_pdu_buflen() in ksmbd_smb2_check_message()

[Namjae Jeon]

2021-10-05 14:03 GMT+09:00, Ralph Boehme <slow@samba.org>:
> No change in behaviour.
>
> Cc: Namjae Jeon <linkinjeon@kernel.org>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Steve French <smfrench@gmail.com>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Signed-off-by: Ralph Boehme <slow@samba.org>
> ---
>  fs/ksmbd/smb2misc.c | 36 +++++++++++++++++++++++++++---------
>  fs/ksmbd/smb2pdu.h  |  1 +
>  2 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
> index 2cc031c39514..7ed266eb6c5e 100644
> --- a/fs/ksmbd/smb2misc.c
> +++ b/fs/ksmbd/smb2misc.c
> @@ -333,14 +333,7 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
>  	struct smb2_hdr *hdr = &pdu->hdr;
>  	int command;
>  	__u32 clc_len;  /* calculated length */
> -	__u32 len = get_rfc1002_len(pdu);
> -
> -	if (le32_to_cpu(hdr->NextCommand) > 0) {
> -		len = le32_to_cpu(hdr->NextCommand);
> -	} else if (work->next_smb2_rcv_hdr_off) {
> -		len -= work->next_smb2_rcv_hdr_off;
> -		len = round_up(len, 8);
> -	}
> +	__u32 len = ksmbd_smb2_cur_pdu_buflen(work);
>
>  	if (check_smb2_hdr(hdr))
>  		return 1;
> @@ -395,7 +388,7 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
>  		 * Some windows servers (win2016) will pad also the final
>  		 * PDU in a compound to 8 bytes.
>  		 */
> -		if (ALIGN(clc_len, 8) == len)
> +		if (ALIGN(clc_len, 8) == ALIGN(len, 8))
Can I know why you align rfc1002 len with 8 here ?

Thanks!

Re: [PATCH v7 3/9] ksmbd: add and use ksmbd_smb2_cur_pdu_buflen() in ksmbd_smb2_check_message()

[Ralph Boehme]

[-- Attachment #1.1: Type: text/plain, Size: 702 bytes --]

Am 05.10.21 um 09:29 schrieb Namjae Jeon:
> 2021-10-05 14:03 GMT+09:00, Ralph Boehme <slow@samba.org>:
>> -		if (ALIGN(clc_len, 8) == len)
>> +		if (ALIGN(clc_len, 8) == ALIGN(len, 8))
> Can I know why you align rfc1002 len with 8 here ?

this should match the previous behaviour where for compound requests we 
called round_up(len, 8).

This could be done differently though:

len = ksmbd_smb2_cur_pdu_buflen(work);
if (work->next_smb2_rcv_hdr_off)
         len = ALIGN(len, 8);

and then just do

                 if (ALIGN(clc_len, 8) == len)

-slow

-- 
Ralph Boehme, Samba Team                 https://samba.org/
SerNet Samba Team Lead      https://sernet.de/en/team-samba

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v7 5/9] ksmdb: validate credit charge after validating SMB2 PDU body size

[Namjae Jeon]

2021-10-05 14:03 GMT+09:00, Ralph Boehme <slow@samba.org>:
> smb2_validate_credit_charge() accesses fields in the SMB2 PDU body, but
> until
> smb2_calc_size() is called the PDU has not yet been verified to be large
> enough
> to access the PDU dynamic part length field.
>
> Signed-off-by: Ralph Boehme <slow@samba.org>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>

Thanks for your work!

Re: [PATCH v7 6/9] ksmdb: use cmd helper variable in smb2_get_ksmbd_tcon()

[Namjae Jeon]

2021-10-05 14:03 GMT+09:00, Ralph Boehme <slow@samba.org>:
> No change in behaviour.
>
> Cc: Namjae Jeon <linkinjeon@kernel.org>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Steve French <smfrench@gmail.com>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Signed-off-by: Ralph Boehme <slow@samba.org>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>

Thanks!

Re: [PATCH v7 3/9] ksmbd: add and use ksmbd_smb2_cur_pdu_buflen() in ksmbd_smb2_check_message()

[Namjae Jeon]

2021-10-05 16:46 GMT+09:00, Ralph Boehme <slow@samba.org>:
> Am 05.10.21 um 09:29 schrieb Namjae Jeon:
>> 2021-10-05 14:03 GMT+09:00, Ralph Boehme <slow@samba.org>:
>>> -		if (ALIGN(clc_len, 8) == len)
>>> +		if (ALIGN(clc_len, 8) == ALIGN(len, 8))
>> Can I know why you align rfc1002 len with 8 here ?
>
> this should match the previous behaviour where for compound requests we
> called round_up(len, 8).
>
> This could be done differently though:
>
> len = ksmbd_smb2_cur_pdu_buflen(work);
> if (work->next_smb2_rcv_hdr_off)
>          len = ALIGN(len, 8);
I think that this code is wrong, we don't need to add pad about last
compound *request* length.

Thanks!

>
> and then just do
>
>                  if (ALIGN(clc_len, 8) == len)
>
> -slow
>
> --
> Ralph Boehme, Samba Team                 https://samba.org/
> SerNet Samba Team Lead      https://sernet.de/en/team-samba
>