linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info
@ 2021-09-19  2:13 Namjae Jeon
  2021-09-19  2:13 ` [PATCH] ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup Namjae Jeon
                   ` (6 more replies)
  0 siblings, 7 replies; 22+ messages in thread
From: Namjae Jeon @ 2021-09-19  2:13 UTC (permalink / raw)
  To: linux-cifs; +Cc: Namjae Jeon, Ronnie Sahlberg, Ralph Böhme, Steve French

Add buffer validation in smb2_set_info.

Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/smb2pdu.c | 113 +++++++++++++++++++++++++++++++++++----------
 fs/ksmbd/smb2pdu.h |   9 ++++
 2 files changed, 97 insertions(+), 25 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 46e0275a77a8..7763f69e1ae8 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -2107,17 +2107,23 @@ 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;
 
+	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)
 		return -ENOMEM;
@@ -2181,7 +2187,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 < eabuf->EaNameLength + le16_to_cpu(eabuf->EaValueLength))
+			break;
+
 	} while (next != 0);
 
 	kfree(attr_name);
@@ -2790,7 +2802,9 @@ 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);
+			rc = smb2_set_ea(&ea_buf->ea,
+					 le32_to_cpu(ea_buf->ccontext.DataLength),
+					 &path);
 			if (rc == -EOPNOTSUPP)
 				rc = 0;
 			else if (rc)
@@ -5375,7 +5389,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,
+			    int buf_len, struct file *filp,
 			    struct nls_table *local_nls)
 {
 	char *link_name = NULL, *target_name = NULL, *pathname = NULL;
@@ -5383,6 +5397,10 @@ static int smb2_create_link(struct ksmbd_work *work,
 	bool file_present = true;
 	int rc;
 
+	if (buf_len < 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)
@@ -5442,7 +5460,7 @@ static int smb2_create_link(struct ksmbd_work *work,
 static int set_file_basic_info(struct ksmbd_file *fp, char *buf,
 			       struct ksmbd_share_config *share)
 {
-	struct smb2_file_all_info *file_info;
+	struct smb2_file_basic_info *file_info;
 	struct iattr attrs;
 	struct timespec64 ctime;
 	struct file *filp;
@@ -5453,7 +5471,7 @@ 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;
+	file_info = (struct smb2_file_basic_info *)buf;
 	attrs.ia_valid = 0;
 	filp = fp->filp;
 	inode = file_inode(filp);
@@ -5619,7 +5637,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,
+			   int buf_len)
 {
 	struct user_namespace *user_ns;
 	struct ksmbd_file *parent_fp;
@@ -5632,6 +5651,10 @@ static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
 		return -EACCES;
 	}
 
+	if (buf_len < 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;
@@ -5654,8 +5677,7 @@ 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);
 }
 
@@ -5741,40 +5763,71 @@ 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) {
+	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, 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, 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, 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, req->Buffer);
+	}
 	case FILE_FULL_EA_INFORMATION:
 	{
 		if (!(fp->daccess & FILE_WRITE_EA_LE)) {
@@ -5783,18 +5836,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, 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, req->Buffer);
+	}
 	}
 
-	pr_err("Unimplemented Fileinfoclass :%d\n", info_class);
+	pr_err("Unimplemented Fileinfoclass :%d\n", req->FileInfoClass);
 	return -EOPNOTSUPP;
 }
 
@@ -5855,8 +5919,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


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH] ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup
  2021-09-19  2:13 [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
@ 2021-09-19  2:13 ` Namjae Jeon
  2021-09-19  2:13 ` [PATCH v2 2/4] ksmbd: add validation in smb2_ioctl Namjae Jeon
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 22+ messages in thread
From: Namjae Jeon @ 2021-09-19  2:13 UTC (permalink / raw)
  To: linux-cifs; +Cc: Namjae Jeon, Ronnie Sahlberg, Ralph Böhme, Steve French

Use  LOOKUP_NO_SYMLINKS flags for default lookup to prohibit the middle of
symlink component lookup.

Test result:
# smbclient -Ulinkinjeon%1234 //172.30.1.42/share -c
"get hacked/passwd passwd"
NT_STATUS_OBJECT_NAME_NOT_FOUND opening remote file \hacked\passwd

Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/smb2pdu.c | 35 ++++++++++++++++++++++++-----------
 fs/ksmbd/vfs.c     | 34 +++++++++++++++++++++++++---------
 2 files changed, 49 insertions(+), 20 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 592d489277e8..afc508c2656d 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -2661,11 +2661,17 @@ int smb2_open(struct ksmbd_work *work)
 	}
 
 	if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) {
-		/*
-		 * On delete request, instead of following up, need to
-		 * look the current entity
-		 */
-		rc = ksmbd_vfs_kern_path(name, 0, &path, 1);
+		if (test_share_config_flag(work->tcon->share_conf,
+					   KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) {
+			/*
+			 * On delete request, instead of following up, need to
+			 * look the current entity
+			 */
+			rc = ksmbd_vfs_kern_path(name, 0, &path, 1);
+		} else {
+			rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1);
+		}
+
 		if (!rc) {
 			/*
 			 * If file exists with under flags, return access
@@ -2698,7 +2704,8 @@ int smb2_open(struct ksmbd_work *work)
 				rc = ksmbd_vfs_kern_path(name, 0, &path, 1);
 			}
 		} else {
-			rc = ksmbd_vfs_kern_path(name, 0, &path, 1);
+			rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS,
+						 &path, 1);
 			if (!rc && d_is_symlink(path.dentry)) {
 				rc = -EACCES;
 				path_put(&path);
@@ -4788,7 +4795,7 @@ static int smb2_get_info_filesystem(struct ksmbd_work *work,
 	struct path path;
 	int rc = 0, len;
 	int fs_infoclass_size = 0;
-	int lookup_flags = 0;
+	int lookup_flags = LOOKUP_NO_SYMLINKS;
 
 	if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
 		lookup_flags = LOOKUP_FOLLOW;
@@ -5300,7 +5307,7 @@ static int smb2_rename(struct ksmbd_work *work,
 	char *pathname = NULL;
 	struct path path;
 	bool file_present = true;
-	int rc;
+	int rc, lookup_flags = LOOKUP_NO_SYMLINKS;
 
 	ksmbd_debug(SMB, "setting FILE_RENAME_INFO\n");
 	pathname = kmalloc(PATH_MAX, GFP_KERNEL);
@@ -5369,8 +5376,11 @@ static int smb2_rename(struct ksmbd_work *work,
 		goto out;
 	}
 
+	if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
+		lookup_flags = LOOKUP_FOLLOW;
+
 	ksmbd_debug(SMB, "new name %s\n", new_name);
-	rc = ksmbd_vfs_kern_path(new_name, 0, &path, 1);
+	rc = ksmbd_vfs_kern_path(new_name, lookup_flags, &path, 1);
 	if (rc)
 		file_present = false;
 	else
@@ -5420,7 +5430,7 @@ static int smb2_create_link(struct ksmbd_work *work,
 	char *link_name = NULL, *target_name = NULL, *pathname = NULL;
 	struct path path;
 	bool file_present = true;
-	int rc;
+	int rc, lookup_flags = LOOKUP_NO_SYMLINKS;
 
 	if (buf_len < sizeof(struct smb2_file_link_info) +
 			le32_to_cpu(file_info->FileNameLength))
@@ -5447,8 +5457,11 @@ static int smb2_create_link(struct ksmbd_work *work,
 		goto out;
 	}
 
+	if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
+		lookup_flags = LOOKUP_FOLLOW;
+
 	ksmbd_debug(SMB, "target name is %s\n", target_name);
-	rc = ksmbd_vfs_kern_path(link_name, 0, &path, 0);
+	rc = ksmbd_vfs_kern_path(link_name, lookup_flags, &path, 0);
 	if (rc)
 		file_present = false;
 	else
diff --git a/fs/ksmbd/vfs.c b/fs/ksmbd/vfs.c
index b047f2980d96..53047f013371 100644
--- a/fs/ksmbd/vfs.c
+++ b/fs/ksmbd/vfs.c
@@ -164,9 +164,13 @@ int ksmbd_vfs_create(struct ksmbd_work *work, const char *name, umode_t mode)
 {
 	struct path path;
 	struct dentry *dentry;
-	int err;
+	int err, lookup_flags = LOOKUP_NO_SYMLINKS;
+
+	if (test_share_config_flag(work->tcon->share_conf,
+				   KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
+		lookup_flags = LOOKUP_FOLLOW;
 
-	dentry = kern_path_create(AT_FDCWD, name, &path, 0);
+	dentry = kern_path_create(AT_FDCWD, name, &path, lookup_flags);
 	if (IS_ERR(dentry)) {
 		err = PTR_ERR(dentry);
 		if (err != -ENOENT)
@@ -201,9 +205,14 @@ int ksmbd_vfs_mkdir(struct ksmbd_work *work, const char *name, umode_t mode)
 	struct user_namespace *user_ns;
 	struct path path;
 	struct dentry *dentry;
-	int err;
+	int err, lookup_flags = LOOKUP_NO_SYMLINKS;
 
-	dentry = kern_path_create(AT_FDCWD, name, &path, LOOKUP_DIRECTORY);
+	if (test_share_config_flag(work->tcon->share_conf,
+				   KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
+		lookup_flags = LOOKUP_FOLLOW;
+
+	dentry = kern_path_create(AT_FDCWD, name, &path,
+				  lookup_flags | LOOKUP_DIRECTORY);
 	if (IS_ERR(dentry)) {
 		err = PTR_ERR(dentry);
 		if (err != -EEXIST)
@@ -588,7 +597,7 @@ int ksmbd_vfs_remove_file(struct ksmbd_work *work, char *name)
 	struct path path;
 	struct dentry *parent;
 	int err;
-	int flags = 0;
+	int flags = LOOKUP_NO_SYMLINKS;
 
 	if (ksmbd_override_fsids(work))
 		return -ENOMEM;
@@ -652,7 +661,7 @@ int ksmbd_vfs_link(struct ksmbd_work *work, const char *oldname,
 	struct path oldpath, newpath;
 	struct dentry *dentry;
 	int err;
-	int flags = 0;
+	int flags = LOOKUP_NO_SYMLINKS;
 
 	if (ksmbd_override_fsids(work))
 		return -ENOMEM;
@@ -788,7 +797,7 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
 	struct dentry *src_dent, *trap_dent, *src_child;
 	char *dst_name;
 	int err;
-	int flags;
+	int flags = LOOKUP_NO_SYMLINKS;
 
 	dst_name = extract_last_component(newname);
 	if (!dst_name)
@@ -800,7 +809,8 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
 	flags = LOOKUP_DIRECTORY;
 	if (test_share_config_flag(work->tcon->share_conf,
 				   KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
-		flags |= LOOKUP_FOLLOW;
+		flags = LOOKUP_FOLLOW;
+	flags |= LOOKUP_DIRECTORY;
 
 	err = kern_path(newname, flags, &dst_path);
 	if (err) {
@@ -861,7 +871,13 @@ int ksmbd_vfs_truncate(struct ksmbd_work *work, const char *name,
 	int err = 0;
 
 	if (name) {
-		err = kern_path(name, 0, &path);
+		int flags = LOOKUP_NO_SYMLINKS;
+
+		if (test_share_config_flag(work->tcon->share_conf,
+					   KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
+			flags = LOOKUP_FOLLOW;
+
+		err = kern_path(name, flags, &path);
 		if (err) {
 			pr_err("cannot get linux path for %s, err %d\n",
 			       name, err);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 2/4] ksmbd: add validation in smb2_ioctl
  2021-09-19  2:13 [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
  2021-09-19  2:13 ` [PATCH] ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup Namjae Jeon
@ 2021-09-19  2:13 ` Namjae Jeon
  2021-09-21  8:08   ` Ralph Boehme
  2021-09-19  2:13 ` [PATCH v2 3/4] ksmbd: add validation for FILE_FULL_EA_INFORMATION of smb2_get_info Namjae Jeon
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 22+ messages in thread
From: Namjae Jeon @ 2021-09-19  2:13 UTC (permalink / raw)
  To: linux-cifs; +Cc: Namjae Jeon, Ronnie Sahlberg, Ralph Böhme, Steve French

Add validation for request/response buffer size check in smb2_ioctl.

Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 v2:
   - fix warning: variable 'ret' is used uninitialized ret.

 fs/ksmbd/smb2pdu.c | 56 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 7763f69e1ae8..6ea50a9ac64e 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -7021,7 +7021,7 @@ static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req,
 	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;
 
 	cnt_code = le32_to_cpu(req->CntCode);
 	ci_req = (struct copychunk_ioctl_req *)&req->Buffer[0];
@@ -7038,6 +7038,8 @@ 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 */
@@ -7142,7 +7144,8 @@ 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,
+					int out_buf_len)
 {
 	struct network_interface_info_ioctl_rsp *nii_rsp = NULL;
 	int nbytes = 0;
@@ -7225,6 +7228,8 @@ static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn,
 			sockaddr_storage->addr6.ScopeId = 0;
 		}
 
+		if (out_buf_len < sizeof(struct network_interface_info_ioctl_rsp))
+			break;
 		nbytes += sizeof(struct network_interface_info_ioctl_rsp);
 	}
 	rtnl_unlock();
