Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
Cc: Ralph Böhme <slow@xxxxxxxxx>
Cc: Steve French <smfrench@xxxxxxxxx>
Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
---
  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: OpenPGP_signature
Description: OpenPGP digital signature


[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux