[PATCH v3 0/5] ksmbd: a bunch of patches

[PATCH v3 0/5] ksmbd: a bunch of patches

[Namjae Jeon]

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: Hyunchul Lee <hyc.lee@gmail.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>

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(). 

Namjae Jeon (5):
  ksmbd: add the check to vaildate if stream protocol length exceeds
    maximum value
  ksmbd: add validation in smb2_ioctl
  ksmbd: add request buffer validation in smb2_set_info
  ksmbd: check strictly data area in ksmbd_smb2_check_message()
  ksmbd: add validation in smb2 negotiate

 fs/ksmbd/connection.c |  10 +-
 fs/ksmbd/smb2misc.c   |  98 +++++++-------
 fs/ksmbd/smb2pdu.c    | 295 ++++++++++++++++++++++++++++++++----------
 fs/ksmbd/smb2pdu.h    |   9 ++
 fs/ksmbd/smb_common.c |  38 ++++--
 fs/ksmbd/smb_common.h |   4 +-
 fs/ksmbd/vfs.c        |   2 +-
 fs/ksmbd/vfs.h        |   2 +-
 8 files changed, 321 insertions(+), 137 deletions(-)

-- 
2.25.1

[PATCH v3 1/5] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value

[Namjae Jeon]

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 | 10 ++++++----
 fs/ksmbd/smb_common.c |  6 ------
 fs/ksmbd/smb_common.h |  4 ++--
 3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c
index af086d35398a..48b18b4ec117 100644
--- a/fs/ksmbd/connection.c
+++ b/fs/ksmbd/connection.c
@@ -296,10 +296,12 @@ 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 < __SMB2_HEADER_STRUCTURE_SIZE ||
+		    pdu_size > MAX_STREAM_PROT_LEN) {
 			continue;
 		}
 
diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index 1da67217698d..36fd9695fbc5 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;
@@ -267,11 +266,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 d7df19c97c4c..b8d225f7dbfc 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
@@ -492,8 +494,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.25.1

[PATCH v3 2/5] ksmbd: add validation in smb2_ioctl

[Namjae Jeon]

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 | 104 ++++++++++++++++++++++++++++++++++-----------
 fs/ksmbd/vfs.c     |   2 +-
 fs/ksmbd/vfs.h     |   2 +-
 3 files changed, 82 insertions(+), 26 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index ad6533cfc190..068f0f3827f9 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -6918,24 +6918,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 =
@@ -6945,12 +6947,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;
@@ -6971,9 +6974,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])) {
@@ -7048,11 +7049,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;
@@ -7061,6 +7062,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;
 
@@ -7132,6 +7137,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();
@@ -7152,11 +7159,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) {
@@ -7190,7 +7202,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;
@@ -7217,7 +7229,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;
@@ -7331,8 +7344,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;
@@ -7361,7 +7373,10 @@ 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, MAX_STREAM_PROT_LEN - 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:
@@ -7389,6 +7404,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:
@@ -7397,9 +7413,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;
 
@@ -7408,7 +7431,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;
@@ -7435,15 +7458,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)
@@ -7462,6 +7503,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];
 
@@ -7481,6 +7527,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],
@@ -7521,6 +7572,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.25.1

[PATCH v3 3/5] ksmbd: add request buffer validation in smb2_set_info

[Namjae Jeon]

Add buffer validation in smb2_set_info, and remove unused variable
in set_file_basic_info. and smb2_set_info infolevel functions take
structure pointer argument.

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 | 149 ++++++++++++++++++++++++++++++++-------------
 fs/ksmbd/smb2pdu.h |   9 +++
 2 files changed, 116 insertions(+), 42 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 068f0f3827f9..dca979d2ef52 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -2102,16 +2102,22 @@ static noinline int create_smb2_pipe(struct ksmbd_work *work)
  * smb2_set_ea() - handler for setting extended attributes using set
  *		info command
  * @eabuf:	set info command buffer
+ * @buf_len:	set info command buffer length
  * @path:	dentry path for get ea
  *
  * Return:	0 on success, otherwise error
  */
-static int smb2_set_ea(struct smb2_ea_info *eabuf, struct path *path)
+static int smb2_set_ea(struct smb2_ea_info *eabuf, unsigned int buf_len,
+		       struct path *path)
 {
 	struct user_namespace *user_ns = mnt_user_ns(path->mnt);
 	char *attr_name = NULL, *value;
 	int rc = 0;
-	int next = 0;
+	unsigned int next = 0;
+
+	if (buf_len < sizeof(struct smb2_ea_info) + eabuf->EaNameLength +
+			le16_to_cpu(eabuf->EaValueLength))
+		return -EINVAL;
 
 	attr_name = kmalloc(XATTR_NAME_MAX + 1, GFP_KERNEL);
 	if (!attr_name)
@@ -2176,7 +2182,13 @@ static int smb2_set_ea(struct smb2_ea_info *eabuf, struct path *path)
 
 next:
 		next = le32_to_cpu(eabuf->NextEntryOffset);
+		if (next == 0 || buf_len < next)
+			break;
+		buf_len -= next;
 		eabuf = (struct smb2_ea_info *)((char *)eabuf + next);
+		if (next < (u32)eabuf->EaNameLength + le16_to_cpu(eabuf->EaValueLength))
+			break;
+
 	} while (next != 0);
 
 	kfree(attr_name);