@@ -7245,11 +7250,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,
+					 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) {
@@ -7425,7 +7435,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;
+	int out_buf_len, in_buf_len;
 	u64 id = KSMBD_NO_FID;
 	struct ksmbd_conn *conn = work->conn;
 	int ret = 0;
@@ -7455,6 +7465,7 @@ 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);
+	in_buf_len = le32_to_cpu(req->InputCount);
 
 	switch (cnt_code) {
 	case FSCTL_DFS_GET_REFERRALS:
@@ -7490,9 +7501,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;
 
@@ -7501,7 +7519,8 @@ 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, req, rsp,
+						      out_buf_len);
 		if (nbytes < 0)
 			goto out;
 		break;
@@ -7528,6 +7547,11 @@ 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;
@@ -7537,6 +7561,11 @@ int smb2_ioctl(struct ksmbd_work *work)
 		fsctl_copychunk(work, req, 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)
@@ -7555,6 +7584,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];
 
@@ -7574,6 +7608,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],
@@ -7614,6 +7653,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,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 3/4] ksmbd: add validation for FILE_FULL_EA_INFORMATION of smb2_get_info
  2021-09-19  2:13 [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
  2021-09-19  2:13 ` [PATCH] ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup Namjae Jeon
  2021-09-19  2:13 ` [PATCH v2 2/4] ksmbd: add validation in smb2_ioctl Namjae Jeon
@ 2021-09-19  2:13 ` Namjae Jeon
  2021-09-21  8:09   ` Ralph Boehme
  2021-09-19  2:13 ` [PATCH v2 4/4] ksmbd: add buffer validation for SMB2_CREATE_CONTEXT Namjae Jeon
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 22+ messages in thread
From: Namjae Jeon @ 2021-09-19  2:13 UTC (permalink / raw)
  To: linux-cifs; +Cc: Namjae Jeon, Ronnie Sahlberg, Ralph Böhme, Steve French

Add validation to check whether req->InputBufferLength is smaller than
smb2_ea_info_req structure size.

Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 v2:
   - fix typo of validation in patch subject.
 fs/ksmbd/smb2pdu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 6ea50a9ac64e..117cf242d9b8 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -4059,6 +4059,10 @@ static int smb2_get_ea(struct ksmbd_work *work, struct ksmbd_file *fp,
 	path = &fp->filp->f_path;
 	/* single EA entry is requested with given user.* name */
 	if (req->InputBufferLength) {
+		if (le32_to_cpu(req->InputBufferLength) <
+		    sizeof(struct smb2_ea_info_req))
+			return -EINVAL;
+
 		ea_req = (struct smb2_ea_info_req *)req->Buffer;
 	} else {
 		/* need to send all EAs, if no specific EA is requested*/
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 4/4] ksmbd: add buffer validation for SMB2_CREATE_CONTEXT
  2021-09-19  2:13 [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
                   ` (2 preceding siblings ...)
  2021-09-19  2:13 ` [PATCH v2 3/4] ksmbd: add validation for FILE_FULL_EA_INFORMATION of smb2_get_info Namjae Jeon
@ 2021-09-19  2:13 ` Namjae Jeon
  2021-09-21  8:32   ` Ralph Boehme
  2021-09-20 14:45 ` [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info Ralph Boehme
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 22+ messages in thread
From: Namjae Jeon @ 2021-09-19  2:13 UTC (permalink / raw)
  To: linux-cifs
  Cc: Hyunchul Lee, Ronnie Sahlberg, Ralph Böhme, Steve French,
	Namjae Jeon

From: Hyunchul Lee <hyc.lee@gmail.com>

Add buffer validation for SMB2_CREATE_CONTEXT.

Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/oplock.c  | 35 +++++++++++++++++++++++++----------
 fs/ksmbd/smb2pdu.c | 25 ++++++++++++++++++++++++-
 fs/ksmbd/smbacl.c  |  9 ++++++++-
 3 files changed, 57 insertions(+), 12 deletions(-)

diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c
index 16b6236d1bd2..3fd2713f2282 100644
--- a/fs/ksmbd/oplock.c
+++ b/fs/ksmbd/oplock.c
@@ -1451,26 +1451,41 @@ struct lease_ctx_info *parse_lease_state(void *open_req)
  */
 struct create_context *smb2_find_context_vals(void *open_req, const char *tag)
 {
-	char *data_offset;
+	struct smb2_create_req *req = (struct smb2_create_req *)open_req;
 	struct create_context *cc;
-	unsigned int next = 0;
+	char *data_offset, *data_end;
 	char *name;
-	struct smb2_create_req *req = (struct smb2_create_req *)open_req;
+	unsigned int next = 0;
+	unsigned int name_off, name_len, value_off, value_len;
 
 	data_offset = (char *)req + 4 + le32_to_cpu(req->CreateContextsOffset);
+	data_end = data_offset + le32_to_cpu(req->CreateContextsLength);
 	cc = (struct create_context *)data_offset;
 	do {
-		int val;
-
 		cc = (struct create_context *)((char *)cc + next);
-		name = le16_to_cpu(cc->NameOffset) + (char *)cc;
-		val = le16_to_cpu(cc->NameLength);
-		if (val < 4)
+		if ((char *)cc + offsetof(struct create_context, Buffer) >
+		    data_end)
 			return ERR_PTR(-EINVAL);
 
-		if (memcmp(name, tag, val) == 0)
-			return cc;
 		next = le32_to_cpu(cc->Next);
+		name_off = le16_to_cpu(cc->NameOffset);
+		name_len = le16_to_cpu(cc->NameLength);
+		value_off = le16_to_cpu(cc->DataOffset);
+		value_len = le32_to_cpu(cc->DataLength);
+
+		if ((char *)cc + name_off + name_len > data_end ||
+		    (value_len && (char *)cc + value_off + value_len > data_end))
+			return ERR_PTR(-EINVAL);
+		else if (next && (next < name_off + name_len ||
+			 (value_len && next < value_off + value_len)))
+			return ERR_PTR(-EINVAL);
+
+		name = (char *)cc + name_off;
+		if (name_len < 4)
+			return ERR_PTR(-EINVAL);
+
+		if (memcmp(name, tag, name_len) == 0)
+			return cc;
 	} while (next != 0);
 
 	return NULL;
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 117cf242d9b8..6d57827320e3 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -2393,6 +2393,10 @@ static int smb2_create_sd_buffer(struct ksmbd_work *work,
 	ksmbd_debug(SMB,
 		    "Set ACLs using SMB2_CREATE_SD_BUFFER context\n");
 	sd_buf = (struct create_sd_buf_req *)context;
+	if (le16_to_cpu(context->DataOffset) +
+	    le32_to_cpu(context->DataLength) <
+	    sizeof(struct create_sd_buf_req))
+		return -EINVAL;
 	return set_info_sec(work->conn, work->tcon, path, &sd_buf->ntsd,
 			    le32_to_cpu(sd_buf->ccontext.DataLength), true);
 }
@@ -2593,6 +2597,12 @@ int smb2_open(struct ksmbd_work *work)
 			goto err_out1;
 		} else if (context) {
 			ea_buf = (struct create_ea_buf_req *)context;
+			if (le16_to_cpu(context->DataOffset) +
+			    le32_to_cpu(context->DataLength) <
+			    sizeof(struct create_ea_buf_req)) {
+				rc = -EINVAL;
+				goto err_out1;
+			}
 			if (req->CreateOptions & FILE_NO_EA_KNOWLEDGE_LE) {
 				rsp->hdr.Status = STATUS_ACCESS_DENIED;
 				rc = -EACCES;
@@ -2631,6 +2641,12 @@ int smb2_open(struct ksmbd_work *work)
 			} else if (context) {
 				struct create_posix *posix =
 					(struct create_posix *)context;
+				if (le16_to_cpu(context->DataOffset) +
+				    le32_to_cpu(context->DataLength) <
+				    sizeof(struct create_posix)) {
+					rc = -EINVAL;
+					goto err_out1;
+				}
 				ksmbd_debug(SMB, "get posix context\n");
 
 				posix_mode = le32_to_cpu(posix->Mode);
@@ -3037,9 +3053,16 @@ int smb2_open(struct ksmbd_work *work)
 			rc = PTR_ERR(az_req);
 			goto err_out;
 		} else if (az_req) {
-			loff_t alloc_size = le64_to_cpu(az_req->AllocationSize);
+			loff_t alloc_size;
 			int err;
 
+			if (le16_to_cpu(az_req->ccontext.DataOffset) +
+			    le32_to_cpu(az_req->ccontext.DataLength) <
+			    sizeof(struct create_alloc_size_req)) {
+				rc = -EINVAL;
+				goto err_out;
+			}
+			alloc_size = le64_to_cpu(az_req->AllocationSize);
 			ksmbd_debug(SMB,
 				    "request smb2 create allocate size : %llu\n",
 				    alloc_size);
diff --git a/fs/ksmbd/smbacl.c b/fs/ksmbd/smbacl.c
index 0a95cdec8c80..f67567e1e178 100644
--- a/fs/ksmbd/smbacl.c
+++ b/fs/ksmbd/smbacl.c
@@ -392,7 +392,7 @@ static void parse_dacl(struct user_namespace *user_ns,
 		return;
 
 	/* validate that we do not go past end of acl */
-	if (end_of_acl <= (char *)pdacl ||
+	if (end_of_acl < (char *)pdacl + sizeof(struct smb_acl) ||
 	    end_of_acl < (char *)pdacl + le16_to_cpu(pdacl->size)) {
 		pr_err("ACL too small to parse DACL\n");
 		return;
@@ -434,6 +434,10 @@ static void parse_dacl(struct user_namespace *user_ns,
 		ppace[i] = (struct smb_ace *)(acl_base + acl_size);
 		acl_base = (char *)ppace[i];
 		acl_size = le16_to_cpu(ppace[i]->size);
+
+		if (acl_base + acl_size > end_of_acl)
+			break;
+
 		ppace[i]->access_req =
 			smb_map_generic_desired_access(ppace[i]->access_req);
 
@@ -807,6 +811,9 @@ int parse_sec_desc(struct user_namespace *user_ns, struct smb_ntsd *pntsd,
 	if (!pntsd)
 		return -EIO;
 
+	if (acl_len < sizeof(struct smb_ntsd))
+		return -EINVAL;
+
 	owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
 			le32_to_cpu(pntsd->osidoffset));
 	group_sid_ptr = (struct smb_sid *)((char *)pntsd +
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info
  2021-09-19  2:13 [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
                   ` (3 preceding siblings ...)
  2021-09-19  2:13 ` [PATCH v2 4/4] ksmbd: add buffer validation for SMB2_CREATE_CONTEXT Namjae Jeon
@ 2021-09-20 14:45 ` Ralph Boehme
  2021-09-20 15:03   ` Ralph Boehme
  2021-09-20 15:38 ` Ralph Boehme
  2021-09-21 14:23 ` Tom Talpey
  6 siblings, 1 reply; 22+ messages in thread
From: Ralph Boehme @ 2021-09-20 14:45 UTC (permalink / raw)
  To: Namjae Jeon, linux-cifs; +Cc: Ronnie Sahlberg, Steve French


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

Am 19.09.21 um 04:13 schrieb Namjae Jeon:
> Use  LOOKUP_NO_SYMLINKS flags for default lookup to prohibit the
> middle of symlink component lookup.

maybe this patch should be squashed with the "ksmbd: remove follow
symlinks support" patch?

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info
  2021-09-20 14:45 ` [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info Ralph Boehme
@ 2021-09-20 15:03   ` Ralph Boehme
  2021-09-20 15:10     ` Steve French
  0 siblings, 1 reply; 22+ messages in thread
From: Ralph Boehme @ 2021-09-20 15:03 UTC (permalink / raw)
  To: Namjae Jeon, linux-cifs; +Cc: Ronnie Sahlberg, Steve French


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

Am 20.09.21 um 16:45 schrieb Ralph Boehme:
> Am 19.09.21 um 04:13 schrieb Namjae Jeon:
>> Use  LOOKUP_NO_SYMLINKS flags for default lookup to prohibit the
>> middle of symlink component lookup.
> 
> maybe this patch should be squashed with the "ksmbd: remove follow
> symlinks support" patch?

also, I noticed that the patches are already included in ksmbd-for-next. 
Did I miss Steve's ack on the ML?

I wonder why the patches are already included in ksmbd-for-next without 
a proper review, I just started to look at the patches and wanted to 
raise several issues.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info
  2021-09-20 15:03   ` Ralph Boehme
@ 2021-09-20 15:10     ` Steve French
  2021-09-20 16:11       ` Ralph Boehme
  0 siblings, 1 reply; 22+ messages in thread
From: Steve French @ 2021-09-20 15:10 UTC (permalink / raw)
  To: Ralph Boehme; +Cc: Namjae Jeon, CIFS, Ronnie Sahlberg

On Mon, Sep 20, 2021 at 10:03 AM Ralph Boehme <slow@samba.org> wrote:
>
> Am 20.09.21 um 16:45 schrieb Ralph Boehme:
> > Am 19.09.21 um 04:13 schrieb Namjae Jeon:
> >> Use  LOOKUP_NO_SYMLINKS flags for default lookup to prohibit the
> >> middle of symlink component lookup.
> >
> > maybe this patch should be squashed with the "ksmbd: remove follow
> > symlinks support" patch?
>
> also, I noticed that the patches are already included in ksmbd-for-next.
> Did I miss Steve's ack on the ML?
>
> I wonder why the patches are already included in ksmbd-for-next without
> a proper review, I just started to look at the patches and wanted to
> raise several issues.

I included them at Namjae's request in for-next to allow the automated
tests to run on them (e.g. the Intel test robot etc.) - those
automated bots can be useful ... but I had done some review of all of
them, and detailed review of most, and had run the automated tests
(buildbot) on them (which passed, even after adding more subtests),
and the smbtorture tests were also automatically run (it is triggered
in Namjae's github setup).

Of the 8 patches in for-next, these 3 are the remaining ones that I am
looking at in more detail now:

24f0f4fc5f76 ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup
1ec1e6928354 ksmbd: add buffer validation for SMB2_CREATE_CONTEXT
e2cd5c814442 ksmbd: add validation in smb2_ioctl





-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info
  2021-09-19  2:13 [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
                   ` (4 preceding siblings ...)
  2021-09-20 14:45 ` [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info Ralph Boehme
@ 2021-09-20 15:38 ` Ralph Boehme
  2021-09-20 16:18   ` Namjae Jeon
  2021-09-21 14:23 ` Tom Talpey
  6 siblings, 1 reply; 22+ messages in thread
From: Ralph Boehme @ 2021-09-20 15:38 UTC (permalink / raw)
  To: Namjae Jeon; +Cc: linux-cifs


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

Hi Namjae,

patch looks great, a few nitpicks inline...

Am 19.09.21 um 04:13 schrieb Namjae Jeon:
> Add buffer validation in smb2_set_info.
> 
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>   fs/ksmbd/smb2pdu.c | 113 +++++++++++++++++++++++++++++++++++----------
>   fs/ksmbd/smb2pdu.h |   9 ++++
>   2 files changed, 97 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 46e0275a77a8..7763f69e1ae8 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -2107,17 +2107,23 @@ 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;
>   
> +	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)
>   		return -ENOMEM;
> @@ -2181,7 +2187,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 < eabuf->EaNameLength + le16_to_cpu(eabuf->EaValueLength))
> +			break;
> +
>   	} while (next != 0);
>   
>   	kfree(attr_name);
> @@ -2790,7 +2802,9 @@ 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);
> +			rc = smb2_set_ea(&ea_buf->ea,
> +					 le32_to_cpu(ea_buf->ccontext.DataLength),
> +					 &path);
>   			if (rc == -EOPNOTSUPP)
>   				rc = 0;
>   			else if (rc)
> @@ -5375,7 +5389,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,
> +			    int buf_len, struct file *filp,
>   			    struct nls_table *local_nls)
>   {
>   	char *link_name = NULL, *target_name = NULL, *pathname = NULL;
> @@ -5383,6 +5397,10 @@ static int smb2_create_link(struct ksmbd_work *work,
>   	bool file_present = true;
>   	int rc;
>   
> +	if (buf_len < 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)
> @@ -5442,7 +5460,7 @@ static int smb2_create_link(struct ksmbd_work *work,
>   static int set_file_basic_info(struct ksmbd_file *fp, char *buf,
>   			       struct ksmbd_share_config *share)
>   {
> -	struct smb2_file_all_info *file_info;
> +	struct smb2_file_basic_info *file_info;

this change should be moved to a seperate commit.

>   	struct iattr attrs;
>   	struct timespec64 ctime;
>   	struct file *filp;
> @@ -5453,7 +5471,7 @@ 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;
> +	file_info = (struct smb2_file_basic_info *)buf;

same here.

>   	attrs.ia_valid = 0;
>   	filp = fp->filp;
>   	inode = file_inode(filp);
> @@ -5619,7 +5637,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,
> +			   int buf_len)
>   {
>   	struct user_namespace *user_ns;
>   	struct ksmbd_file *parent_fp;
> @@ -5632,6 +5651,10 @@ static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
>   		return -EACCES;
>   	}
>   
> +	if (buf_len < 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;
> @@ -5654,8 +5677,7 @@ 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);
>   }
>   
> @@ -5741,40 +5763,71 @@ 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) {
> +	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, req->Buffer, share);

set_file_basic_info() should take a pointer to struct 
smb2_file_basic_info to make it clear that the buffer size has already 
been validated.

The same needs to be changed in the several other infolevel handlers.

> +	}
>   	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, req->Buffer);

here

> +	}
>   	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, req->Buffer);

here.

> +	}
>   	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, req->Buffer);

here

> +	}
>   	case FILE_FULL_EA_INFORMATION:
>   	{
>   		if (!(fp->daccess & FILE_WRITE_EA_LE)) {
> @@ -5783,18 +5836,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, req->Buffer);

here

> +	}
>   	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, req->Buffer);

here

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info
  2021-09-20 15:10     ` Steve French
@ 2021-09-20 16:11       ` Ralph Boehme
  2021-09-20 16:20         ` Steve French
  0 siblings, 1 reply; 22+ messages in thread
From: Ralph Boehme @ 2021-09-20 16:11 UTC (permalink / raw)
  To: Steve French; +Cc: Namjae Jeon, CIFS, Ronnie Sahlberg


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

Am 20.09.21 um 17:10 schrieb Steve French:
> On Mon, Sep 20, 2021 at 10:03 AM Ralph Boehme <slow@samba.org> wrote:
>>
>> Am 20.09.21 um 16:45 schrieb Ralph Boehme:
>>> Am 19.09.21 um 04:13 schrieb Namjae Jeon:
>>>> Use  LOOKUP_NO_SYMLINKS flags for default lookup to prohibit the
>>>> middle of symlink component lookup.
>>>
>>> maybe this patch should be squashed with the "ksmbd: remove follow
>>> symlinks support" patch?
>>
>> also, I noticed that the patches are already included in ksmbd-for-next.
>> Did I miss Steve's ack on the ML?
>>
>> I wonder why the patches are already included in ksmbd-for-next without
>> a proper review, I just started to look at the patches and wanted to
>> raise several issues.
> 
> I included them at Namjae's request in for-next to allow the automated
> tests to run on them (e.g. the Intel test robot etc.) - those
> automated bots can be useful ... but I had done some review of all of
> them, and detailed review of most, and had run the automated tests
> (buildbot) on them (which passed, even after adding more subtests),
> and the smbtorture tests were also automatically run (it is triggered
> in Namjae's github setup).
> 
> Of the 8 patches in for-next, these 3 are the remaining ones that I am
> looking at in more detail now:
> 
> 24f0f4fc5f76 ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup
> 1ec1e6928354 ksmbd: add buffer validation for SMB2_CREATE_CONTEXT
> e2cd5c814442 ksmbd: add validation in smb2_ioctl

ok, thanks for explaining.

To be honest, I'm still trying to make sense of the patch workflow. 
Hopefully I get there eventually.

How can I detect if a patch is already reviewed and queued for upstrea 
merge, so it's "too late" for me to do a review?

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info
  2021-09-20 15:38 ` Ralph Boehme
@ 2021-09-20 16:18   ` Namjae Jeon
  0 siblings, 0 replies; 22+ messages in thread
From: Namjae Jeon @ 2021-09-20 16:18 UTC (permalink / raw)
  To: Ralph Boehme; +Cc: linux-cifs

2021-09-21 0:38 GMT+09:00, Ralph Boehme <slow@samba.org>:
> Hi Namjae,
>
> patch looks great, a few nitpicks inline...
Okay, I have checked the below your comments. I will fix it on v2.

Thanks for your review!
>
> Am 19.09.21 um 04:13 schrieb Namjae Jeon:
>> Add buffer validation in smb2_set_info.
>>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>   fs/ksmbd/smb2pdu.c | 113 +++++++++++++++++++++++++++++++++++----------
>>   fs/ksmbd/smb2pdu.h |   9 ++++
>>   2 files changed, 97 insertions(+), 25 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index 46e0275a77a8..7763f69e1ae8 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -2107,17 +2107,23 @@ 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;
>>
>> +	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)
>>   		return -ENOMEM;
>> @@ -2181,7 +2187,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 < eabuf->EaNameLength + le16_to_cpu(eabuf->EaValueLength))
>> +			break;
>> +
>>   	} while (next != 0);
>>
>>   	kfree(attr_name);
>> @@ -2790,7 +2802,9 @@ 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);
>> +			rc = smb2_set_ea(&ea_buf->ea,
>> +					 le32_to_cpu(ea_buf->ccontext.DataLength),
>> +					 &path);
>>   			if (rc == -EOPNOTSUPP)
>>   				rc = 0;
>>   			else if (rc)
>> @@ -5375,7 +5389,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,
>> +			    int buf_len, struct file *filp,
>>   			    struct nls_table *local_nls)
>>   {
>>   	char *link_name = NULL, *target_name = NULL, *pathname = NULL;
>> @@ -5383,6 +5397,10 @@ static int smb2_create_link(struct ksmbd_work
>> *work,
>>   	bool file_present = true;
>>   	int rc;
>>
>> +	if (buf_len < 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)
>> @@ -5442,7 +5460,7 @@ static int smb2_create_link(struct ksmbd_work
>> *work,
>>   static int set_file_basic_info(struct ksmbd_file *fp, char *buf,
>>   			       struct ksmbd_share_config *share)
>>   {
>> -	struct smb2_file_all_info *file_info;
>> +	struct smb2_file_basic_info *file_info;
>
> this change should be moved to a seperate commit.
>
>>   	struct iattr attrs;
>>   	struct timespec64 ctime;
>>   	struct file *filp;
>> @@ -5453,7 +5471,7 @@ 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;
>> +	file_info = (struct smb2_file_basic_info *)buf;
>
> same here.
>
>>   	attrs.ia_valid = 0;
>>   	filp = fp->filp;
>>   	inode = file_inode(filp);
>> @@ -5619,7 +5637,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,
>> +			   int buf_len)
>>   {
>>   	struct user_namespace *user_ns;
>>   	struct ksmbd_file *parent_fp;
>> @@ -5632,6 +5651,10 @@ static int set_rename_info(struct ksmbd_work *work,
>> struct ksmbd_file *fp,
>>   		return -EACCES;
>>   	}
>>
>> +	if (buf_len < 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;
>> @@ -5654,8 +5677,7 @@ 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);
>>   }
>>
>> @@ -5741,40 +5763,71 @@ 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) {
>> +	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, req->Buffer, share);
>
> set_file_basic_info() should take a pointer to struct
> smb2_file_basic_info to make it clear that the buffer size has already
> been validated.
>
> The same needs to be changed in the several other infolevel handlers.
>
>> +	}
>>   	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, req->Buffer);
>
> here
>
>> +	}
>>   	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, req->Buffer);
>
> here.
>
>> +	}
>>   	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, req->Buffer);
>
> here
>
>> +	}
>>   	case FILE_FULL_EA_INFORMATION:
>>   	{
>>   		if (!(fp->daccess & FILE_WRITE_EA_LE)) {
>> @@ -5783,18 +5836,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, req->Buffer);
>
> here
>
>> +	}
>>   	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, req->Buffer);
>
> here
>
> Cheers!
> -slow
>
> --
> Ralph Boehme, Samba Team                 https://samba.org/
> SerNet Samba Team Lead      https://sernet.de/en/team-samba
>
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info
  2021-09-20 16:11       ` Ralph Boehme
@ 2021-09-20 16:20         ` Steve French
  2021-09-20 16:30           ` Ralph Boehme
  0 siblings, 1 reply; 22+ messages in thread
From: Steve French @ 2021-09-20 16:20 UTC (permalink / raw)
  To: Ralph Boehme; +Cc: Namjae Jeon, CIFS, Ronnie Sahlberg

On Mon, Sep 20, 2021 at 11:11 AM Ralph Boehme <slow@samba.org> wrote:
>
> Am 20.09.21 um 17:10 schrieb Steve French:
> > On Mon, Sep 20, 2021 at 10:03 AM Ralph Boehme <slow@samba.org> wrote:
> >>
> >> Am 20.09.21 um 16:45 schrieb Ralph Boehme:
> >>> Am 19.09.21 um 04:13 schrieb Namjae Jeon:
> >>>> Use  LOOKUP_NO_SYMLINKS flags for default lookup to prohibit the
> >>>> middle of symlink component lookup.
> >>>
> >>> maybe this patch should be squashed with the "ksmbd: remove follow
> >>> symlinks support" patch?
> >>
> >> also, I noticed that the patches are already included in ksmbd-for-next.
> >> Did I miss Steve's ack on the ML?
> >>
> >> I wonder why the patches are already included in ksmbd-for-next without
> >> a proper review, I just started to look at the patches and wanted to
> >> raise several issues.
> >
> > I included them at Namjae's request in for-next to allow the automated
> > tests to run on them (e.g. the Intel test robot etc.) - those
> > automated bots can be useful ... but I had done some review of all of
> > them, and detailed review of most, and had run the automated tests
> > (buildbot) on them (which passed, even after adding more subtests),
> > and the smbtorture tests were also automatically run (it is triggered
> > in Namjae's github setup).
> >
> > Of the 8 patches in for-next, these 3 are the remaining ones that I am
> > looking at in more detail now:
> >
> > 24f0f4fc5f76 ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup
> > 1ec1e6928354 ksmbd: add buffer validation for SMB2_CREATE_CONTEXT
> > e2cd5c814442 ksmbd: add validation in smb2_ioctl
>
> ok, thanks for explaining.
>
> To be honest, I'm still trying to make sense of the patch workflow.
> Hopefully I get there eventually.
>
> How can I detect if a patch is already reviewed and queued for upstrea
> merge, so it's "too late" for me to do a review?

It is not too late to do review of any of these 8.  Given the
importance of security, the more reviews the better.  Earliest we
would send them (the larger set of 8) upstream would be in a few days.
  I typically like to have them sit in for-next for 48 hours (although
in some cases make exceptions, e.g.  for important bug fixes I will
shorten this if later in the week so they make it in time for the next
rc)

-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info
  2021-09-20 16:20         ` Steve French
@ 2021-09-20 16:30           ` Ralph Boehme
  0 siblings, 0 replies; 22+ messages in thread
From: Ralph Boehme @ 2021-09-20 16:30 UTC (permalink / raw)
  To: Steve French; +Cc: Namjae Jeon, CIFS, Ronnie Sahlberg


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

Am 20.09.21 um 18:20 schrieb Steve French:
> It is not too late to do review of any of these 8.  Given the
> importance of security, the more reviews the better.  Earliest we
> would send them (the larger set of 8) upstream would be in a few days.
>    I typically like to have them sit in for-next for 48 hours (although
> in some cases make exceptions, e.g.  for important bug fixes I will
> shorten this if later in the week so they make it in time for the next
> rc)

which "for-next"?

git://git.samba.org/ksmbd.git/ksmbd-for-next ?

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 2/4] ksmbd: add validation in smb2_ioctl
  2021-09-19  2:13 ` [PATCH v2 2/4] ksmbd: add validation in smb2_ioctl Namjae Jeon
@ 2021-09-21  8:08   ` Ralph Boehme
  2021-09-21 11:15     ` Namjae Jeon
  0 siblings, 1 reply; 22+ messages in thread
From: Ralph Boehme @ 2021-09-21  8:08 UTC (permalink / raw)
  To: Namjae Jeon, linux-cifs; +Cc: Ronnie Sahlberg, Steve French


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

Hi Namjae,


Am 19.09.21 um 04:13 schrieb Namjae Jeon:
> Add validation for request/response buffer size check in smb2_ioctl.
> 
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>

the check looks good. May I suggest to also pass the pointers to the 
structs instead of the raw buffers consistently, same as we do for the 
setinfo levels now?

We still pass the raw buffer to fsctl_query_iface_info_ioctl and 
fsctl_copychunk().

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 3/4] ksmbd: add validation for FILE_FULL_EA_INFORMATION of smb2_get_info
  2021-09-19  2:13 ` [PATCH v2 3/4] ksmbd: add validation for FILE_FULL_EA_INFORMATION of smb2_get_info Namjae Jeon
@ 2021-09-21  8:09   ` Ralph Boehme
  0 siblings, 0 replies; 22+ messages in thread
From: Ralph Boehme @ 2021-09-21  8:09 UTC (permalink / raw)
  To: Namjae Jeon, linux-cifs; +Cc: Ronnie Sahlberg, Steve French


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

Am 19.09.21 um 04:13 schrieb Namjae Jeon:
> Add validation to check whether req->InputBufferLength is smaller than
> smb2_ea_info_req structure size.
> 
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>

reviewed-by: me.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 4/4] ksmbd: add buffer validation for SMB2_CREATE_CONTEXT
  2021-09-19  2:13 ` [PATCH v2 4/4] ksmbd: add buffer validation for SMB2_CREATE_CONTEXT Namjae Jeon
@ 2021-09-21  8:32   ` Ralph Boehme
  2021-09-22  0:26     ` Namjae Jeon
  0 siblings, 1 reply; 22+ messages in thread
From: Ralph Boehme @ 2021-09-21  8:32 UTC (permalink / raw)
  To: Namjae Jeon, linux-cifs; +Cc: Hyunchul Lee, Ronnie Sahlberg, Steve French


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

Hi Namjae,

thanks! One nitpick below.

Am 19.09.21 um 04:13 schrieb Namjae Jeon:
> From: Hyunchul Lee <hyc.lee@gmail.com>
> 
> Add buffer validation for SMB2_CREATE_CONTEXT.
> 
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>   fs/ksmbd/oplock.c  | 35 +++++++++++++++++++++++++----------
>   fs/ksmbd/smb2pdu.c | 25 ++++++++++++++++++++++++-
>   fs/ksmbd/smbacl.c  |  9 ++++++++-
>   3 files changed, 57 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c
> index 16b6236d1bd2..3fd2713f2282 100644
> --- a/fs/ksmbd/oplock.c
> +++ b/fs/ksmbd/oplock.c
> @@ -1451,26 +1451,41 @@ struct lease_ctx_info *parse_lease_state(void *open_req)
>    */
>   struct create_context *smb2_find_context_vals(void *open_req, const char *tag)
>   {
> -	char *data_offset;
> +	struct smb2_create_req *req = (struct smb2_create_req *)open_req;
>   	struct create_context *cc;
> -	unsigned int next = 0;
> +	char *data_offset, *data_end;
>   	char *name;
> -	struct smb2_create_req *req = (struct smb2_create_req *)open_req;
> +	unsigned int next = 0;
> +	unsigned int name_off, name_len, value_off, value_len;
>   
>   	data_offset = (char *)req + 4 + le32_to_cpu(req->CreateContextsOffset);
> +	data_end = data_offset + le32_to_cpu(req->CreateContextsLength);
>   	cc = (struct create_context *)data_offset;
>   	do {
> -		int val;
> -
>   		cc = (struct create_context *)((char *)cc + next);
> -		name = le16_to_cpu(cc->NameOffset) + (char *)cc;
> -		val = le16_to_cpu(cc->NameLength);
> -		if (val < 4)
> +		if ((char *)cc + offsetof(struct create_context, Buffer) >
> +		    data_end)
>   			return ERR_PTR(-EINVAL);
>   
> -		if (memcmp(name, tag, val) == 0)
> -			return cc;
>   		next = le32_to_cpu(cc->Next);
> +		name_off = le16_to_cpu(cc->NameOffset);
> +		name_len = le16_to_cpu(cc->NameLength);
> +		value_off = le16_to_cpu(cc->DataOffset);
> +		value_len = le32_to_cpu(cc->DataLength);
> +
> +		if ((char *)cc + name_off + name_len > data_end ||
> +		    (value_len && (char *)cc + value_off + value_len > data_end))
> +			return ERR_PTR(-EINVAL);
> +		else if (next && (next < name_off + name_len ||
> +			 (value_len && next < value_off + value_len)))
> +			return ERR_PTR(-EINVAL);

The else is a bit confusing and not needed. Also, Samba has a few 
additional checks, I wonder whether we should add those two:

                 if ((next & 0x7) != 0 ||
                     next > remaining ||
                     name_offset != 16 ||
                     name_length < 4 ||
                     name_offset + name_length > remaining ||
                     (data_offset & 0x7) != 0 ||
                     (data_offset && (data_offset < name_offset + 
name_length)) ||
                     (data_offset > remaining) ||
                     (data_offset + (uint64_t)data_length > remaining)) {
                         return NT_STATUS_INVALID_PARAMETER;
                 }

Other then that lgtm.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 2/4] ksmbd: add validation in smb2_ioctl
  2021-09-21  8:08   ` Ralph Boehme
@ 2021-09-21 11:15     ` Namjae Jeon
  0 siblings, 0 replies; 22+ messages in thread
From: Namjae Jeon @ 2021-09-21 11:15 UTC (permalink / raw)
  To: Ralph Boehme; +Cc: linux-cifs, Ronnie Sahlberg, Steve French

2021-09-21 17:08 GMT+09:00, Ralph Boehme <slow@samba.org>:
> Hi Namjae,
>
>
> Am 19.09.21 um 04:13 schrieb Namjae Jeon:
>> Add validation for request/response buffer size check in smb2_ioctl.
>>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>
> the check looks good. May I suggest to also pass the pointers to the
> structs instead of the raw buffers consistently, same as we do for the
> setinfo levels now?
>
> We still pass the raw buffer to fsctl_query_iface_info_ioctl and
> fsctl_copychunk().
Hm.. Let me check it. maybe will fix it on another patch.
>
> -slow
>
> --
> Ralph Boehme, Samba Team                 https://samba.org/
> SerNet Samba Team Lead      https://sernet.de/en/team-samba
>
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info
  2021-09-19  2:13 [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
                   ` (5 preceding siblings ...)
  2021-09-20 15:38 ` Ralph Boehme
@ 2021-09-21 14:23 ` Tom Talpey
  2021-09-22  2:31   ` Namjae Jeon
  6 siblings, 1 reply; 22+ messages in thread
From: Tom Talpey @ 2021-09-21 14:23 UTC (permalink / raw)
  To: Namjae Jeon, linux-cifs; +Cc: Ronnie Sahlberg, Ralph Böhme, Steve French

On 9/18/2021 10:13 PM, Namjae Jeon wrote:
> Add buffer validation in smb2_set_info.
> 
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>   fs/ksmbd/smb2pdu.c | 113 +++++++++++++++++++++++++++++++++++----------
>   fs/ksmbd/smb2pdu.h |   9 ++++
>   2 files changed, 97 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 46e0275a77a8..7763f69e1ae8 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -2107,17 +2107,23 @@ 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;
>   
> +	if (buf_len < sizeof(struct smb2_ea_info) + eabuf->EaNameLength +
> +			le16_to_cpu(eabuf->EaValueLength))
> +		return -EINVAL;

How certain is it that EaNameLength and EaValueLength are sane? One
might imagine a forged packet with various combinations of invalid
values, which arithmetically satisfy the above check...

> +
>   	attr_name = kmalloc(XATTR_NAME_MAX + 1, GFP_KERNEL);
>   	if (!attr_name)
>   		return -ENOMEM;
> @@ -2181,7 +2187,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;

Because buf_len is unsigned int and next is signed int, these compares
are risky. I think "next" should also be unsigned, but if not, testing
it for negative values before these checks is important.

In the many changes below, buf_len is passed in as signed. Those should
be consistent with the same criteria. It pays to be paranoid everywhere!

Tom.

>   		eabuf = (struct smb2_ea_info *)((char *)eabuf + next);
> +		if (next < eabuf->EaNameLength + le16_to_cpu(eabuf->EaValueLength))
> +			break;
> +
>   	} while (next != 0);
>   
>   	kfree(attr_name);
> @@ -2790,7 +2802,9 @@ 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);
> +			rc = smb2_set_ea(&ea_buf->ea,
> +					 le32_to_cpu(ea_buf->ccontext.DataLength),
> +					 &path);

Again, is DataLength verified?

>   			if (rc == -EOPNOTSUPP)
>   				rc = 0;
>   			else if (rc)
> @@ -5375,7 +5389,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,
> +			    int buf_len, struct file *filp,
>   			    struct nls_table *local_nls)
>   {
>   	char *link_name = NULL, *target_name = NULL, *pathname = NULL;
> @@ -5383,6 +5397,10 @@ static int smb2_create_link(struct ksmbd_work *work,
>   	bool file_present = true;
>   	int rc;
>   
> +	if (buf_len < 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)
> @@ -5442,7 +5460,7 @@ static int smb2_create_link(struct ksmbd_work *work,
>   static int set_file_basic_info(struct ksmbd_file *fp, char *buf,
>   			       struct ksmbd_share_config *share)
>   {
> -	struct smb2_file_all_info *file_info;
> +	struct smb2_file_basic_info *file_info;
>   	struct iattr attrs;
>   	struct timespec64 ctime;
>   	struct file *filp;
> @@ -5453,7 +5471,7 @@ 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;
> +	file_info = (struct smb2_file_basic_info *)buf;
>   	attrs.ia_valid = 0;
>   	filp = fp->filp;
>   	inode = file_inode(filp);
> @@ -5619,7 +5637,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,
> +			   int buf_len)
>   {
>   	struct user_namespace *user_ns;
>   	struct ksmbd_file *parent_fp;
> @@ -5632,6 +5651,10 @@ static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
>   		return -EACCES;
>   	}
>   
> +	if (buf_len < 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;
> @@ -5654,8 +5677,7 @@ 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);
>   }
>   
> @@ -5741,40 +5763,71 @@ 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) {
> +	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, 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, 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, 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, req->Buffer);
> +	}
>   	case FILE_FULL_EA_INFORMATION:
>   	{
>   		if (!(fp->daccess & FILE_WRITE_EA_LE)) {
> @@ -5783,18 +5836,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, 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, req->Buffer);
> +	}
>   	}
>   
> -	pr_err("Unimplemented Fileinfoclass :%d\n", info_class);
> +	pr_err("Unimplemented Fileinfoclass :%d\n", req->FileInfoClass);
>   	return -EOPNOTSUPP;
>   }
>   
> @@ -5855,8 +5919,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];
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 4/4] ksmbd: add buffer validation for SMB2_CREATE_CONTEXT
  2021-09-21  8:32   ` Ralph Boehme
@ 2021-09-22  0:26     ` Namjae Jeon
  0 siblings, 0 replies; 22+ messages in thread
From: Namjae Jeon @ 2021-09-22  0:26 UTC (permalink / raw)
  To: Ralph Boehme; +Cc: linux-cifs, Hyunchul Lee, Ronnie Sahlberg, Steve French

2021-09-21 17:32 GMT+09:00, Ralph Boehme <slow@samba.org>:
> Hi Namjae,
>
> thanks! One nitpick below.
>
> Am 19.09.21 um 04:13 schrieb Namjae Jeon:
>> From: Hyunchul Lee <hyc.lee@gmail.com>
>>
>> Add buffer validation for SMB2_CREATE_CONTEXT.
>>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>   fs/ksmbd/oplock.c  | 35 +++++++++++++++++++++++++----------
>>   fs/ksmbd/smb2pdu.c | 25 ++++++++++++++++++++++++-
>>   fs/ksmbd/smbacl.c  |  9 ++++++++-
>>   3 files changed, 57 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c
>> index 16b6236d1bd2..3fd2713f2282 100644
>> --- a/fs/ksmbd/oplock.c
>> +++ b/fs/ksmbd/oplock.c
>> @@ -1451,26 +1451,41 @@ struct lease_ctx_info *parse_lease_state(void
>> *open_req)
>>    */
>>   struct create_context *smb2_find_context_vals(void *open_req, const char
>> *tag)
>>   {
>> -	char *data_offset;
>> +	struct smb2_create_req *req = (struct smb2_create_req *)open_req;
>>   	struct create_context *cc;
>> -	unsigned int next = 0;
>> +	char *data_offset, *data_end;
>>   	char *name;
>> -	struct smb2_create_req *req = (struct smb2_create_req *)open_req;
>> +	unsigned int next = 0;
>> +	unsigned int name_off, name_len, value_off, value_len;
>>
>>   	data_offset = (char *)req + 4 +
>> le32_to_cpu(req->CreateContextsOffset);
>> +	data_end = data_offset + le32_to_cpu(req->CreateContextsLength);
>>   	cc = (struct create_context *)data_offset;
>>   	do {
>> -		int val;
>> -
>>   		cc = (struct create_context *)((char *)cc + next);
>> -		name = le16_to_cpu(cc->NameOffset) + (char *)cc;
>> -		val = le16_to_cpu(cc->NameLength);
>> -		if (val < 4)
>> +		if ((char *)cc + offsetof(struct create_context, Buffer) >
>> +		    data_end)
>>   			return ERR_PTR(-EINVAL);
>>
>> -		if (memcmp(name, tag, val) == 0)
>> -			return cc;
>>   		next = le32_to_cpu(cc->Next);
>> +		name_off = le16_to_cpu(cc->NameOffset);
>> +		name_len = le16_to_cpu(cc->NameLength);
>> +		value_off = le16_to_cpu(cc->DataOffset);
>> +		value_len = le32_to_cpu(cc->DataLength);
>> +
>> +		if ((char *)cc + name_off + name_len > data_end ||
>> +		    (value_len && (char *)cc + value_off + value_len > data_end))
>> +			return ERR_PTR(-EINVAL);
>> +		else if (next && (next < name_off + name_len ||
>> +			 (value_len && next < value_off + value_len)))
>> +			return ERR_PTR(-EINVAL);
>
> The else is a bit confusing and not needed. Also, Samba has a few
> additional checks, I wonder whether we should add those two:
>
>                  if ((next & 0x7) != 0 ||
>                      next > remaining ||
>                      name_offset != 16 ||
>                      name_length < 4 ||
>                      name_offset + name_length > remaining ||
>                      (data_offset & 0x7) != 0 ||
>                      (data_offset && (data_offset < name_offset +
> name_length)) ||
>                      (data_offset > remaining) ||
>                      (data_offset + (uint64_t)data_length > remaining)) {
>                          return NT_STATUS_INVALID_PARAMETER;
>                  }
I will fix it on v2.

Thank your review!
>
> Other then that lgtm.
>
> Thanks!
> -slow
>
> --
> Ralph Boehme, Samba Team                 https://samba.org/
> SerNet Samba Team Lead      https://sernet.de/en/team-samba
>
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info
  2021-09-21 14:23 ` Tom Talpey
@ 2021-09-22  2:31   ` Namjae Jeon
  2021-09-22  3:40     ` Namjae Jeon
  0 siblings, 1 reply; 22+ messages in thread
From: Namjae Jeon @ 2021-09-22  2:31 UTC (permalink / raw)
  To: Tom Talpey; +Cc: linux-cifs, Ronnie Sahlberg, Ralph Böhme, Steve French

2021-09-21 23:23 GMT+09:00, Tom Talpey <tom@talpey.com>:
> On 9/18/2021 10:13 PM, Namjae Jeon wrote:
>> Add buffer validation in smb2_set_info.
>>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>   fs/ksmbd/smb2pdu.c | 113 +++++++++++++++++++++++++++++++++++----------
>>   fs/ksmbd/smb2pdu.h |   9 ++++
>>   2 files changed, 97 insertions(+), 25 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index 46e0275a77a8..7763f69e1ae8 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -2107,17 +2107,23 @@ 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;
>>
>> +	if (buf_len < sizeof(struct smb2_ea_info) + eabuf->EaNameLength +
>> +			le16_to_cpu(eabuf->EaValueLength))
>> +		return -EINVAL;
>
> How certain is it that EaNameLength and EaValueLength are sane? One
> might imagine a forged packet with various combinations of invalid
> values, which arithmetically satisfy the above check...
Sorry, I didn't fully understand what you pointed out. Could you
please elaborate more ?

>
>> +
>>   	attr_name = kmalloc(XATTR_NAME_MAX + 1, GFP_KERNEL);
>>   	if (!attr_name)
>>   		return -ENOMEM;
>> @@ -2181,7 +2187,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;
>
> Because buf_len is unsigned int and next is signed int, these compares
> are risky. I think "next" should also be unsigned, but if not, testing
> it for negative values before these checks is important.
>
> In the many changes below, buf_len is passed in as signed. Those should
> be consistent with the same criteria. It pays to be paranoid everywhere!
Okay, I will update it on next version.
>
> Tom.
>
>>   		eabuf = (struct smb2_ea_info *)((char *)eabuf + next);
>> +		if (next < eabuf->EaNameLength + le16_to_cpu(eabuf->EaValueLength))
>> +			break;
>> +
>>   	} while (next != 0);
>>
>>   	kfree(attr_name);
>> @@ -2790,7 +2802,9 @@ 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);
>> +			rc = smb2_set_ea(&ea_buf->ea,
>> +					 le32_to_cpu(ea_buf->ccontext.DataLength),
>> +					 &path);
>
> Again, is DataLength verified?
This field is checked by create context validation patch.
See: https://marc.info/?l=linux-cifs&m=163227401430586&w=2
>
>>   			if (rc == -EOPNOTSUPP)
>>   				rc = 0;
>>   			else if (rc)
>> @@ -5375,7 +5389,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,
>> +			    int buf_len, struct file *filp,
>>   			    struct nls_table *local_nls)
>>   {
>>   	char *link_name = NULL, *target_name = NULL, *pathname = NULL;
>> @@ -5383,6 +5397,10 @@ static int smb2_create_link(struct ksmbd_work
>> *work,
>>   	bool file_present = true;
>>   	int rc;
>>
>> +	if (buf_len < 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)
>> @@ -5442,7 +5460,7 @@ static int smb2_create_link(struct ksmbd_work
>> *work,
>>   static int set_file_basic_info(struct ksmbd_file *fp, char *buf,
>>   			       struct ksmbd_share_config *share)
>>   {
>> -	struct smb2_file_all_info *file_info;
>> +	struct smb2_file_basic_info *file_info;
>>   	struct iattr attrs;
>>   	struct timespec64 ctime;
>>   	struct file *filp;
>> @@ -5453,7 +5471,7 @@ 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;
>> +	file_info = (struct smb2_file_basic_info *)buf;
>>   	attrs.ia_valid = 0;
>>   	filp = fp->filp;
>>   	inode = file_inode(filp);
>> @@ -5619,7 +5637,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,
>> +			   int buf_len)
>>   {
>>   	struct user_namespace *user_ns;
>>   	struct ksmbd_file *parent_fp;
>> @@ -5632,6 +5651,10 @@ static int set_rename_info(struct ksmbd_work *work,
>> struct ksmbd_file *fp,
>>   		return -EACCES;
>>   	}
>>
>> +	if (buf_len < 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;
>> @@ -5654,8 +5677,7 @@ 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);
>>   }
>>
>> @@ -5741,40 +5763,71 @@ 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) {
>> +	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, 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, 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, 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, req->Buffer);
>> +	}
>>   	case FILE_FULL_EA_INFORMATION:
>>   	{
>>   		if (!(fp->daccess & FILE_WRITE_EA_LE)) {
>> @@ -5783,18 +5836,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, 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, req->Buffer);
>> +	}
>>   	}
>>
>> -	pr_err("Unimplemented Fileinfoclass :%d\n", info_class);
>> +	pr_err("Unimplemented Fileinfoclass :%d\n", req->FileInfoClass);
>>   	return -EOPNOTSUPP;
>>   }
>>
>> @@ -5855,8 +5919,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];
>>
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info
  2021-09-22  2:31   ` Namjae Jeon
@ 2021-09-22  3:40     ` Namjae Jeon
  2021-09-22 18:39       ` Tom Talpey
  0 siblings, 1 reply; 22+ messages in thread
From: Namjae Jeon @ 2021-09-22  3:40 UTC (permalink / raw)
  To: Tom Talpey; +Cc: linux-cifs, Ronnie Sahlberg, Ralph Böhme, Steve French

2021-09-22 11:31 GMT+09:00, Namjae Jeon <linkinjeon@kernel.org>:
> 2021-09-21 23:23 GMT+09:00, Tom Talpey <tom@talpey.com>:
>> On 9/18/2021 10:13 PM, Namjae Jeon wrote:
>>> Add buffer validation in smb2_set_info.
>>>
>>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>>> Cc: Ralph Böhme <slow@samba.org>
>>> Cc: Steve French <smfrench@gmail.com>
>>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>>> ---
>>>   fs/ksmbd/smb2pdu.c | 113 +++++++++++++++++++++++++++++++++++----------
>>>   fs/ksmbd/smb2pdu.h |   9 ++++
>>>   2 files changed, 97 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>>> index 46e0275a77a8..7763f69e1ae8 100644
>>> --- a/fs/ksmbd/smb2pdu.c
>>> +++ b/fs/ksmbd/smb2pdu.c
>>> @@ -2107,17 +2107,23 @@ 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;
>>>
>>> +	if (buf_len < sizeof(struct smb2_ea_info) + eabuf->EaNameLength +
>>> +			le16_to_cpu(eabuf->EaValueLength))
>>> +		return -EINVAL;
>>
>> How certain is it that EaNameLength and EaValueLength are sane? One
>> might imagine a forged packet with various combinations of invalid
>> values, which arithmetically satisfy the above check...
> Sorry, I didn't fully understand what you pointed out. Could you
> please elaborate more ?

Maybe, You are saying we need the below check?
@@ -2577,6 +2581,12 @@ int smb2_open(struct ksmbd_work *work)
 			goto err_out1;
 		} else if (context) {
 			ea_buf = (struct create_ea_buf_req *)context;
+			if (le16_to_cpu(context->DataOffset) +
+			    le32_to_cpu(context->DataLength) <
+			    sizeof(struct create_ea_buf_req)) {
+				rc = -EINVAL;
+				goto err_out1;
+			}

This check is in create context patch.
(https://marc.info/?l=linux-cifs&m=163227401430586&w=2)
>
>>
>>> +
>>>   	attr_name = kmalloc(XATTR_NAME_MAX + 1, GFP_KERNEL);
>>>   	if (!attr_name)
>>>   		return -ENOMEM;
>>> @@ -2181,7 +2187,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;
>>
>> Because buf_len is unsigned int and next is signed int, these compares
>> are risky. I think "next" should also be unsigned, but if not, testing
>> it for negative values before these checks is important.
>>
>> In the many changes below, buf_len is passed in as signed. Those should
>> be consistent with the same criteria. It pays to be paranoid everywhere!
> Okay, I will update it on next version.
>>
>> Tom.
>>
>>>   		eabuf = (struct smb2_ea_info *)((char *)eabuf + next);
>>> +		if (next < eabuf->EaNameLength + le16_to_cpu(eabuf->EaValueLength))
>>> +			break;
>>> +
>>>   	} while (next != 0);
>>>
>>>   	kfree(attr_name);
>>> @@ -2790,7 +2802,9 @@ 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);
>>> +			rc = smb2_set_ea(&ea_buf->ea,
>>> +					 le32_to_cpu(ea_buf->ccontext.DataLength),
>>> +					 &path);
>>
>> Again, is DataLength verified?
> This field is checked by create context validation patch.
> See: https://marc.info/?l=linux-cifs&m=163227401430586&w=2
>>
>>>   			if (rc == -EOPNOTSUPP)
>>>   				rc = 0;
>>>   			else if (rc)
>>> @@ -5375,7 +5389,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,
>>> +			    int buf_len, struct file *filp,
>>>   			    struct nls_table *local_nls)
>>>   {
>>>   	char *link_name = NULL, *target_name = NULL, *pathname = NULL;
>>> @@ -5383,6 +5397,10 @@ static int smb2_create_link(struct ksmbd_work
>>> *work,
>>>   	bool file_present = true;
>>>   	int rc;
>>>
>>> +	if (buf_len < 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)
>>> @@ -5442,7 +5460,7 @@ static int smb2_create_link(struct ksmbd_work
>>> *work,
>>>   static int set_file_basic_info(struct ksmbd_file *fp, char *buf,
>>>   			       struct ksmbd_share_config *share)
>>>   {
>>> -	struct smb2_file_all_info *file_info;
>>> +	struct smb2_file_basic_info *file_info;
>>>   	struct iattr attrs;
>>>   	struct timespec64 ctime;
>>>   	struct file *filp;
>>> @@ -5453,7 +5471,7 @@ 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;
>>> +	file_info = (struct smb2_file_basic_info *)buf;
>>>   	attrs.ia_valid = 0;
>>>   	filp = fp->filp;
>>>   	inode = file_inode(filp);
>>> @@ -5619,7 +5637,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,
>>> +			   int buf_len)
>>>   {
>>>   	struct user_namespace *user_ns;
>>>   	struct ksmbd_file *parent_fp;
>>> @@ -5632,6 +5651,10 @@ static int set_rename_info(struct ksmbd_work
>>> *work,
>>> struct ksmbd_file *fp,
>>>   		return -EACCES;
>>>   	}
>>>
>>> +	if (buf_len < 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;
>>> @@ -5654,8 +5677,7 @@ 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);
>>>   }
>>>
>>> @@ -5741,40 +5763,71 @@ 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) {
>>> +	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, 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, 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, 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, req->Buffer);
>>> +	}
>>>   	case FILE_FULL_EA_INFORMATION:
>>>   	{
>>>   		if (!(fp->daccess & FILE_WRITE_EA_LE)) {
>>> @@ -5783,18 +5836,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, 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, req->Buffer);
>>> +	}
>>>   	}
>>>
>>> -	pr_err("Unimplemented Fileinfoclass :%d\n", info_class);
>>> +	pr_err("Unimplemented Fileinfoclass :%d\n", req->FileInfoClass);
>>>   	return -EOPNOTSUPP;
>>>   }
>>>
>>> @@ -5855,8 +5919,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];
>>>
>>
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info
  2021-09-22  3:40     ` Namjae Jeon
