[PATCH v6 00/14] Buffer validation patches

[PATCH v6 00/14] Buffer validation 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

Namjae Jeon (4):
  ksmbd: add the check to vaildate if stream protocol length exceeds
    maximum value
  ksmbd: add validation in smb2_ioctl
  ksmbd: check strictly data area in ksmbd_smb2_check_message()
  ksmbd: remove the leftover of smb2.0 dialect support

Ralph Boehme (10):
  ksmbd: use ksmbd_req_buf_next() in ksmbd_verify_smb_message()
  ksmbd: check buffer is big enough to access the ProtocolId field
  ksmbd: use ksmbd_req_buf_next() in ksmbd_smb2_check_message()
  ksmbd: use ksmbd_smb2_cur_pdu_buflen() in ksmbd_smb2_check_message()
  ksmbd: check PDU len is at least header plus body size in
    ksmbd_smb2_check_message()
  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 ksmbd_smb2_check_message()
  ksmdb: validate credit charge after validating SMB2 PDU body size

 fs/ksmbd/connection.c |   9 ++-
 fs/ksmbd/ksmbd_work.h |   1 +
 fs/ksmbd/server.c     |  46 +++++++----
 fs/ksmbd/smb2misc.c   | 152 +++++++++++++++++++-----------------
 fs/ksmbd/smb2ops.c    |   5 --
 fs/ksmbd/smb2pdu.c    | 178 +++++++++++++++++++++++++++++-------------
 fs/ksmbd/smb2pdu.h    |   2 +-
 fs/ksmbd/smb_common.c |  22 +++---
 fs/ksmbd/smb_common.h |   4 +-
 fs/ksmbd/vfs.c        |   2 +-
 fs/ksmbd/vfs.h        |   2 +-
 11 files changed, 256 insertions(+), 167 deletions(-)

-- 
2.31.1

[PATCH v6 01/14] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value

[Ralph Boehme]

From: Namjae Jeon <linkinjeon@kernel.org>

This patch add MAX_STREAM_PROT_LEN macro and check if stream protocol
length exceeds maximum value. opencode pdu size check in
ksmbd_pdu_size_has_room().

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>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/connection.c | 9 +++++----
 fs/ksmbd/smb_common.c | 6 ------
 fs/ksmbd/smb_common.h | 4 ++--
 3 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c
index af086d35398a..e50353c50661 100644
--- a/fs/ksmbd/connection.c
+++ b/fs/ksmbd/connection.c
@@ -296,10 +296,11 @@ int ksmbd_conn_handler_loop(void *p)
 		pdu_size = get_rfc1002_len(hdr_buf);
 		ksmbd_debug(CONN, "RFC1002 header %u bytes\n", pdu_size);
 
-		/* make sure we have enough to get to SMB header end */
-		if (!ksmbd_pdu_size_has_room(pdu_size)) {
-			ksmbd_debug(CONN, "SMB request too short (%u bytes)\n",
-				    pdu_size);
+		/*
+		 * Check if pdu size is valid (min : smb header size,
+		 * max : 0x00FFFFFF).
+		 */
+		if (pdu_size > MAX_STREAM_PROT_LEN) {
 			continue;
 		}
 
diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index db8042a173d0..b6c4c7e960fa 100644
--- a/fs/ksmbd/smb_common.c
+++ b/fs/ksmbd/smb_common.c
@@ -21,7 +21,6 @@ static const char basechars[43] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_-!@#$%";
 #define MAGIC_CHAR '~'
 #define PERIOD '.'
 #define mangle(V) ((char)(basechars[(V) % MANGLE_BASE]))
-#define KSMBD_MIN_SUPPORTED_HEADER_SIZE	(sizeof(struct smb2_hdr))
 
 struct smb_protocol {
 	int		index;
@@ -294,11 +293,6 @@ int ksmbd_init_smb_server(struct ksmbd_work *work)
 	return 0;
 }
 
-bool ksmbd_pdu_size_has_room(unsigned int pdu)
-{
-	return (pdu >= KSMBD_MIN_SUPPORTED_HEADER_SIZE - 4);
-}
-
 int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work, int info_level,
 				      struct ksmbd_file *dir,
 				      struct ksmbd_dir_info *d_info,
diff --git a/fs/ksmbd/smb_common.h b/fs/ksmbd/smb_common.h
index 994abede27e9..6e79e7577f6b 100644
--- a/fs/ksmbd/smb_common.h
+++ b/fs/ksmbd/smb_common.h
@@ -48,6 +48,8 @@
 #define CIFS_DEFAULT_IOSIZE	(64 * 1024)
 #define MAX_CIFS_SMALL_BUFFER_SIZE 448 /* big enough for most */
 
+#define MAX_STREAM_PROT_LEN	0x00FFFFFF
+
 /* Responses when opening a file. */
 #define F_SUPERSEDED	0
 #define F_OPENED	1
@@ -493,8 +495,6 @@ int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects, __le16 dialects_count);
 
 int ksmbd_init_smb_server(struct ksmbd_work *work);
 
-bool ksmbd_pdu_size_has_room(unsigned int pdu);
-
 struct ksmbd_kstat;
 int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work,
 				      int info_level,
-- 
2.31.1

[PATCH v6 02/14] ksmbd: add validation in smb2_ioctl

[Ralph Boehme]

From: Namjae Jeon <linkinjeon@kernel.org>

Add validation for request/response buffer size check in smb2_ioctl and
fsctl_copychunk() take copychunk_ioctl_req pointer and the other arguments
instead of smb2_ioctl_req structure and remove an unused smb2_ioctl_req
argument of fsctl_validate_negotiate_info.

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>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/smb2pdu.c | 105 ++++++++++++++++++++++++++++++++++-----------
 fs/ksmbd/vfs.c     |   2 +-
 fs/ksmbd/vfs.h     |   2 +-
 3 files changed, 83 insertions(+), 26 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index dcf907738610..3476cacd2784 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -7044,24 +7044,26 @@ int smb2_lock(struct ksmbd_work *work)
 	return err;
 }
 