@@ -2757,7 +2769,15 @@ int smb2_open(struct ksmbd_work *work)
 		created = true;
 		user_ns = mnt_user_ns(path.mnt);
 		if (ea_buf) {
-			rc = smb2_set_ea(&ea_buf->ea, &path);
+			if (le32_to_cpu(ea_buf->ccontext.DataLength) <
+			    sizeof(struct smb2_ea_info)) {
+				rc = -EINVAL;
+				goto err_out;
+			}
+
+			rc = smb2_set_ea(&ea_buf->ea,
+					 le32_to_cpu(ea_buf->ccontext.DataLength),
+					 &path);
 			if (rc == -EOPNOTSUPP)
 				rc = 0;
 			else if (rc)
@@ -5342,7 +5362,7 @@ static int smb2_rename(struct ksmbd_work *work,
 static int smb2_create_link(struct ksmbd_work *work,
 			    struct ksmbd_share_config *share,
 			    struct smb2_file_link_info *file_info,
-			    struct file *filp,
+			    unsigned int buf_len, struct file *filp,
 			    struct nls_table *local_nls)
 {
 	char *link_name = NULL, *target_name = NULL, *pathname = NULL;
@@ -5350,6 +5370,10 @@ static int smb2_create_link(struct ksmbd_work *work,
 	bool file_present = true;
 	int rc;
 
+	if (buf_len < (u64)sizeof(struct smb2_file_link_info) +
+			le32_to_cpu(file_info->FileNameLength))
+		return -EINVAL;
+
 	ksmbd_debug(SMB, "setting FILE_LINK_INFORMATION\n");
 	pathname = kmalloc(PATH_MAX, GFP_KERNEL);
 	if (!pathname)
@@ -5409,10 +5433,10 @@ static int smb2_create_link(struct ksmbd_work *work,
 	return rc;
 }
 
-static int set_file_basic_info(struct ksmbd_file *fp, char *buf,
+static int set_file_basic_info(struct ksmbd_file *fp,
+			       struct smb2_file_basic_info *file_info,
 			       struct ksmbd_share_config *share)
 {
-	struct smb2_file_all_info *file_info;
 	struct iattr attrs;
 	struct timespec64 ctime;
 	struct file *filp;
@@ -5423,7 +5447,6 @@ static int set_file_basic_info(struct ksmbd_file *fp, char *buf,
 	if (!(fp->daccess & FILE_WRITE_ATTRIBUTES_LE))
 		return -EACCES;
 
-	file_info = (struct smb2_file_all_info *)buf;
 	attrs.ia_valid = 0;
 	filp = fp->filp;
 	inode = file_inode(filp);
@@ -5500,7 +5523,8 @@ static int set_file_basic_info(struct ksmbd_file *fp, char *buf,
 }
 
 static int set_file_allocation_info(struct ksmbd_work *work,
-				    struct ksmbd_file *fp, char *buf)
+				    struct ksmbd_file *fp,
+				    struct smb2_file_alloc_info *file_alloc_info)
 {
 	/*
 	 * TODO : It's working fine only when store dos attributes
@@ -5508,7 +5532,6 @@ static int set_file_allocation_info(struct ksmbd_work *work,
 	 * properly with any smb.conf option
 	 */
 
-	struct smb2_file_alloc_info *file_alloc_info;
 	loff_t alloc_blks;
 	struct inode *inode;
 	int rc;
@@ -5516,7 +5539,6 @@ static int set_file_allocation_info(struct ksmbd_work *work,
 	if (!(fp->daccess & FILE_WRITE_DATA_LE))
 		return -EACCES;
 
-	file_alloc_info = (struct smb2_file_alloc_info *)buf;
 	alloc_blks = (le64_to_cpu(file_alloc_info->AllocationSize) + 511) >> 9;
 	inode = file_inode(fp->filp);
 
@@ -5552,9 +5574,8 @@ static int set_file_allocation_info(struct ksmbd_work *work,
 }
 
 static int set_end_of_file_info(struct ksmbd_work *work, struct ksmbd_file *fp,
-				char *buf)
+				struct smb2_file_eof_info *file_eof_info)
 {
-	struct smb2_file_eof_info *file_eof_info;
 	loff_t newsize;
 	struct inode *inode;
 	int rc;
@@ -5562,7 +5583,6 @@ static int set_end_of_file_info(struct ksmbd_work *work, struct ksmbd_file *fp,
 	if (!(fp->daccess & FILE_WRITE_DATA_LE))
 		return -EACCES;
 
-	file_eof_info = (struct smb2_file_eof_info *)buf;
 	newsize = le64_to_cpu(file_eof_info->EndOfFile);
 	inode = file_inode(fp->filp);
 
@@ -5589,7 +5609,8 @@ static int set_end_of_file_info(struct ksmbd_work *work, struct ksmbd_file *fp,
 }
 
 static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
-			   char *buf)
+			   struct smb2_file_rename_info *rename_info,
+			   unsigned int buf_len)
 {
 	struct user_namespace *user_ns;
 	struct ksmbd_file *parent_fp;
@@ -5602,6 +5623,10 @@ static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
 		return -EACCES;
 	}
 
+	if (buf_len < (u64)sizeof(struct smb2_file_rename_info) +
+			le32_to_cpu(rename_info->FileNameLength))
+		return -EINVAL;
+
 	user_ns = file_mnt_user_ns(fp->filp);
 	if (ksmbd_stream_fd(fp))
 		goto next;
@@ -5624,14 +5649,13 @@ static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
 		}
 	}
 next:
-	return smb2_rename(work, fp, user_ns,
-			   (struct smb2_file_rename_info *)buf,
+	return smb2_rename(work, fp, user_ns, rename_info,
 			   work->sess->conn->local_nls);
 }
 
-static int set_file_disposition_info(struct ksmbd_file *fp, char *buf)
+static int set_file_disposition_info(struct ksmbd_file *fp,
+				     struct smb2_file_disposition_info *file_info)
 {
-	struct smb2_file_disposition_info *file_info;
 	struct inode *inode;
 
 	if (!(fp->daccess & FILE_DELETE_LE)) {
@@ -5640,7 +5664,6 @@ static int set_file_disposition_info(struct ksmbd_file *fp, char *buf)
 	}
 
 	inode = file_inode(fp->filp);
-	file_info = (struct smb2_file_disposition_info *)buf;
 	if (file_info->DeletePending) {
 		if (S_ISDIR(inode->i_mode) &&
 		    ksmbd_vfs_empty_dir(fp) == -ENOTEMPTY)
@@ -5652,15 +5675,14 @@ static int set_file_disposition_info(struct ksmbd_file *fp, char *buf)
 	return 0;
 }
 
-static int set_file_position_info(struct ksmbd_file *fp, char *buf)
+static int set_file_position_info(struct ksmbd_file *fp,
+				  struct smb2_file_pos_info *file_info)
 {
-	struct smb2_file_pos_info *file_info;
 	loff_t current_byte_offset;
 	unsigned long sector_size;
 	struct inode *inode;
 
 	inode = file_inode(fp->filp);
-	file_info = (struct smb2_file_pos_info *)buf;
 	current_byte_offset = le64_to_cpu(file_info->CurrentByteOffset);
 	sector_size = inode->i_sb->s_blocksize;
 
@@ -5676,12 +5698,11 @@ static int set_file_position_info(struct ksmbd_file *fp, char *buf)
 	return 0;
 }
 
-static int set_file_mode_info(struct ksmbd_file *fp, char *buf)
+static int set_file_mode_info(struct ksmbd_file *fp,
+			      struct smb2_file_mode_info *file_info)
 {
-	struct smb2_file_mode_info *file_info;
 	__le32 mode;
 
-	file_info = (struct smb2_file_mode_info *)buf;
 	mode = file_info->Mode;
 
 	if ((mode & ~FILE_MODE_INFO_MASK) ||
@@ -5711,40 +5732,74 @@ static int set_file_mode_info(struct ksmbd_file *fp, char *buf)
  * TODO: need to implement an error handling for STATUS_INFO_LENGTH_MISMATCH
  */
 static int smb2_set_info_file(struct ksmbd_work *work, struct ksmbd_file *fp,
-			      int info_class, char *buf,
+			      struct smb2_set_info_req *req,
 			      struct ksmbd_share_config *share)
 {
-	switch (info_class) {
+	unsigned int buf_len = le32_to_cpu(req->BufferLength);
+
+	switch (req->FileInfoClass) {
 	case FILE_BASIC_INFORMATION:
-		return set_file_basic_info(fp, buf, share);
+	{
+		if (buf_len < sizeof(struct smb2_file_basic_info))
+			return -EINVAL;
 
+		return set_file_basic_info(fp, (struct smb2_file_basic_info *)req->Buffer, share);
+	}
 	case FILE_ALLOCATION_INFORMATION:
-		return set_file_allocation_info(work, fp, buf);
+	{
+		if (buf_len < sizeof(struct smb2_file_alloc_info))
+			return -EINVAL;
 
+		return set_file_allocation_info(work, fp,
+						(struct smb2_file_alloc_info *)req->Buffer);
+	}
 	case FILE_END_OF_FILE_INFORMATION:
-		return set_end_of_file_info(work, fp, buf);
+	{
+		if (buf_len < sizeof(struct smb2_file_eof_info))
+			return -EINVAL;
 
+		return set_end_of_file_info(work, fp,
+					    (struct smb2_file_eof_info *)req->Buffer);
+	}
 	case FILE_RENAME_INFORMATION:
+	{
 		if (!test_tree_conn_flag(work->tcon, KSMBD_TREE_CONN_FLAG_WRITABLE)) {
 			ksmbd_debug(SMB,
 				    "User does not have write permission\n");
 			return -EACCES;
 		}
-		return set_rename_info(work, fp, buf);
 
+		if (buf_len < sizeof(struct smb2_file_rename_info))
+			return -EINVAL;
+
+		return set_rename_info(work, fp,
+				       (struct smb2_file_rename_info *)req->Buffer,
+				       buf_len);
+	}
 	case FILE_LINK_INFORMATION:
+	{
+		if (buf_len < sizeof(struct smb2_file_link_info))
+			return -EINVAL;
+
 		return smb2_create_link(work, work->tcon->share_conf,
-					(struct smb2_file_link_info *)buf, fp->filp,
+					(struct smb2_file_link_info *)req->Buffer,
+					buf_len, fp->filp,
 					work->sess->conn->local_nls);
-
+	}
 	case FILE_DISPOSITION_INFORMATION:
+	{
 		if (!test_tree_conn_flag(work->tcon, KSMBD_TREE_CONN_FLAG_WRITABLE)) {
 			ksmbd_debug(SMB,
 				    "User does not have write permission\n");
 			return -EACCES;
 		}
-		return set_file_disposition_info(fp, buf);
 
+		if (buf_len < sizeof(struct smb2_file_disposition_info))
+			return -EINVAL;
+
+		return set_file_disposition_info(fp,
+						 (struct smb2_file_disposition_info *)req->Buffer);
+	}
 	case FILE_FULL_EA_INFORMATION:
 	{
 		if (!(fp->daccess & FILE_WRITE_EA_LE)) {
@@ -5753,18 +5808,29 @@ static int smb2_set_info_file(struct ksmbd_work *work, struct ksmbd_file *fp,
 			return -EACCES;
 		}
 
-		return smb2_set_ea((struct smb2_ea_info *)buf,
-				   &fp->filp->f_path);
-	}
+		if (buf_len < sizeof(struct smb2_ea_info))
+			return -EINVAL;
 
+		return smb2_set_ea((struct smb2_ea_info *)req->Buffer,
+				   buf_len, &fp->filp->f_path);
+	}
 	case FILE_POSITION_INFORMATION:
-		return set_file_position_info(fp, buf);
+	{
+		if (buf_len < sizeof(struct smb2_file_pos_info))
+			return -EINVAL;
 
+		return set_file_position_info(fp, (struct smb2_file_pos_info *)req->Buffer);
+	}
 	case FILE_MODE_INFORMATION:
-		return set_file_mode_info(fp, buf);
+	{
+		if (buf_len < sizeof(struct smb2_file_mode_info))
+			return -EINVAL;
+
+		return set_file_mode_info(fp, (struct smb2_file_mode_info *)req->Buffer);
+	}
 	}
 
-	pr_err("Unimplemented Fileinfoclass :%d\n", info_class);
+	pr_err("Unimplemented Fileinfoclass :%d\n", req->FileInfoClass);
 	return -EOPNOTSUPP;
 }
 
@@ -5825,8 +5891,7 @@ int smb2_set_info(struct ksmbd_work *work)
 	switch (req->InfoType) {
 	case SMB2_O_INFO_FILE:
 		ksmbd_debug(SMB, "GOT SMB2_O_INFO_FILE\n");
-		rc = smb2_set_info_file(work, fp, req->FileInfoClass,
-					req->Buffer, work->tcon->share_conf);
+		rc = smb2_set_info_file(work, fp, req, work->tcon->share_conf);
 		break;
 	case SMB2_O_INFO_SECURITY:
 		ksmbd_debug(SMB, "GOT SMB2_O_INFO_SECURITY\n");
diff --git a/fs/ksmbd/smb2pdu.h b/fs/ksmbd/smb2pdu.h
index bcec845b03f3..261825d06391 100644
--- a/fs/ksmbd/smb2pdu.h
+++ b/fs/ksmbd/smb2pdu.h
@@ -1464,6 +1464,15 @@ struct smb2_file_all_info { /* data block encoding of response to level 18 */
 	char   FileName[1];
 } __packed; /* level 18 Query */
 
+struct smb2_file_basic_info { /* data block encoding of response to level 18 */
+	__le64 CreationTime;	/* Beginning of FILE_BASIC_INFO equivalent */
+	__le64 LastAccessTime;
+	__le64 LastWriteTime;
+	__le64 ChangeTime;
+	__le32 Attributes;
+	__u32  Pad1;		/* End of FILE_BASIC_INFO_INFO equivalent */
+} __packed;
+
 struct smb2_file_alt_name_info {
 	__le32 FileNameLength;
 	char FileName[0];
-- 
2.25.1

[PATCH v3 4/5] ksmbd: check strictly data area in ksmbd_smb2_check_message()

[Namjae Jeon]

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>
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.25.1

[PATCH v3 5/5] ksmbd: add validation in smb2 negotiate

[Namjae Jeon]

This patch add validation to check request buffer check in smb2
negotiate and fix null pointer deferencing oops in smb3_preauth_hash_rsp()
that found from manual test.

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: Hyunchul Lee <hyc.lee@gmail.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/smb2pdu.c    | 42 +++++++++++++++++++++++++++++++++++++++++-
 fs/ksmbd/smb_common.c | 32 +++++++++++++++++++++++++++-----
 2 files changed, 68 insertions(+), 6 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index dca979d2ef52..6e93ff0f164e 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -1067,6 +1067,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
 	struct smb2_negotiate_req *req = work->request_buf;
 	struct smb2_negotiate_rsp *rsp = work->response_buf;
 	int rc = 0;
+	unsigned int smb2_buf_len, smb2_neg_size;
 	__le32 status;
 
 	ksmbd_debug(SMB, "Received negotiate request\n");
@@ -1084,6 +1085,44 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
 		goto err_out;
 	}
 
+	smb2_buf_len = get_rfc1002_len(work->request_buf);
+	smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects) - 4;
+	if (conn->dialect == SMB311_PROT_ID) {
+		unsigned int nego_ctxt_off = le32_to_cpu(req->NegotiateContextOffset);
+
+		if (smb2_buf_len < nego_ctxt_off) {
+			rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+			rc = -EINVAL;
+			goto err_out;
+		}
+
+		if (smb2_neg_size > nego_ctxt_off) {
+			rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+			rc = -EINVAL;
+			goto err_out;
+		}
+
+		if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) >
+		    nego_ctxt_off) {
+			rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+			rc = -EINVAL;
+			goto err_out;
+		}
+	} else {
+		if (smb2_neg_size > smb2_buf_len) {
+			rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+			rc = -EINVAL;
+			goto err_out;
+		}
+
+		if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) >
+		    smb2_buf_len) {
+			rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+			rc = -EINVAL;
+			goto err_out;
+		}
+	}
+
 	conn->cli_cap = le32_to_cpu(req->Capabilities);
 	switch (conn->dialect) {
 	case SMB311_PROT_ID:
@@ -8301,7 +8340,8 @@ void smb3_preauth_hash_rsp(struct ksmbd_work *work)
 
 	WORK_BUFFERS(work, req, rsp);
 
-	if (le16_to_cpu(req->Command) == SMB2_NEGOTIATE_HE)
+	if (le16_to_cpu(req->Command) == SMB2_NEGOTIATE_HE &&
+	    conn->preauth_info)
 		ksmbd_gen_preauth_integrity_hash(conn, (char *)rsp,
 						 conn->preauth_info->Preauth_HashValue);
 
diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index 36fd9695fbc5..5d8e32041c15 100644
--- a/fs/ksmbd/smb_common.c
+++ b/fs/ksmbd/smb_common.c
@@ -162,10 +162,12 @@ static bool supported_protocol(int idx)
 		idx <= server_conf.max_protocol);
 }
 
-static char *next_dialect(char *dialect, int *next_off)
+static char *next_dialect(char *dialect, int *next_off, int bcount)
 {
 	dialect = dialect + *next_off;
-	*next_off = strlen(dialect);
+	*next_off = strnlen(dialect, bcount);
+	if (dialect[*next_off] != '\0')
+		return NULL;
 	return dialect;
 }
 
@@ -180,7 +182,9 @@ static int ksmbd_lookup_dialect_by_name(char *cli_dialects, __le16 byte_count)
 		dialect = cli_dialects;
 		bcount = le16_to_cpu(byte_count);
 		do {
-			dialect = next_dialect(dialect, &next);
+			dialect = next_dialect(dialect, &next, bcount);
+			if (!dialect)
+				break;
 			ksmbd_debug(SMB, "client requested dialect %s\n",
 				    dialect);
 			if (!strcmp(dialect, smb1_protos[i].name)) {
@@ -228,13 +232,22 @@ int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects, __le16 dialects_count)
 
 static int ksmbd_negotiate_smb_dialect(void *buf)
 {
-	__le32 proto;
+	int smb_buf_length = get_rfc1002_len(buf);
+	__le32 proto = ((struct smb2_hdr *)buf)->ProtocolId;
 
-	proto = ((struct smb2_hdr *)buf)->ProtocolId;
 	if (proto == SMB2_PROTO_NUMBER) {
 		struct smb2_negotiate_req *req;
+		int smb2_neg_size =
+			offsetof(struct smb2_negotiate_req, Dialects) - 4;
 
 		req = (struct smb2_negotiate_req *)buf;
+		if (smb2_neg_size > smb_buf_length)
+			goto err_out;
+
+		if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) >
+		    smb_buf_length)
+			goto err_out;
+
 		return ksmbd_lookup_dialect_by_id(req->Dialects,
 						  req->DialectCount);
 	}
@@ -244,10 +257,19 @@ static int ksmbd_negotiate_smb_dialect(void *buf)
 		struct smb_negotiate_req *req;
 
 		req = (struct smb_negotiate_req *)buf;
+		if (le16_to_cpu(req->ByteCount) < 2)
+			goto err_out;
+
+		if (offsetof(struct smb_negotiate_req, DialectsArray) - 4 +
+			le16_to_cpu(req->ByteCount) > smb_buf_length) {
+			goto err_out;
+		}
+
 		return ksmbd_lookup_dialect_by_name(req->DialectsArray,
 						    req->ByteCount);
 	}
 
+err_out:
 	return BAD_PROT_ID;
 }
 
-- 
2.25.1

Re: [PATCH v3 0/5] ksmbd: a bunch of patches

[Ralph Boehme]

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

Am 26.09.21 um 15:55 schrieb Namjae Jeon:
> 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: Hyunchul Lee <hyc.lee@gmail.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> 
> 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().

looks like you haven't pushed those to github yet? At least I don't see 
an update to ksmbd-for-next-pending.

I'd prefer fetching from git to review the patches. I also have a few 
patches on top.

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 v3 0/5] ksmbd: a bunch of patches

[Namjae Jeon]

2021-09-26 23:27 GMT+09:00, Ralph Boehme <slow@samba.org>:
> Am 26.09.21 um 15:55 schrieb Namjae Jeon:
>> 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: Hyunchul Lee <hyc.lee@gmail.com>
>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
>>
>> 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().
>
> looks like you haven't pushed those to github yet? At least I don't see
> an update to ksmbd-for-next-pending.
>
> I'd prefer fetching from git to review the patches. I also have a few
> patches on top.
Done, Please check it:)

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