@ 2021-09-22 18:39       ` Tom Talpey
  0 siblings, 0 replies; 22+ messages in thread
From: Tom Talpey @ 2021-09-22 18:39 UTC (permalink / raw)
  To: Namjae Jeon; +Cc: linux-cifs, Ronnie Sahlberg, Ralph Böhme, Steve French

On 9/21/2021 11:40 PM, Namjae Jeon wrote:
> 2021-09-22 11:31 GMT+09:00, Namjae Jeon <linkinjeon@kernel.org>:
>> 2021-09-21 23:23 GMT+09:00, Tom Talpey <tom@talpey.com>:
>>> On 9/18/2021 10:13 PM, Namjae Jeon wrote:
>>>> Add buffer validation in smb2_set_info.
>>>>
>>>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>>>> Cc: Ralph Böhme <slow@samba.org>
>>>> Cc: Steve French <smfrench@gmail.com>
>>>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>>>> ---
>>>>    fs/ksmbd/smb2pdu.c | 113 +++++++++++++++++++++++++++++++++++----------
>>>>    fs/ksmbd/smb2pdu.h |   9 ++++
>>>>    2 files changed, 97 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>>>> index 46e0275a77a8..7763f69e1ae8 100644
>>>> --- a/fs/ksmbd/smb2pdu.c
>>>> +++ b/fs/ksmbd/smb2pdu.c
>>>> @@ -2107,17 +2107,23 @@ 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;
>>>>
>>>> +	if (buf_len < sizeof(struct smb2_ea_info) + eabuf->EaNameLength +
>>>> +			le16_to_cpu(eabuf->EaValueLength))
>>>> +		return -EINVAL;
>>>
>>> How certain is it that EaNameLength and EaValueLength are sane? One
>>> might imagine a forged packet with various combinations of invalid
>>> values, which arithmetically satisfy the above check...
>> Sorry, I didn't fully understand what you pointed out. Could you
>> please elaborate more ?
> 
> Maybe, You are saying we need the below check?
> @@ -2577,6 +2581,12 @@ int smb2_open(struct ksmbd_work *work)
>   			goto err_out1;
>   		} else if (context) {
>   			ea_buf = (struct create_ea_buf_req *)context;
> +			if (le16_to_cpu(context->DataOffset) +
> +			    le32_to_cpu(context->DataLength) <
> +			    sizeof(struct create_ea_buf_req)) {
> +				rc = -EINVAL;
> +				goto err_out1;
> +			}
> 
> This check is in create context patch.
> (https://marc.info/?l=linux-cifs&m=163227401430586&w=2)

Ah, yes something like that. I'll look over the other patch thanks.

Tom.
Tom.

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2021-09-22 18:39 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-19  2:13 [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
2021-09-19  2:13 ` [PATCH] ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup Namjae Jeon
2021-09-19  2:13 ` [PATCH v2 2/4] ksmbd: add validation in smb2_ioctl Namjae Jeon
2021-09-21  8:08   ` Ralph Boehme
2021-09-21 11:15     ` Namjae Jeon
2021-09-19  2:13 ` [PATCH v2 3/4] ksmbd: add validation for FILE_FULL_EA_INFORMATION of smb2_get_info Namjae Jeon
2021-09-21  8:09   ` Ralph Boehme
2021-09-19  2:13 ` [PATCH v2 4/4] ksmbd: add buffer validation for SMB2_CREATE_CONTEXT Namjae Jeon
2021-09-21  8:32   ` Ralph Boehme
2021-09-22  0:26     ` Namjae Jeon
2021-09-20 14:45 ` [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info Ralph Boehme
2021-09-20 15:03   ` Ralph Boehme
2021-09-20 15:10     ` Steve French
2021-09-20 16:11       ` Ralph Boehme
2021-09-20 16:20         ` Steve French
2021-09-20 16:30           ` Ralph Boehme
2021-09-20 15:38 ` Ralph Boehme
2021-09-20 16:18   ` Namjae Jeon
2021-09-21 14:23 ` Tom Talpey
2021-09-22  2:31   ` Namjae Jeon
2021-09-22  3:40     ` Namjae Jeon
2021-09-22 18:39       ` Tom Talpey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).