-static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req,
+static int fsctl_copychunk(struct ksmbd_work *work,
+			   struct copychunk_ioctl_req *ci_req,
+			   unsigned int cnt_code,
+			   unsigned int input_count,
+			   unsigned long long volatile_id,
+			   unsigned long long persistent_id,
 			   struct smb2_ioctl_rsp *rsp)
 {
-	struct copychunk_ioctl_req *ci_req;
 	struct copychunk_ioctl_rsp *ci_rsp;
 	struct ksmbd_file *src_fp = NULL, *dst_fp = NULL;
 	struct srv_copychunk *chunks;
 	unsigned int i, chunk_count, chunk_count_written = 0;
 	unsigned int chunk_size_written = 0;
 	loff_t total_size_written = 0;
-	int ret, cnt_code;
+	int ret = 0;
 
-	cnt_code = le32_to_cpu(req->CntCode);
-	ci_req = (struct copychunk_ioctl_req *)&req->Buffer[0];
 	ci_rsp = (struct copychunk_ioctl_rsp *)&rsp->Buffer[0];
 
-	rsp->VolatileFileId = req->VolatileFileId;
-	rsp->PersistentFileId = req->PersistentFileId;
+	rsp->VolatileFileId = cpu_to_le64(volatile_id);
+	rsp->PersistentFileId = cpu_to_le64(persistent_id);
 	ci_rsp->ChunksWritten =
 		cpu_to_le32(ksmbd_server_side_copy_max_chunk_count());
 	ci_rsp->ChunkBytesWritten =
@@ -7071,12 +7073,13 @@ static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req,
 
 	chunks = (struct srv_copychunk *)&ci_req->Chunks[0];
 	chunk_count = le32_to_cpu(ci_req->ChunkCount);
+	if (chunk_count == 0)
+		goto out;
 	total_size_written = 0;
 
 	/* verify the SRV_COPYCHUNK_COPY packet */
 	if (chunk_count > ksmbd_server_side_copy_max_chunk_count() ||
-	    le32_to_cpu(req->InputCount) <
-	     offsetof(struct copychunk_ioctl_req, Chunks) +
+	    input_count < offsetof(struct copychunk_ioctl_req, Chunks) +
 	     chunk_count * sizeof(struct srv_copychunk)) {
 		rsp->hdr.Status = STATUS_INVALID_PARAMETER;
 		return -EINVAL;
@@ -7097,9 +7100,7 @@ static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req,
 
 	src_fp = ksmbd_lookup_foreign_fd(work,
 					 le64_to_cpu(ci_req->ResumeKey[0]));
-	dst_fp = ksmbd_lookup_fd_slow(work,
-				      le64_to_cpu(req->VolatileFileId),
-				      le64_to_cpu(req->PersistentFileId));
+	dst_fp = ksmbd_lookup_fd_slow(work, volatile_id, persistent_id);
 	ret = -EINVAL;
 	if (!src_fp ||
 	    src_fp->persistent_id != le64_to_cpu(ci_req->ResumeKey[1])) {
@@ -7174,11 +7175,11 @@ static __be32 idev_ipv4_address(struct in_device *idev)
 }
 
 static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn,
-					struct smb2_ioctl_req *req,
-					struct smb2_ioctl_rsp *rsp)
+					struct smb2_ioctl_rsp *rsp,
+					unsigned int out_buf_len)
 {
 	struct network_interface_info_ioctl_rsp *nii_rsp = NULL;
-	int nbytes = 0;
+	unsigned int nbytes = 0;
 	struct net_device *netdev;
 	struct sockaddr_storage_rsp *sockaddr_storage;
 	unsigned int flags;
@@ -7187,6 +7188,10 @@ static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn,
 
 	rtnl_lock();
 	for_each_netdev(&init_net, netdev) {
+		if (out_buf_len <
+		    nbytes + sizeof(struct network_interface_info_ioctl_rsp))
+			break;
+
 		if (netdev->type == ARPHRD_LOOPBACK)
 			continue;
 
@@ -7258,6 +7263,8 @@ static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn,
 			sockaddr_storage->addr6.ScopeId = 0;
 		}
 
+		if (out_buf_len - nbytes < sizeof(struct network_interface_info_ioctl_rsp))
+			break;
 		nbytes += sizeof(struct network_interface_info_ioctl_rsp);
 	}
 	rtnl_unlock();
@@ -7278,11 +7285,16 @@ static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn,
 
 static int fsctl_validate_negotiate_info(struct ksmbd_conn *conn,
 					 struct validate_negotiate_info_req *neg_req,
-					 struct validate_negotiate_info_rsp *neg_rsp)
+					 struct validate_negotiate_info_rsp *neg_rsp,
+					 unsigned int in_buf_len)
 {
 	int ret = 0;
 	int dialect;
 
+	if (in_buf_len < sizeof(struct validate_negotiate_info_req) +
+			le16_to_cpu(neg_req->DialectCount) * sizeof(__le16))
+		return -EINVAL;
+
 	dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects,
 					     neg_req->DialectCount);
 	if (dialect == BAD_PROT_ID || dialect != conn->dialect) {
@@ -7316,7 +7328,7 @@ static int fsctl_validate_negotiate_info(struct ksmbd_conn *conn,
 static int fsctl_query_allocated_ranges(struct ksmbd_work *work, u64 id,
 					struct file_allocated_range_buffer *qar_req,
 					struct file_allocated_range_buffer *qar_rsp,
-					int in_count, int *out_count)
+					unsigned int in_count, unsigned int *out_count)
 {
 	struct ksmbd_file *fp;
 	loff_t start, length;
@@ -7343,7 +7355,8 @@ static int fsctl_query_allocated_ranges(struct ksmbd_work *work, u64 id,
 }
 
 static int fsctl_pipe_transceive(struct ksmbd_work *work, u64 id,
-				 int out_buf_len, struct smb2_ioctl_req *req,
+				 unsigned int out_buf_len,
+				 struct smb2_ioctl_req *req,
 				 struct smb2_ioctl_rsp *rsp)
 {
 	struct ksmbd_rpc_command *rpc_resp;
@@ -7457,8 +7470,7 @@ int smb2_ioctl(struct ksmbd_work *work)
 {
 	struct smb2_ioctl_req *req;
 	struct smb2_ioctl_rsp *rsp, *rsp_org;
-	int cnt_code, nbytes = 0;
-	int out_buf_len;
+	unsigned int cnt_code, nbytes = 0, out_buf_len, in_buf_len;
 	u64 id = KSMBD_NO_FID;
 	struct ksmbd_conn *conn = work->conn;
 	int ret = 0;
@@ -7487,7 +7499,11 @@ int smb2_ioctl(struct ksmbd_work *work)
 
 	cnt_code = le32_to_cpu(req->CntCode);
 	out_buf_len = le32_to_cpu(req->MaxOutputResponse);
-	out_buf_len = min(KSMBD_IPC_MAX_PAYLOAD, out_buf_len);
+	out_buf_len =
+		min_t(u32, work->response_sz - work->next_smb2_rsp_hdr_off -
+				(offsetof(struct smb2_ioctl_rsp, Buffer) - 4),
+		      out_buf_len);
+	in_buf_len = le32_to_cpu(req->InputCount);
 
 	switch (cnt_code) {
 	case FSCTL_DFS_GET_REFERRALS:
@@ -7515,6 +7531,7 @@ int smb2_ioctl(struct ksmbd_work *work)
 		break;
 	}
 	case FSCTL_PIPE_TRANSCEIVE:
+		out_buf_len = min_t(u32, KSMBD_IPC_MAX_PAYLOAD, out_buf_len);
 		nbytes = fsctl_pipe_transceive(work, id, out_buf_len, req, rsp);
 		break;
 	case FSCTL_VALIDATE_NEGOTIATE_INFO:
@@ -7523,9 +7540,16 @@ int smb2_ioctl(struct ksmbd_work *work)
 			goto out;
 		}
 
+		if (in_buf_len < sizeof(struct validate_negotiate_info_req))
+			return -EINVAL;
+
+		if (out_buf_len < sizeof(struct validate_negotiate_info_rsp))
+			return -EINVAL;
+
 		ret = fsctl_validate_negotiate_info(conn,
 			(struct validate_negotiate_info_req *)&req->Buffer[0],
-			(struct validate_negotiate_info_rsp *)&rsp->Buffer[0]);
+			(struct validate_negotiate_info_rsp *)&rsp->Buffer[0],
+			in_buf_len);
 		if (ret < 0)
 			goto out;
 
@@ -7534,7 +7558,7 @@ int smb2_ioctl(struct ksmbd_work *work)
 		rsp->VolatileFileId = cpu_to_le64(SMB2_NO_FID);
 		break;
 	case FSCTL_QUERY_NETWORK_INTERFACE_INFO:
-		nbytes = fsctl_query_iface_info_ioctl(conn, req, rsp);
+		nbytes = fsctl_query_iface_info_ioctl(conn, rsp, out_buf_len);
 		if (nbytes < 0)
 			goto out;
 		break;
@@ -7561,15 +7585,33 @@ int smb2_ioctl(struct ksmbd_work *work)
 			goto out;
 		}
 
+		if (in_buf_len < sizeof(struct copychunk_ioctl_req)) {
+			ret = -EINVAL;
+			goto out;
+		}
+
 		if (out_buf_len < sizeof(struct copychunk_ioctl_rsp)) {
 			ret = -EINVAL;
 			goto out;
 		}
 
 		nbytes = sizeof(struct copychunk_ioctl_rsp);
-		fsctl_copychunk(work, req, rsp);
+		rsp->VolatileFileId = req->VolatileFileId;
+		rsp->PersistentFileId = req->PersistentFileId;
+		fsctl_copychunk(work,
+				(struct copychunk_ioctl_req *)&req->Buffer[0],
+				le32_to_cpu(req->CntCode),
+				le32_to_cpu(req->InputCount),
+				le64_to_cpu(req->VolatileFileId),
+				le64_to_cpu(req->PersistentFileId),
+				rsp);
 		break;
 	case FSCTL_SET_SPARSE:
+		if (in_buf_len < sizeof(struct file_sparse)) {
+			ret = -EINVAL;
+			goto out;
+		}
+
 		ret = fsctl_set_sparse(work, id,
 				       (struct file_sparse *)&req->Buffer[0]);
 		if (ret < 0)
@@ -7588,6 +7630,11 @@ int smb2_ioctl(struct ksmbd_work *work)
 			goto out;
 		}
 
+		if (in_buf_len < sizeof(struct file_zero_data_information)) {
+			ret = -EINVAL;
+			goto out;
+		}
+
 		zero_data =
 			(struct file_zero_data_information *)&req->Buffer[0];
 
@@ -7607,6 +7654,11 @@ int smb2_ioctl(struct ksmbd_work *work)
 		break;
 	}
 	case FSCTL_QUERY_ALLOCATED_RANGES:
+		if (in_buf_len < sizeof(struct file_allocated_range_buffer)) {
+			ret = -EINVAL;
+			goto out;
+		}
+
 		ret = fsctl_query_allocated_ranges(work, id,
 			(struct file_allocated_range_buffer *)&req->Buffer[0],
 			(struct file_allocated_range_buffer *)&rsp->Buffer[0],
@@ -7647,6 +7699,11 @@ int smb2_ioctl(struct ksmbd_work *work)
 		struct duplicate_extents_to_file *dup_ext;
 		loff_t src_off, dst_off, length, cloned;
 
+		if (in_buf_len < sizeof(struct duplicate_extents_to_file)) {
+			ret = -EINVAL;
+			goto out;
+		}
+
 		dup_ext = (struct duplicate_extents_to_file *)&req->Buffer[0];
 
 		fp_in = ksmbd_lookup_fd_slow(work, dup_ext->VolatileFileHandle,
diff --git a/fs/ksmbd/vfs.c b/fs/ksmbd/vfs.c
index b41954294d38..835b384b0895 100644
--- a/fs/ksmbd/vfs.c
+++ b/fs/ksmbd/vfs.c
@@ -1023,7 +1023,7 @@ int ksmbd_vfs_zero_data(struct ksmbd_work *work, struct ksmbd_file *fp,
 
 int ksmbd_vfs_fqar_lseek(struct ksmbd_file *fp, loff_t start, loff_t length,
 			 struct file_allocated_range_buffer *ranges,
-			 int in_count, int *out_count)
+			 unsigned int in_count, unsigned int *out_count)
 {
 	struct file *f = fp->filp;
 	struct inode *inode = file_inode(fp->filp);
diff --git a/fs/ksmbd/vfs.h b/fs/ksmbd/vfs.h
index 7b1dcaa3fbdc..b0d5b8feb4a3 100644
--- a/fs/ksmbd/vfs.h
+++ b/fs/ksmbd/vfs.h
@@ -166,7 +166,7 @@ int ksmbd_vfs_zero_data(struct ksmbd_work *work, struct ksmbd_file *fp,
 struct file_allocated_range_buffer;
 int ksmbd_vfs_fqar_lseek(struct ksmbd_file *fp, loff_t start, loff_t length,
 			 struct file_allocated_range_buffer *ranges,
-			 int in_count, int *out_count);
+			 unsigned int in_count, unsigned int *out_count);
 int ksmbd_vfs_unlink(struct user_namespace *user_ns,
 		     struct dentry *dir, struct dentry *dentry);
 void *ksmbd_vfs_init_kstat(char **p, struct ksmbd_kstat *ksmbd_kstat);
-- 
2.31.1

[PATCH v6 03/14] ksmbd: check strictly data area in ksmbd_smb2_check_message()

[Ralph Boehme]

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

[PATCH v6 04/14] ksmbd: remove the leftover of smb2.0 dialect support

[Ralph Boehme]

From: Namjae Jeon <linkinjeon@kernel.org>

Although ksmbd doesn't send SMB2.0 support in supported dialect list of smb
negotiate response, There is the leftover of smb2.0 dialect.
This patch remove it not to support SMB2.0 in ksmbd.

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>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/smb2ops.c    |  5 -----
 fs/ksmbd/smb2pdu.c    | 34 +++++++++-------------------------
 fs/ksmbd/smb2pdu.h    |  1 -
 fs/ksmbd/smb_common.c |  6 +++---
 4 files changed, 12 insertions(+), 34 deletions(-)

diff --git a/fs/ksmbd/smb2ops.c b/fs/ksmbd/smb2ops.c
index 197473871aa4..b06456eb587b 100644
--- a/fs/ksmbd/smb2ops.c
+++ b/fs/ksmbd/smb2ops.c
@@ -187,11 +187,6 @@ static struct smb_version_cmds smb2_0_server_cmds[NUMBER_OF_SMB2_COMMANDS] = {
 	[SMB2_CHANGE_NOTIFY_HE]	=	{ .proc = smb2_notify},
 };
 
-int init_smb2_0_server(struct ksmbd_conn *conn)
-{
-	return -EOPNOTSUPP;
-}
-
 /**
  * init_smb2_1_server() - initialize a smb server connection with smb2.1
  *			command dispatcher
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 3476cacd2784..b06361313889 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -236,9 +236,6 @@ int init_smb2_neg_rsp(struct ksmbd_work *work)
 
 	if (conn->need_neg == false)
 		return -EINVAL;
-	if (!(conn->dialect >= SMB20_PROT_ID &&
-	      conn->dialect <= SMB311_PROT_ID))
-		return -EINVAL;
 
 	rsp_hdr = work->response_buf;
 
@@ -1166,13 +1163,6 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
 	case SMB21_PROT_ID:
 		init_smb2_1_server(conn);
 		break;
-	case SMB20_PROT_ID:
-		rc = init_smb2_0_server(conn);
-		if (rc) {
-			rsp->hdr.Status = STATUS_NOT_SUPPORTED;
-			goto err_out;
-		}
-		break;
 	case SMB2X_PROT_ID:
 	case BAD_PROT_ID:
 	default:
@@ -1191,11 +1181,9 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
 	rsp->MaxReadSize = cpu_to_le32(conn->vals->max_read_size);
 	rsp->MaxWriteSize = cpu_to_le32(conn->vals->max_write_size);
 
-	if (conn->dialect > SMB20_PROT_ID) {
-		memcpy(conn->ClientGUID, req->ClientGUID,
-		       SMB2_CLIENT_GUID_SIZE);
-		conn->cli_sec_mode = le16_to_cpu(req->SecurityMode);
-	}
+	memcpy(conn->ClientGUID, req->ClientGUID,
+			SMB2_CLIENT_GUID_SIZE);
+	conn->cli_sec_mode = le16_to_cpu(req->SecurityMode);
 
 	rsp->StructureSize = cpu_to_le16(65);
 	rsp->DialectRevision = cpu_to_le16(conn->dialect);
@@ -1537,11 +1525,9 @@ static int ntlm_authenticate(struct ksmbd_work *work)
 		}
 	}
 
-	if (conn->dialect > SMB20_PROT_ID) {
-		if (!ksmbd_conn_lookup_dialect(conn)) {
-			pr_err("fail to verify the dialect\n");
-			return -ENOENT;
-		}
+	if (!ksmbd_conn_lookup_dialect(conn)) {
+		pr_err("fail to verify the dialect\n");
+		return -ENOENT;
 	}
 	return 0;
 }
@@ -1623,11 +1609,9 @@ static int krb5_authenticate(struct ksmbd_work *work)
 		}
 	}
 
-	if (conn->dialect > SMB20_PROT_ID) {
-		if (!ksmbd_conn_lookup_dialect(conn)) {
-			pr_err("fail to verify the dialect\n");
-			return -ENOENT;
-		}
+	if (!ksmbd_conn_lookup_dialect(conn)) {
+		pr_err("fail to verify the dialect\n");
+		return -ENOENT;
 	}
 	return 0;
 }
diff --git a/fs/ksmbd/smb2pdu.h b/fs/ksmbd/smb2pdu.h
index 261825d06391..a6dec5ec6a54 100644
--- a/fs/ksmbd/smb2pdu.h
+++ b/fs/ksmbd/smb2pdu.h
@@ -1637,7 +1637,6 @@ struct smb2_posix_info {
 } __packed;
 
 /* functions */
-int init_smb2_0_server(struct ksmbd_conn *conn);
 void init_smb2_1_server(struct ksmbd_conn *conn);
 void init_smb3_0_server(struct ksmbd_conn *conn);
 void init_smb3_02_server(struct ksmbd_conn *conn);
diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index b6c4c7e960fa..707490ab1f4c 100644
--- a/fs/ksmbd/smb_common.c
+++ b/fs/ksmbd/smb_common.c
@@ -88,7 +88,7 @@ unsigned int ksmbd_server_side_copy_max_total_size(void)
 
 inline int ksmbd_min_protocol(void)
 {
-	return SMB2_PROT;
+	return SMB21_PROT;
 }
 
 inline int ksmbd_max_protocol(void)
@@ -427,7 +427,7 @@ int ksmbd_extract_shortname(struct ksmbd_conn *conn, const char *longname,
 
 static int __smb2_negotiate(struct ksmbd_conn *conn)
 {
-	return (conn->dialect >= SMB20_PROT_ID &&
+	return (conn->dialect >= SMB21_PROT_ID &&
 		conn->dialect <= SMB311_PROT_ID);
 }
 
@@ -457,7 +457,7 @@ int ksmbd_smb_negotiate_common(struct ksmbd_work *work, unsigned int command)
 		}
 	}
 
-	if (command == SMB2_NEGOTIATE_HE) {
+	if (command == SMB2_NEGOTIATE_HE && __smb2_negotiate(conn)) {
 		ret = smb2_handle_negotiate(work);
 		init_smb2_neg_rsp(work);
 		return ret;
-- 
2.31.1

[PATCH v6 05/14] 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 v6 06/14] ksmbd: check buffer is big enough to access the ProtocolId field

[Ralph Boehme]

Signed-off-by: Ralph Boehme <slow@samba.org>
---
 fs/ksmbd/smb2misc.c   | 25 +++++++++++++++++++++++++
 fs/ksmbd/smb2pdu.h    |  1 +
 fs/ksmbd/smb_common.c |  8 ++++++++
 3 files changed, 34 insertions(+)

diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
index 9edd9c161b27..c1f0f10ca9f9 100644
--- a/fs/ksmbd/smb2misc.c
+++ b/fs/ksmbd/smb2misc.c
@@ -432,3 +432,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);
diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index e1e5a071678e..0dc70ed2a5be 100644
--- a/fs/ksmbd/smb_common.c
+++ b/fs/ksmbd/smb_common.c
@@ -133,8 +133,16 @@ int ksmbd_lookup_protocol_idx(char *str)
 int ksmbd_verify_smb_message(struct ksmbd_work *work)
 {
 	struct smb2_hdr *smb2_hdr = ksmbd_req_buf_next(work);
+	unsigned int buflen = ksmbd_smb2_cur_pdu_buflen(work);
 	struct smb_hdr *hdr;
 
+	/*
+	 * ksmbd_smb2_check_message() will verify all SMB2 PDU buffer sizes,
+	 * here we just check we can access the ProtocolId field in the header.
+	 */
+	if (buflen < sizeof(smb2_hdr->ProtocolId))
+		return -EINVAL;
+
 	if (smb2_hdr->ProtocolId == SMB2_PROTO_NUMBER)
 		return ksmbd_smb2_check_message(work);
 
-- 
2.31.1

[PATCH v6 07/14] 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 c1f0f10ca9f9..76f53db7db8d 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 v6 08/14] ksmbd: 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 | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
index 76f53db7db8d..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;
 
 		/*
-- 
2.31.1

[PATCH v6 09/14] ksmbd: check PDU len is at least header plus body size in ksmbd_smb2_check_message()

[Ralph Boehme]

Note: we already have the same check in is_chained_smb2_message(), but there it
only applies to compound requests, so we have to repeat the check here to cover
both cases.

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..541b39b7a84b 100644
--- a/fs/ksmbd/smb2misc.c
+++ b/fs/ksmbd/smb2misc.c
@@ -338,6 +338,9 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
 	if (check_smb2_hdr(hdr))
 		return 1;
 
+	if (len < sizeof(struct smb2_pdu) - 4)
+		return 1;
+
 	if (hdr->StructureSize != SMB2_HEADER_STRUCTURE_SIZE) {
 		ksmbd_debug(SMB, "Illegal structure size %u\n",
 			    le16_to_cpu(hdr->StructureSize));
-- 
2.31.1

[PATCH v6 10/14] 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 b06361313889..7d3344b5519c 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 v6 11/14] 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 7d3344b5519c..8cbce9a9c2e0 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 v6 12/14] 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 8cbce9a9c2e0..2f71905503b5 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 v6 13/14] ksmdb: move session and tcon validation to ksmbd_smb2_check_message()

[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

[PATCH v6 14/14] 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 541b39b7a84b..6e6d64b796c9 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

Re: [PATCH v6 07/14] ksmbd: use ksmbd_req_buf_next() in ksmbd_smb2_check_message()

[Namjae Jeon]

2021-10-02 22:12 GMT+09:00, Ralph Boehme <slow@samba.org>:
> No change in behaviour.
It is better to add patch subject here if there is nothing to write
patch description.
i.e. "Use ksmbd_req_buf_next() in ksmbd_smb2_check_message()"

>
> 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 c1f0f10ca9f9..76f53db7db8d 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
>
>

Re: [PATCH v6 09/14] ksmbd: check PDU len is at least header plus body size in ksmbd_smb2_check_message()

[Namjae Jeon]

2021-10-02 22:12 GMT+09:00, Ralph Boehme <slow@samba.org>:
> Note: we already have the same check in is_chained_smb2_message(), but there
> it
> only applies to compound requests, so we have to repeat the check here to
> cover
> both cases.
>
> 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..541b39b7a84b 100644
> --- a/fs/ksmbd/smb2misc.c
> +++ b/fs/ksmbd/smb2misc.c
> @@ -338,6 +338,9 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
>  	if (check_smb2_hdr(hdr))
>  		return 1;
>
> +	if (len < sizeof(struct smb2_pdu) - 4)
> +		return 1;
The point I'm talking about is an issue caused by you moving the
header buffer size here first( and you remove header buffer size check
in ksmbd_conn_handler_loop().)
When I apply patch till 09/14 number, check_user_session() and
get_ksmbd_tcon() is still has problem that access header without
header buffer size check.

And there still are functions to access header without header buffer
size check. Please check allocate_rsp_buf(), is_transform_hdr() and
init_rsp_hdr() in __handle_ksmbd_work().

> +
>  	if (hdr->StructureSize != SMB2_HEADER_STRUCTURE_SIZE) {
>  		ksmbd_debug(SMB, "Illegal structure size %u\n",
>  			    le16_to_cpu(hdr->StructureSize));
> --
> 2.31.1
>
>

Re: [PATCH v6 01/14] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value

[Namjae Jeon]

2021-10-02 22:11 GMT+09:00, Ralph Boehme <slow@samba.org>:
> From: Namjae Jeon <linkinjeon@kernel.org>
>
> This patch add MAX_STREAM_PROT_LEN macro and check if stream protocol
> length exceeds maximum value. opencode pdu size check in
> ksmbd_pdu_size_has_room().
>
> 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>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>  fs/ksmbd/connection.c | 9 +++++----
>  fs/ksmbd/smb_common.c | 6 ------
>  fs/ksmbd/smb_common.h | 4 ++--
>  3 files changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c
> index af086d35398a..e50353c50661 100644
> --- a/fs/ksmbd/connection.c
> +++ b/fs/ksmbd/connection.c
> @@ -296,10 +296,11 @@ int ksmbd_conn_handler_loop(void *p)
>  		pdu_size = get_rfc1002_len(hdr_buf);
>  		ksmbd_debug(CONN, "RFC1002 header %u bytes\n", pdu_size);
>
> -		/* make sure we have enough to get to SMB header end */
> -		if (!ksmbd_pdu_size_has_room(pdu_size)) {
> -			ksmbd_debug(CONN, "SMB request too short (%u bytes)\n",
> -				    pdu_size);
> +		/*
> +		 * Check if pdu size is valid (min : smb header size,
> +		 * max : 0x00FFFFFF).
> +		 */
> +		if (pdu_size > MAX_STREAM_PROT_LEN) {
You changed this patch not to check header size check. I think that
this patch should be backed to original one. Because your patches are
clean-up patches, not fixes.
So you need to remove header size check here in "ksmbd: check PDU len
is at least header plus body size... ".

>  			continue;
>  		}
>
> diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
> index db8042a173d0..b6c4c7e960fa 100644
> --- a/fs/ksmbd/smb_common.c
> +++ b/fs/ksmbd/smb_common.c
> @@ -21,7 +21,6 @@ static const char basechars[43] =
> "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_-!@#$%";
>  #define MAGIC_CHAR '~'
>  #define PERIOD '.'
>  #define mangle(V) ((char)(basechars[(V) % MANGLE_BASE]))
> -#define KSMBD_MIN_SUPPORTED_HEADER_SIZE	(sizeof(struct smb2_hdr))
>
>  struct smb_protocol {
>  	int		index;
> @@ -294,11 +293,6 @@ int ksmbd_init_smb_server(struct ksmbd_work *work)
>  	return 0;
>  }
>
> -bool ksmbd_pdu_size_has_room(unsigned int pdu)
> -{
> -	return (pdu >= KSMBD_MIN_SUPPORTED_HEADER_SIZE - 4);
> -}
> -
>  int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work, int
> info_level,
>  				      struct ksmbd_file *dir,
>  				      struct ksmbd_dir_info *d_info,
> diff --git a/fs/ksmbd/smb_common.h b/fs/ksmbd/smb_common.h
> index 994abede27e9..6e79e7577f6b 100644
> --- a/fs/ksmbd/smb_common.h
> +++ b/fs/ksmbd/smb_common.h
> @@ -48,6 +48,8 @@
>  #define CIFS_DEFAULT_IOSIZE	(64 * 1024)
>  #define MAX_CIFS_SMALL_BUFFER_SIZE 448 /* big enough for most */
>
> +#define MAX_STREAM_PROT_LEN	0x00FFFFFF
> +
>  /* Responses when opening a file. */
>  #define F_SUPERSEDED	0
>  #define F_OPENED	1
> @@ -493,8 +495,6 @@ int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects,
> __le16 dialects_count);
>
>  int ksmbd_init_smb_server(struct ksmbd_work *work);
>
> -bool ksmbd_pdu_size_has_room(unsigned int pdu);
> -
>  struct ksmbd_kstat;
>  int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work,
>  				      int info_level,
> --
> 2.31.1
>
>

Re: [PATCH v6 02/14] ksmbd: add validation in smb2_ioctl

[Namjae Jeon]

2021-10-02 22:12 GMT+09:00, Ralph Boehme <slow@samba.org>:
> From: Namjae Jeon <linkinjeon@kernel.org>
>
> Add validation for request/response buffer size check in smb2_ioctl and
> fsctl_copychunk() take copychunk_ioctl_req pointer and the other arguments
> instead of smb2_ioctl_req structure and remove an unused smb2_ioctl_req
> argument of fsctl_validate_negotiate_info.
>
> 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>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
If you don't find any issue in this patch, please mark your reviewed-by tag.
Other patches also..

Thanks!

> ---
>  fs/ksmbd/smb2pdu.c | 105 ++++++++++++++++++++++++++++++++++-----------
>  fs/ksmbd/vfs.c     |   2 +-
>  fs/ksmbd/vfs.h     |   2 +-
>  3 files changed, 83 insertions(+), 26 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index dcf907738610..3476cacd2784 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -7044,24 +7044,26 @@ int smb2_lock(struct ksmbd_work *work)
>  	return err;
>  }
>
> -static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req
> *req,
> +static int fsctl_copychunk(struct ksmbd_work *work,
> +			   struct copychunk_ioctl_req *ci_req,
> +			   unsigned int cnt_code,
> +			   unsigned int input_count,
> +			   unsigned long long volatile_id,
> +			   unsigned long long persistent_id,
>  			   struct smb2_ioctl_rsp *rsp)
>  {
> -	struct copychunk_ioctl_req *ci_req;
>  	struct copychunk_ioctl_rsp *ci_rsp;
>  	struct ksmbd_file *src_fp = NULL, *dst_fp = NULL;
>  	struct srv_copychunk *chunks;
>  	unsigned int i, chunk_count, chunk_count_written = 0;
>  	unsigned int chunk_size_written = 0;
>  	loff_t total_size_written = 0;
> -	int ret, cnt_code;
> +	int ret = 0;
>
> -	cnt_code = le32_to_cpu(req->CntCode);
> -	ci_req = (struct copychunk_ioctl_req *)&req->Buffer[0];
>  	ci_rsp = (struct copychunk_ioctl_rsp *)&rsp->Buffer[0];
>
> -	rsp->VolatileFileId = req->VolatileFileId;
> -	rsp->PersistentFileId = req->PersistentFileId;
> +	rsp->VolatileFileId = cpu_to_le64(volatile_id);
> +	rsp->PersistentFileId = cpu_to_le64(persistent_id);
>  	ci_rsp->ChunksWritten =
>  		cpu_to_le32(ksmbd_server_side_copy_max_chunk_count());
>  	ci_rsp->ChunkBytesWritten =
> @@ -7071,12 +7073,13 @@ static int fsctl_copychunk(struct ksmbd_work *work,
> struct smb2_ioctl_req *req,
>
>  	chunks = (struct srv_copychunk *)&ci_req->Chunks[0];
>  	chunk_count = le32_to_cpu(ci_req->ChunkCount);
> +	if (chunk_count == 0)
> +		goto out;
>  	total_size_written = 0;
>
>  	/* verify the SRV_COPYCHUNK_COPY packet */
>  	if (chunk_count > ksmbd_server_side_copy_max_chunk_count() ||
> -	    le32_to_cpu(req->InputCount) <
> -	     offsetof(struct copychunk_ioctl_req, Chunks) +
> +	    input_count < offsetof(struct copychunk_ioctl_req, Chunks) +
>  	     chunk_count * sizeof(struct srv_copychunk)) {
>  		rsp->hdr.Status = STATUS_INVALID_PARAMETER;
>  		return -EINVAL;
> @@ -7097,9 +7100,7 @@ static int fsctl_copychunk(struct ksmbd_work *work,
> struct smb2_ioctl_req *req,
>
>  	src_fp = ksmbd_lookup_foreign_fd(work,
>  					 le64_to_cpu(ci_req->ResumeKey[0]));
> -	dst_fp = ksmbd_lookup_fd_slow(work,
> -				      le64_to_cpu(req->VolatileFileId),
> -				      le64_to_cpu(req->PersistentFileId));
> +	dst_fp = ksmbd_lookup_fd_slow(work, volatile_id, persistent_id);
>  	ret = -EINVAL;
>  	if (!src_fp ||
>  	    src_fp->persistent_id != le64_to_cpu(ci_req->ResumeKey[1])) {
> @@ -7174,11 +7175,11 @@ static __be32 idev_ipv4_address(struct in_device
> *idev)
>  }
>
>  static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn,
> -					struct smb2_ioctl_req *req,
> -					struct smb2_ioctl_rsp *rsp)
> +					struct smb2_ioctl_rsp *rsp,
> +					unsigned int out_buf_len)
>  {
>  	struct network_interface_info_ioctl_rsp *nii_rsp = NULL;
> -	int nbytes = 0;
> +	unsigned int nbytes = 0;
>  	struct net_device *netdev;
>  	struct sockaddr_storage_rsp *sockaddr_storage;
>  	unsigned int flags;
> @@ -7187,6 +7188,10 @@ static int fsctl_query_iface_info_ioctl(struct
> ksmbd_conn *conn,
>
>  	rtnl_lock();
>  	for_each_netdev(&init_net, netdev) {
> +		if (out_buf_len <
> +		    nbytes + sizeof(struct network_interface_info_ioctl_rsp))
> +			break;
> +
>  		if (netdev->type == ARPHRD_LOOPBACK)
>  			continue;
>
> @@ -7258,6 +7263,8 @@ static int fsctl_query_iface_info_ioctl(struct
> ksmbd_conn *conn,
>  			sockaddr_storage->addr6.ScopeId = 0;
>  		}
>
> +		if (out_buf_len - nbytes < sizeof(struct
> network_interface_info_ioctl_rsp))
> +			break;
>  		nbytes += sizeof(struct network_interface_info_ioctl_rsp);
>  	}
>  	rtnl_unlock();
> @@ -7278,11 +7285,16 @@ static int fsctl_query_iface_info_ioctl(struct
> ksmbd_conn *conn,
>
>  static int fsctl_validate_negotiate_info(struct ksmbd_conn *conn,
>  					 struct validate_negotiate_info_req *neg_req,
> -					 struct validate_negotiate_info_rsp *neg_rsp)
> +					 struct validate_negotiate_info_rsp *neg_rsp,
> +					 unsigned int in_buf_len)
>  {
>  	int ret = 0;
>  	int dialect;
>
> +	if (in_buf_len < sizeof(struct validate_negotiate_info_req) +
> +			le16_to_cpu(neg_req->DialectCount) * sizeof(__le16))
> +		return -EINVAL;
> +
>  	dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects,
>  					     neg_req->DialectCount);
>  	if (dialect == BAD_PROT_ID || dialect != conn->dialect) {
> @@ -7316,7 +7328,7 @@ static int fsctl_validate_negotiate_info(struct
> ksmbd_conn *conn,
>  static int fsctl_query_allocated_ranges(struct ksmbd_work *work, u64 id,
>  					struct file_allocated_range_buffer *qar_req,
>  					struct file_allocated_range_buffer *qar_rsp,
> -					int in_count, int *out_count)
> +					unsigned int in_count, unsigned int *out_count)
>  {
>  	struct ksmbd_file *fp;
>  	loff_t start, length;
> @@ -7343,7 +7355,8 @@ static int fsctl_query_allocated_ranges(struct
> ksmbd_work *work, u64 id,
>  }
>
>  static int fsctl_pipe_transceive(struct ksmbd_work *work, u64 id,
> -				 int out_buf_len, struct smb2_ioctl_req *req,
> +				 unsigned int out_buf_len,
> +				 struct smb2_ioctl_req *req,
>  				 struct smb2_ioctl_rsp *rsp)
>  {
>  	struct ksmbd_rpc_command *rpc_resp;
> @@ -7457,8 +7470,7 @@ int smb2_ioctl(struct ksmbd_work *work)
>  {
>  	struct smb2_ioctl_req *req;
>  	struct smb2_ioctl_rsp *rsp, *rsp_org;
> -	int cnt_code, nbytes = 0;
> -	int out_buf_len;
> +	unsigned int cnt_code, nbytes = 0, out_buf_len, in_buf_len;
>  	u64 id = KSMBD_NO_FID;
>  	struct ksmbd_conn *conn = work->conn;
>  	int ret = 0;
> @@ -7487,7 +7499,11 @@ int smb2_ioctl(struct ksmbd_work *work)
>
>  	cnt_code = le32_to_cpu(req->CntCode);
>  	out_buf_len = le32_to_cpu(req->MaxOutputResponse);
> -	out_buf_len = min(KSMBD_IPC_MAX_PAYLOAD, out_buf_len);
> +	out_buf_len =
> +		min_t(u32, work->response_sz - work->next_smb2_rsp_hdr_off -
> +				(offsetof(struct smb2_ioctl_rsp, Buffer) - 4),
> +		      out_buf_len);
> +	in_buf_len = le32_to_cpu(req->InputCount);
>
>  	switch (cnt_code) {
>  	case FSCTL_DFS_GET_REFERRALS:
> @@ -7515,6 +7531,7 @@ int smb2_ioctl(struct ksmbd_work *work)
>  		break;
>  	}
>  	case FSCTL_PIPE_TRANSCEIVE:
> +		out_buf_len = min_t(u32, KSMBD_IPC_MAX_PAYLOAD, out_buf_len);
>  		nbytes = fsctl_pipe_transceive(work, id, out_buf_len, req, rsp);
>  		break;
>  	case FSCTL_VALIDATE_NEGOTIATE_INFO:
> @@ -7523,9 +7540,16 @@ int smb2_ioctl(struct ksmbd_work *work)
>  			goto out;
>  		}
>
> +		if (in_buf_len < sizeof(struct validate_negotiate_info_req))
> +			return -EINVAL;
> +
> +		if (out_buf_len < sizeof(struct validate_negotiate_info_rsp))
> +			return -EINVAL;
> +
>  		ret = fsctl_validate_negotiate_info(conn,
>  			(struct validate_negotiate_info_req *)&req->Buffer[0],
> -			(struct validate_negotiate_info_rsp *)&rsp->Buffer[0]);
> +			(struct validate_negotiate_info_rsp *)&rsp->Buffer[0],
> +			in_buf_len);
>  		if (ret < 0)
>  			goto out;
>
> @@ -7534,7 +7558,7 @@ int smb2_ioctl(struct ksmbd_work *work)
>  		rsp->VolatileFileId = cpu_to_le64(SMB2_NO_FID);
>  		break;
>  	case FSCTL_QUERY_NETWORK_INTERFACE_INFO:
> -		nbytes = fsctl_query_iface_info_ioctl(conn, req, rsp);
> +		nbytes = fsctl_query_iface_info_ioctl(conn, rsp, out_buf_len);
>  		if (nbytes < 0)
>  			goto out;
>  		break;
> @@ -7561,15 +7585,33 @@ int smb2_ioctl(struct ksmbd_work *work)
>  			goto out;
>  		}
>
> +		if (in_buf_len < sizeof(struct copychunk_ioctl_req)) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
>  		if (out_buf_len < sizeof(struct copychunk_ioctl_rsp)) {
>  			ret = -EINVAL;
>  			goto out;
>  		}
>
>  		nbytes = sizeof(struct copychunk_ioctl_rsp);
> -		fsctl_copychunk(work, req, rsp);
> +		rsp->VolatileFileId = req->VolatileFileId;
> +		rsp->PersistentFileId = req->PersistentFileId;
> +		fsctl_copychunk(work,
> +				(struct copychunk_ioctl_req *)&req->Buffer[0],
> +				le32_to_cpu(req->CntCode),
> +				le32_to_cpu(req->InputCount),
> +				le64_to_cpu(req->VolatileFileId),
> +				le64_to_cpu(req->PersistentFileId),
> +				rsp);
>  		break;
>  	case FSCTL_SET_SPARSE:
> +		if (in_buf_len < sizeof(struct file_sparse)) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
>  		ret = fsctl_set_sparse(work, id,
>  				       (struct file_sparse *)&req->Buffer[0]);
>  		if (ret < 0)
> @@ -7588,6 +7630,11 @@ int smb2_ioctl(struct ksmbd_work *work)
>  			goto out;
>  		}
>
> +		if (in_buf_len < sizeof(struct file_zero_data_information)) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
>  		zero_data =
>  			(struct file_zero_data_information *)&req->Buffer[0];
>
> @@ -7607,6 +7654,11 @@ int smb2_ioctl(struct ksmbd_work *work)
>  		break;
>  	}
>  	case FSCTL_QUERY_ALLOCATED_RANGES:
> +		if (in_buf_len < sizeof(struct file_allocated_range_buffer)) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
>  		ret = fsctl_query_allocated_ranges(work, id,
>  			(struct file_allocated_range_buffer *)&req->Buffer[0],
>  			(struct file_allocated_range_buffer *)&rsp->Buffer[0],
> @@ -7647,6 +7699,11 @@ int smb2_ioctl(struct ksmbd_work *work)
>  		struct duplicate_extents_to_file *dup_ext;
>  		loff_t src_off, dst_off, length, cloned;
>
> +		if (in_buf_len < sizeof(struct duplicate_extents_to_file)) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
>  		dup_ext = (struct duplicate_extents_to_file *)&req->Buffer[0];
>
>  		fp_in = ksmbd_lookup_fd_slow(work, dup_ext->VolatileFileHandle,
> diff --git a/fs/ksmbd/vfs.c b/fs/ksmbd/vfs.c
> index b41954294d38..835b384b0895 100644
> --- a/fs/ksmbd/vfs.c
> +++ b/fs/ksmbd/vfs.c
> @@ -1023,7 +1023,7 @@ int ksmbd_vfs_zero_data(struct ksmbd_work *work,
> struct ksmbd_file *fp,
>
>  int ksmbd_vfs_fqar_lseek(struct ksmbd_file *fp, loff_t start, loff_t
> length,
>  			 struct file_allocated_range_buffer *ranges,
> -			 int in_count, int *out_count)
> +			 unsigned int in_count, unsigned int *out_count)
>  {
>  	struct file *f = fp->filp;
>  	struct inode *inode = file_inode(fp->filp);
> diff --git a/fs/ksmbd/vfs.h b/fs/ksmbd/vfs.h
> index 7b1dcaa3fbdc..b0d5b8feb4a3 100644
> --- a/fs/ksmbd/vfs.h
> +++ b/fs/ksmbd/vfs.h
> @@ -166,7 +166,7 @@ int ksmbd_vfs_zero_data(struct ksmbd_work *work, struct
> ksmbd_file *fp,
>  struct file_allocated_range_buffer;
>  int ksmbd_vfs_fqar_lseek(struct ksmbd_file *fp, loff_t start, loff_t
> length,
>  			 struct file_allocated_range_buffer *ranges,
> -			 int in_count, int *out_count);
> +			 unsigned int in_count, unsigned int *out_count);
>  int ksmbd_vfs_unlink(struct user_namespace *user_ns,
>  		     struct dentry *dir, struct dentry *dentry);
>  void *ksmbd_vfs_init_kstat(char **p, struct ksmbd_kstat *ksmbd_kstat);
> --
> 2.31.1
>
>

Re: [PATCH v6 01/14] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value

[Ralph Boehme]

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

Am 03.10.21 um 03:18 schrieb Namjae Jeon:
> 2021-10-02 22:11 GMT+09:00, Ralph Boehme <slow@samba.org>:
>> From: Namjae Jeon <linkinjeon@kernel.org>
>>
>> This patch add MAX_STREAM_PROT_LEN macro and check if stream protocol
>> length exceeds maximum value. opencode pdu size check in
>> ksmbd_pdu_size_has_room().
>>
>> 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>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>   fs/ksmbd/connection.c | 9 +++++----
>>   fs/ksmbd/smb_common.c | 6 ------
>>   fs/ksmbd/smb_common.h | 4 ++--
>>   3 files changed, 7 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c
>> index af086d35398a..e50353c50661 100644
>> --- a/fs/ksmbd/connection.c
>> +++ b/fs/ksmbd/connection.c
>> @@ -296,10 +296,11 @@ int ksmbd_conn_handler_loop(void *p)
>>   		pdu_size = get_rfc1002_len(hdr_buf);
>>   		ksmbd_debug(CONN, "RFC1002 header %u bytes\n", pdu_size);
>>
>> -		/* make sure we have enough to get to SMB header end */
>> -		if (!ksmbd_pdu_size_has_room(pdu_size)) {
>> -			ksmbd_debug(CONN, "SMB request too short (%u bytes)\n",
>> -				    pdu_size);
>> +		/*
>> +		 * Check if pdu size is valid (min : smb header size,
>> +		 * max : 0x00FFFFFF).
>> +		 */
>> +		if (pdu_size > MAX_STREAM_PROT_LEN) {
> You changed this patch not to check header size check.

yes, on purpose. Because I though that your change was wrong and the 
whole logic should be done differently. Unfortunately I missed the fact 
that there are a bunch of other places where header data is accessed 
early, before reaching ksmbd_smb2_check_message() where I wanted to 
consolidate the checks.

> I think that
> this patch should be backed to original one.
> Because your patches are
> clean-up patches, not fixes.

there are two more invalid reads that the patchset address.

But agreed, I will now simply work my patches on-top of yours.

Thanks!
-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 v6 07/14] ksmbd: use ksmbd_req_buf_next() in ksmbd_smb2_check_message()

[Ralph Boehme]

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

Am 03.10.21 um 02:59 schrieb Namjae Jeon:
> 2021-10-02 22:12 GMT+09:00, Ralph Boehme <slow@samba.org>:
>> No change in behaviour.
> It is better to add patch subject here if there is nothing to write
> patch description.
> i.e. "Use ksmbd_req_buf_next() in ksmbd_smb2_check_message()"
See the mail subject, that's the patch subject.

In Samba we often help reviewers reviewing large patchsets that may 
include a lot of small preparational/cleanup patches with a sentence 
like "No change in behaviour" to give a hint which patches are critical 
and need extra care when reviewing (ie all others) and which may not 
require this.

-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 v6 09/14] ksmbd: check PDU len is at least header plus body size in ksmbd_smb2_check_message()

[Ralph Boehme]

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

Am 03.10.21 um 03:11 schrieb Namjae Jeon:
> 2021-10-02 22:12 GMT+09:00, Ralph Boehme <slow@samba.org>:
>> Note: we already have the same check in is_chained_smb2_message(), but there
>> it
>> only applies to compound requests, so we have to repeat the check here to
>> cover
>> both cases.
>>
>> 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..541b39b7a84b 100644
>> --- a/fs/ksmbd/smb2misc.c
>> +++ b/fs/ksmbd/smb2misc.c
>> @@ -338,6 +338,9 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
>>   	if (check_smb2_hdr(hdr))
>>   		return 1;
>>
>> +	if (len < sizeof(struct smb2_pdu) - 4)
>> +		return 1;
> The point I'm talking about is an issue caused by you moving the
> header buffer size here first( and you remove header buffer size check
> in ksmbd_conn_handler_loop().)
> When I apply patch till 09/14 number, check_user_session() and
> get_ksmbd_tcon() is still has problem that access header without
> header buffer size check.
> 
> And there still are functions to access header without header buffer
> size check. Please check allocate_rsp_buf(), is_transform_hdr() and
> init_rsp_hdr() in __handle_ksmbd_work().

oh, that's a problem, missed that, sorry. Looks like we have to keep the 
validation at this layer. :(

-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 v6 07/14] ksmbd: use ksmbd_req_buf_next() in ksmbd_smb2_check_message()

[Steve French]

Typically kernel style encourages even a brief description in all
changesets (even trivial ones) e.g.

"Simplifies message parsing slightly.  No change in behavior"

On Mon, Oct 4, 2021 at 11:29 PM Ralph Boehme <slow@samba.org> wrote:
>
> Am 03.10.21 um 02:59 schrieb Namjae Jeon:
> > 2021-10-02 22:12 GMT+09:00, Ralph Boehme <slow@samba.org>:
> >> No change in behaviour.
> > It is better to add patch subject here if there is nothing to write
> > patch description.
> > i.e. "Use ksmbd_req_buf_next() in ksmbd_smb2_check_message()"
> See the mail subject, that's the patch subject.
>
> In Samba we often help reviewers reviewing large patchsets that may
> include a lot of small preparational/cleanup patches with a sentence
> like "No change in behaviour" to give a hint which patches are critical
> and need extra care when reviewing (ie all others) and which may not
> require this.
>
> -slow
>
> --
> Ralph Boehme, Samba Team                 https://samba.org/
> SerNet Samba Team Lead      https://sernet.de/en/team-samba



-- 
Thanks,

Steve

Re: [PATCH v6 07/14] ksmbd: use ksmbd_req_buf_next() in ksmbd_smb2_check_message()

[Ralph Boehme]

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

Am 05.10.21 um 20:43 schrieb Steve French:
> Typically kernel style encourages even a brief description in all
> changesets (even trivial ones) e.g.
> 
> "Simplifies message parsing slightly.  No change in behavior"

Sure, I could add this. Otoh

bcf130f9dfbaccf91376a44b18f51ed8007840d6

:)

To me it doesn't make sense.

Cheers!
-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 v6 07/14] ksmbd: use ksmbd_req_buf_next() in ksmbd_smb2_check_message()

[Steve French]

On Tue, Oct 5, 2021 at 2:28 PM Ralph Boehme <slow@samba.org> wrote:
>
> Am 05.10.21 um 20:43 schrieb Steve French:
> > Typically kernel style encourages even a brief description in all
> > changesets (even trivial ones) e.g.
> >
> > "Simplifies message parsing slightly.  No change in behavior"
>
> Sure, I could add this. Otoh
>
> bcf130f9dfbaccf91376a44b18f51ed8007840d6
>
> :)
>
> To me it doesn't make sense.

The patch submission guidelines for the kernel (see
Documentation/process/submitting-patches.rst) are not too bad to
understand (you can see why slightly more description is needed from
some examples mentioned there), and seem reasonably logical.  Also
checkpatch already auto-verifies a few of the things mentioned in the
submitting-patches guidelines.

Note that your example is an old patch (from 10 years ago); rules have
gotten a bit stricter.  Here is a more recent patch from the same
committer, note that he no longer uses the minimalist description ("No
change in behavior") see below (and another example from same
committer commit 4b03d99794eeed27650597a886247c6427ce1055)

commit ebd9d2c2f5a7ebaaed2d7bb4dee148755f46033d
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Fri Apr 16 14:00:17 2021 -0400

    nfsd: reshuffle some code

    No change in behavior, I'm just moving some code around to avoid forward
    references in a following patch.

    (To do someday: figure out how to split up nfs4state.c.  It's big and
    disorganized.)

    Signed-off-by: J. Bruce Fields <bfields@redhat.com>
    Signed-off-by: Chuck Lever <chuck.lever@oracle.com>


-- 
Thanks,

Steve