Re: [PATCH v3 0/5] ksmbd: a bunch of patches

[Ralph Boehme]

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

Hi Namjae

Am 26.09.21 um 15:55 schrieb Namjae Jeon:
> 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: Hyunchul Lee <hyc.lee@gmail.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> 
> 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().

I think there are a few issues with this patchset. I'm working on fixes 
and improvements and will push my branch to my github clone once I'm 
ready. I guess it's going to take a few days.

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 v3 0/5] ksmbd: a bunch of patches

[Namjae Jeon]

2021-09-28 0:42 GMT+09:00, Ralph Boehme <slow@samba.org>:
> Hi Namjae
Hi Ralph,

>
> Am 26.09.21 um 15:55 schrieb Namjae Jeon:
>> 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: Hyunchul Lee <hyc.lee@gmail.com>
>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
>>
>> 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().
>
> I think there are a few issues with this patchset. I'm working on fixes
> and improvements and will push my branch to my github clone once I'm
> ready. I guess it's going to take a few days.
It sounds like you're making a patch based on this patch-set. If there
is missing something for buffer check, You can add a patch on top of
this, but if there are wrong codes in patch-set, It is right to leave
a review comment to update this patch-set.

Thanks!
>
> Cheers!
> -slow
>
> --
> Ralph Boehme, Samba Team                 https://samba.org/
> SerNet Samba Team Lead      https://sernet.de/en/team-samba
>

