linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Tom Talpey <tom@talpey.com>
To: Namjae Jeon <linkinjeon@kernel.org>, linux-cifs@vger.kernel.org
Cc: "Ronnie Sahlberg" <ronniesahlberg@gmail.com>,
	"Ralph Böhme" <slow@samba.org>,
	"Steve French" <smfrench@gmail.com>
Subject: Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info
Date: Tue, 21 Sep 2021 10:23:15 -0400	[thread overview]
Message-ID: <890f7c14-b0b9-12e1-8d2e-e1ca5fb9c2d5@talpey.com> (raw)
In-Reply-To: <20210919021315.642856-1-linkinjeon@kernel.org>

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

  parent reply	other threads:[~2021-09-21 14:23 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-19  2:13 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 [this message]
2021-09-22  2:31   ` Namjae Jeon
2021-09-22  3:40     ` Namjae Jeon
2021-09-22 18:39       ` Tom Talpey

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=890f7c14-b0b9-12e1-8d2e-e1ca5fb9c2d5@talpey.com \
    --to=tom@talpey.com \
    --cc=linkinjeon@kernel.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=ronniesahlberg@gmail.com \
    --cc=slow@samba.org \
    --cc=smfrench@gmail.com \
    --subject='Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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