Re: [PATCH v3 0/5] ksmbd: a bunch of patches

[Ralph Boehme]

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

Am 28.09.21 um 01:57 schrieb Namjae Jeon:
> 2021-09-28 0:42 GMT+09:00, Ralph Boehme <slow@samba.org>:
>> Hi Namjae
> Hi Ralph,
> 
>>
>> Am 26.09.21 um 15:55 schrieb Namjae Jeon:
>>> 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: Hyunchul Lee <hyc.lee@gmail.com>
>>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
>>>
>>> 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().
>>
>> I think there are a few issues with this patchset. I'm working on fixes
>> and improvements and will push my branch to my github clone once I'm
>> ready. I guess it's going to take a few days.
> It sounds like you're making a patch based on this patch-set. If there
> is missing something for buffer check, You can add a patch on top of
> this, but if there are wrong codes in patch-set, It is right to leave
> a review comment to update this patch-set.

both: there are issues with the patch and I have changes on-top. :) It 
just takes a bit of time due to other stuff going on currently like SDC.

-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 v3 0/5] ksmbd: a bunch of patches

[Ralph Boehme]

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

Am 28.09.21 um 05:26 schrieb Ralph Boehme:
> both: there are issues with the patch and I have changes on-top. :) It 
> just takes a bit of time due to other stuff going on currently like SDC.

finally... :)

Please check my branch 
<https://github.com/slowfranklin/smb3-kernel/commits/ksmbd-for-next-pending>

for added commits and two SQUASHes. Remaining commits reviewed-by: me.

Oh, and I also split out the setinfo basic infolevel changes into its 
own commit.

Let me know what you think of the additional checks I've added.

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 v3 0/5] ksmbd: a bunch of patches

[Namjae Jeon]

2021-09-28 22:43 GMT+09:00, Ralph Boehme <slow@samba.org>:
> Am 28.09.21 um 05:26 schrieb Ralph Boehme:
>> both: there are issues with the patch and I have changes on-top. :) It
>> just takes a bit of time due to other stuff going on currently like SDC.
>
> finally... :)
>
> Please check my branch
> <https://github.com/slowfranklin/smb3-kernel/commits/ksmbd-for-next-pending>
>
> for added commits and two SQUASHes. Remaining commits reviewed-by: me.
Yep, looks good, I will update them in patches. And thanks for your review!
>
> Oh, and I also split out the setinfo basic infolevel changes into its
> own commit.
If you want to add clean-up patch first, we can change
get_file_basic_info() together in patch. I will update it also.
>
> Let me know what you think of the additional checks I've added.
You should submit patches to the list to be checked by other developers.

Thanks!
>
> Cheers!
> -slow
>
> --
> Ralph Boehme, Samba Team                 https://samba.org/
> SerNet Samba Team Lead      https://sernet.de/en/team-samba
>

Re: [PATCH v3 0/5] ksmbd: a bunch of patches

[Ralph Boehme]

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

Am 28.09.21 um 16:23 schrieb Namjae Jeon:
> 2021-09-28 22:43 GMT+09:00, Ralph Boehme <slow@samba.org>:
>> Am 28.09.21 um 05:26 schrieb Ralph Boehme:
>>> both: there are issues with the patch and I have changes on-top. :) It
>>> just takes a bit of time due to other stuff going on currently like SDC.
>>
>> finally... :)
>>
>> Please check my branch
>> <https://github.com/slowfranklin/smb3-kernel/commits/ksmbd-for-next-pending>
>>
>> for added commits and two SQUASHes. Remaining commits reviewed-by: me.
> Yep, looks good, I will update them in patches. And thanks for your review!

thanks!

>> Oh, and I also split out the setinfo basic infolevel changes into its
>> own commit.
> If you want to add clean-up patch first, we can change
> get_file_basic_info() together in patch. I will update it also.
>>
>> Let me know what you think of the additional checks I've added.
> You should submit patches to the list to be checked by other developers.

everyone can fetch from that branch. And as I'm not merely doing patch 
review, but am changing, expanding, fixing patches, an email patch 
workflow doesn't work.

Once the patchset has stabilized enough, it can go to the list.

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 v3 0/5] ksmbd: a bunch of patches

[Jeremy Allison]

On Tue, Sep 28, 2021 at 06:33:46PM +0200, Ralph Boehme wrote:
>Am 28.09.21 um 16:23 schrieb Namjae Jeon:
>>2021-09-28 22:43 GMT+09:00, Ralph Boehme <slow@samba.org>:
>>>Am 28.09.21 um 05:26 schrieb Ralph Boehme:
>>>>both: there are issues with the patch and I have changes on-top. :) It
>>>>just takes a bit of time due to other stuff going on currently like SDC.
>>>
>>>finally... :)
>>>
>>>Please check my branch
>>><https://github.com/slowfranklin/smb3-kernel/commits/ksmbd-for-next-pending>
>>>
>>>for added commits and two SQUASHes. Remaining commits reviewed-by: me.
>>Yep, looks good, I will update them in patches. And thanks for your review!
>
>thanks!
>
>>>Oh, and I also split out the setinfo basic infolevel changes into its
>>>own commit.
>>If you want to add clean-up patch first, we can change
>>get_file_basic_info() together in patch. I will update it also.
>>>
>>>Let me know what you think of the additional checks I've added.
>>You should submit patches to the list to be checked by other developers.
>
>everyone can fetch from that branch. And as I'm not merely doing patch 
>review, but am changing, expanding, fixing patches, an email patch 
>workflow doesn't work.

+1 on this. email-based patch workflow is fine when patches
don't go through many iterations or have many people working
on them, but when those things are true a repository-based
workflow is far better (IMHO). Everyone knows how to use
git (these days :-).

It would be good to get to the point where the list is
used as a "release management" tool where patches that
have already been completely reviewed and signed-off
are sent and archived.

Re: [PATCH v3 0/5] ksmbd: a bunch of patches

[Namjae Jeon]

2021-09-28 23:23 GMT+09:00, Namjae Jeon <linkinjeon@kernel.org>:
> 2021-09-28 22:43 GMT+09:00, Ralph Boehme <slow@samba.org>:
>> Am 28.09.21 um 05:26 schrieb Ralph Boehme:
>>> both: there are issues with the patch and I have changes on-top. :) It
>>> just takes a bit of time due to other stuff going on currently like SDC.
>>
>> finally... :)
>>
>> Please check my branch
>> <https://github.com/slowfranklin/smb3-kernel/commits/ksmbd-for-next-pending>
>>
>> for added commits and two SQUASHes. Remaining commits reviewed-by: me.
> Yep, looks good, I will update them in patches. And thanks for your review!
When I take a look, I found issues in two squashes. I leave comments...
I still prefer you give review comments on patches in the list.

>>
>> Oh, and I also split out the setinfo basic infolevel changes into its
>> own commit.
> If you want to add clean-up patch first, we can change
> get_file_basic_info() together in patch. I will update it also.
>>
>> Let me know what you think of the additional checks I've added.
> You should submit patches to the list to be checked by other developers.
>
> Thanks!
>>
>> Cheers!
>> -slow
>>
>> --
>> Ralph Boehme, Samba Team                 https://samba.org/
>> SerNet Samba Team Lead      https://sernet.de/en/team-samba
>>
>

Re: [PATCH v3 0/5] ksmbd: a bunch of patches

[Tom Talpey]

On 9/28/2021 1:33 PM, Jeremy Allison wrote:
> On Tue, Sep 28, 2021 at 06:33:46PM +0200, Ralph Boehme wrote:
>> Am 28.09.21 um 16:23 schrieb Namjae Jeon:
>>> 2021-09-28 22:43 GMT+09:00, Ralph Boehme <slow@samba.org>:
>>>> Am 28.09.21 um 05:26 schrieb Ralph Boehme:
>>>>> both: there are issues with the patch and I have changes on-top. :) It
>>>>> just takes a bit of time due to other stuff going on currently like 
>>>>> SDC.
>>>>
>>>> finally... :)
>>>>
>>>> Please check my branch
>>>> <https://github.com/slowfranklin/smb3-kernel/commits/ksmbd-for-next-pending> 
>>>>
>>>>
>>>> for added commits and two SQUASHes. Remaining commits reviewed-by: me.
>>> Yep, looks good, I will update them in patches. And thanks for your 
>>> review!
>>
>> thanks!
>>
>>>> Oh, and I also split out the setinfo basic infolevel changes into its
>>>> own commit.
>>> If you want to add clean-up patch first, we can change
>>> get_file_basic_info() together in patch. I will update it also.
>>>>
>>>> Let me know what you think of the additional checks I've added.
>>> You should submit patches to the list to be checked by other developers.
>>
>> everyone can fetch from that branch. And as I'm not merely doing patch 
>> review, but am changing, expanding, fixing patches, an email patch 
>> workflow doesn't work.
> 
> +1 on this. email-based patch workflow is fine when patches
> don't go through many iterations or have many people working
> on them, but when those things are true a repository-based
> workflow is far better (IMHO). Everyone knows how to use
> git (these days :-).

I completely agree that email is inefficient, but git is a terrible
way to have a discussion. We should attempt to be sure we have
those, and that everybody has a chance to see the proposals without
having to go to the web five times a day.

Please take this as a request for regular git-send-email updates to
this list, so I can see them if I'm not online. Maybe add a boilerplate
line to direct to the git repo webui. I'm sure a few others will
appreciate it too.

Tom.

> It would be good to get to the point where the list is
> used as a "release management" tool where patches that
> have already been completely reviewed and signed-off
> are sent and archived.
> 

Re: [PATCH v3 0/5] ksmbd: a bunch of patches

[Jeremy Allison]

On Wed, Sep 29, 2021 at 11:28:09AM -0400, Tom Talpey wrote:
>
>I completely agree that email is inefficient, but git is a terrible
>way to have a discussion. We should attempt to be sure we have
>those, and that everybody has a chance to see the proposals without
>having to go to the web five times a day.
>
>Please take this as a request for regular git-send-email updates to
>this list, so I can see them if I'm not online. Maybe add a boilerplate
>line to direct to the git repo webui. I'm sure a few others will
>appreciate it too.

Samba does well with the web-based discussion mechanism
around merge-requests (MR's) in gitlab. I assume github
has something similar.

Maybe send the initial patch to the list with a link
to the github MR so people interested in reviewing/discussing
can follow along there ?

Re: [PATCH v3 0/5] ksmbd: a bunch of patches

[Ralph Boehme]

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

Am 29.09.21 um 17:42 schrieb Jeremy Allison:
> On Wed, Sep 29, 2021 at 11:28:09AM -0400, Tom Talpey wrote:
>>
>> I completely agree that email is inefficient, but git is a terrible
>> way to have a discussion. We should attempt to be sure we have
>> those, and that everybody has a chance to see the proposals without
>> having to go to the web five times a day.
>>
>> Please take this as a request for regular git-send-email updates to
>> this list, so I can see them if I'm not online. Maybe add a boilerplate
>> line to direct to the git repo webui. I'm sure a few others will
>> appreciate it too.
> 
> Samba does well with the web-based discussion mechanism
> around merge-requests (MR's) in gitlab. I assume github
> has something similar.
> 
> Maybe send the initial patch to the list with a link
> to the github MR so people interested in reviewing/discussing
> can follow along there ?

well, if I could have it the way I wanted, then this would be it. But I 
understand that adopting new workflows is not something I can impose -- 
at least not without paying for an insane amount of Lakritz-Gitarren 
that I tend to use to bribe metze into doing something I want him to do. :)

The problem is not so much doing the *review* on patches sent to the 
list. While Samba has moved away from doing review on patch emails, it 
can certainly be done.

The point is, once you go beyond "review" by taking someone else's 
patchset, modifying it deeply, reordering patches, adding patches, 
rewriting patches, dropping patches and so on, that's when the 
patchset-as-email workflow explodes and coordination via git is needed.

Once such a collaboratively worked on patchset stabilizes, it can of 
course again go to the mailing list.

-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 v3 0/5] ksmbd: a bunch of patches

[Tom Talpey]

On 9/29/2021 12:38 PM, Ralph Boehme wrote:
> Am 29.09.21 um 17:42 schrieb Jeremy Allison:
>> On Wed, Sep 29, 2021 at 11:28:09AM -0400, Tom Talpey wrote:
>>>
>>> I completely agree that email is inefficient, but git is a terrible
>>> way to have a discussion. We should attempt to be sure we have
>>> those, and that everybody has a chance to see the proposals without
>>> having to go to the web five times a day.
>>>
>>> Please take this as a request for regular git-send-email updates to
>>> this list, so I can see them if I'm not online. Maybe add a boilerplate
>>> line to direct to the git repo webui. I'm sure a few others will
>>> appreciate it too.
>>
>> Samba does well with the web-based discussion mechanism
>> around merge-requests (MR's) in gitlab. I assume github
>> has something similar.
>>
>> Maybe send the initial patch to the list with a link
>> to the github MR so people interested in reviewing/discussing
>> can follow along there ?
> 
> well, if I could have it the way I wanted, then this would be it. But I 
> understand that adopting new workflows is not something I can impose -- 
> at least not without paying for an insane amount of Lakritz-Gitarren 
> that I tend to use to bribe metze into doing something I want him to do. :)

I'm in for github if you send me some too!

https://www.gutschmecker.com/produkt/haevy-metal-salzige-gitarren-10-x-150-g-tuete/


> The problem is not so much doing the *review* on patches sent to the 
> list. While Samba has moved away from doing review on patch emails, it 
> can certainly be done.

Clearly, this effort bridges the Linux and Samba processes. We can
definitely try. I guess I'm going to take some convincing.

Tom.

> The point is, once you go beyond "review" by taking someone else's 
> patchset, modifying it deeply, reordering patches, adding patches, 
> rewriting patches, dropping patches and so on, that's when the 
> patchset-as-email workflow explodes and coordination via git is needed.
> 
> Once such a collaboratively worked on patchset stabilizes, it can of 
> course again go to the mailing list.
> 
> -slow
> 

Re: [PATCH v3 0/5] ksmbd: a bunch of patches

[Ralph Boehme]

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

Am 29.09.21 um 18:45 schrieb Tom Talpey:
> On 9/29/2021 12:38 PM, Ralph Boehme wrote:
>> well, if I could have it the way I wanted, then this would be it. But 
>> I understand that adopting new workflows is not something I can impose 
>> -- at least not without paying for an insane amount of 
>> Lakritz-Gitarren that I tend to use to bribe metze into doing 
>> something I want him to do. :)
> 
> I'm in for github if you send me some too!
> 
> https://www.gutschmecker.com/produkt/haevy-metal-salzige-gitarren-10-x-150-g-tuete/ 

rofl

I have to bookmark this, looks like one of those would buy me at least a 
100-reviews-granted package from metze. :)))

-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 v3 0/5] ksmbd: a bunch of patches

[Steve French]

On Wed, Sep 29, 2021 at 11:45 AM Tom Talpey <tom@talpey.com> wrote:
>
> On 9/29/2021 12:38 PM, Ralph Boehme wrote:
> > Am 29.09.21 um 17:42 schrieb Jeremy Allison:
> >> On Wed, Sep 29, 2021 at 11:28:09AM -0400, Tom Talpey wrote:
> >>>
> >>> I completely agree that email is inefficient, but git is a terrible
> >>> way to have a discussion.

Agreed

> >> Maybe send the initial patch to the list with a link
> >> to the github MR so people interested in reviewing/discussing
> >> can follow along there ?
> >
> > well, if I could have it the way I wanted, then this would be it. But I
> > understand that adopting new workflows is not something I can impose --
> > at least not without paying for an insane amount of Lakritz-Gitarren
> > that I tend to use to bribe metze into doing something I want him to do. :)
>
> I'm in for github if you send me some too!

gitnub is fine for many things, and we can automated "kernel
development process"
things presumably with github easier than alternatives:
- running "scripts/checkpatch"
- make with C=1 and "_CHECK_ENDIAN" support turned on
- kick off smbtorture tests (as Namjae already does in his branches in github)

BUT ... we have to ensure a couple things.
- we don't annoy Linus (and linux-next and stable maintainers) by doing things
like web merges in github (he complained about the meaningless/distracting
github web ui empty merge messages)
- we don't miss reviewers in Linux who also want to see them on
mailing lists (presumably
some of fsdevel - ie more general patches that other fs developers outside
smb world need to comment on)


> >
> > Once such a collaboratively worked on patchset stabilizes, it can of
> > course again go to the mailing list.

Makes sense


-- 
Thanks,

Steve

Re: [PATCH v3 0/5] ksmbd: a bunch of patches

[Ralph Boehme]

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

Am 29.09.21 um 19:11 schrieb Steve French:
> gitnub is fine for many things, and we can automated "kernel
> development process"
> things presumably with github easier than alternatives:
> - running "scripts/checkpatch"
> - make with C=1 and "_CHECK_ENDIAN" support turned on
> - kick off smbtorture tests (as Namjae already does in his branches in github)

you can also add "doing review". :)

As for running tests: I want that as well! :) How can I get that? Maybe 
other want to run CI on their patches too before posting them.

> BUT ... we have to ensure a couple things.
> - we don't annoy Linus (and linux-next and stable maintainers) by doing things
> like web merges in github (he complained about the meaningless/distracting
> github web ui empty merge messages)

as said before: just don't do the merge there, just the review. That's 
the way Samba has been doing it for years. Are you actually aware of the 
current Samba workflow?

-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 v3 0/5] ksmbd: a bunch of patches

[Namjae Jeon]

2021-09-30 2:18 GMT+09:00, Ralph Boehme <slow@samba.org>:
> Am 29.09.21 um 19:11 schrieb Steve French:
>> gitnub is fine for many things, and we can automated "kernel
>> development process"
>> things presumably with github easier than alternatives:
>> - running "scripts/checkpatch"
>> - make with C=1 and "_CHECK_ENDIAN" support turned on
>> - kick off smbtorture tests (as Namjae already does in his branches in
>> github)
>
> you can also add "doing review". :)
>
> As for running tests: I want that as well! :) How can I get that? Maybe
> other want to run CI on their patches too before posting them.
>
>> BUT ... we have to ensure a couple things.
>> - we don't annoy Linus (and linux-next and stable maintainers) by doing
>> things
>> like web merges in github (he complained about the
>> meaningless/distracting
>> github web ui empty merge messages)
>
> as said before: just don't do the merge there, just the review. That's
> the way Samba has been doing it for years. Are you actually aware of the
> current Samba workflow?
Is it friendly to new developers? I know samba workflow now too. New
developers can do everything easily by simply subscribing to the
mailing list. And do we review only the SMB protocol on github? If we
review and discuss kernel common code usage and touching, it should be
visible to the component kernel maintainers as well.

And is the review history likely to be discarded on github? Doesn't it
get thrown away the moment you change or update a patch? Also, review
discussions left on each individual's github cannot be easily searched
like mailing list.
>
> -slow
>
> --
> Ralph Boehme, Samba Team                 https://samba.org/
> SerNet Samba Team Lead      https://sernet.de/en/team-samba
>

Re: [PATCH v3 0/5] ksmbd: a bunch of patches

[Hyunchul Lee]

2021년 9월 30일 (목) 오전 9:32, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
> 2021-09-30 2:18 GMT+09:00, Ralph Boehme <slow@samba.org>:
> > Am 29.09.21 um 19:11 schrieb Steve French:
> >> gitnub is fine for many things, and we can automated "kernel
> >> development process"
> >> things presumably with github easier than alternatives:
> >> - running "scripts/checkpatch"
> >> - make with C=1 and "_CHECK_ENDIAN" support turned on
> >> - kick off smbtorture tests (as Namjae already does in his branches in
> >> github)
> >
> > you can also add "doing review". :)
> >
> > As for running tests: I want that as well! :) How can I get that? Maybe
> > other want to run CI on their patches too before posting them.
> >
> >> BUT ... we have to ensure a couple things.
> >> - we don't annoy Linus (and linux-next and stable maintainers) by doing
> >> things
> >> like web merges in github (he complained about the
> >> meaningless/distracting
> >> github web ui empty merge messages)
> >
> > as said before: just don't do the merge there, just the review. That's
> > the way Samba has been doing it for years. Are you actually aware of the
> > current Samba workflow?
> Is it friendly to new developers? I know samba workflow now too. New
> developers can do everything easily by simply subscribing to the
> mailing list. And do we review only the SMB protocol on github? If we
> review and discuss kernel common code usage and touching, it should be
> visible to the component kernel maintainers as well.
>

I agreed. Kernel developers are familiar with reviews in the mailing list.
And only for patches about the SMB protocol, If someone asks for
review in github,
we can do it.

> And is the review history likely to be discarded on github? Doesn't it
> get thrown away the moment you change or update a patch? Also, review
> discussions left on each individual's github cannot be easily searched
> like mailing list.
> >
> > -slow
> >
> > --
> > Ralph Boehme, Samba Team                 https://samba.org/
> > SerNet Samba Team Lead      https://sernet.de/en/team-samba
> >



-- 
Thanks,
Hyunchul

Re: [PATCH v3 0/5] ksmbd: a bunch of patches

[Ralph Boehme]

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

Am 30.09.21 um 02:32 schrieb Namjae Jeon:
> 2021-09-30 2:18 GMT+09:00, Ralph Boehme <slow@samba.org>:
>> as said before: just don't do the merge there, just the review. That's
>> the way Samba has been doing it for years. Are you actually aware of the
>> current Samba workflow?
> Is it friendly to new developers?

yes. It's just a different tooling you have to wrap your head around. In 
the case of Samba it has the added benefit, that every interested 
contributor can make use of the full Samba CI. This works by using a 
shared repo on gitlab where every registered developer (so you have to 
ask to be added, but this is freely granted) where you push your 
patchset to a branch name prefixed with your username (to avoid stepping 
on someone else's branch). That triggers an automated *full* CI run, so 
new contributors can be sure not to waste time of other developers 
before asking for review.

> I know samba workflow now too. New
> developers can do everything easily by simply subscribing to the
> mailing list. 

Sure, instead of pushing the resulting patchset from the review of your 
patchset to a github branch, I could send my patchset to the ML.

Having now used a more a different workflow for a few years, I 
appreciate how much more natural it feels to share, work on and review 
code with a tooling that is much more intergrated to git.

So what I'd like to propose for now is let's stick to the ML for now, 
next time I will send my patchset to the ML instead, but let's also 
consider and maybe test different possible toolings.

> And do we review only the SMB protocol on github? If we
> review and discuss kernel common code usage and touching, it should be
> visible to the component kernel maintainers as well.
> 
> And is the review history likely to be discarded on github?

I don't think so, I'm not 100% sure on this though. But as it's NOT 
discarded on gitlab, I guess github will match feature wise.

> Doesn't it
> get thrown away the moment you change or update a patch? Also, review
> discussions left on each individual's github cannot be easily searched
> like mailing list.

Yeah, but does that really matter? So far this was not missed in the 
Samba gitlab tooling.

-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 --]