From: Ralph Boehme <slow@samba.org>
To: Namjae Jeon <linkinjeon@kernel.org>
Cc: linux-cifs@vger.kernel.org
Subject: Re: [PATCH v2 1/4] ksmbd: add request buffer validation in smb2_set_info
Date: Mon, 20 Sep 2021 17:38:08 +0200 [thread overview]
Message-ID: <7f51fab9-ff31-aee0-aee2-e91e8bc45d07@samba.org> (raw)
In-Reply-To: <20210919021315.642856-1-linkinjeon@kernel.org>
[-- 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 --]
next prev parent reply other threads:[~2021-09-20 15:38 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
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=7f51fab9-ff31-aee0-aee2-e91e8bc45d07@samba.org \
--to=slow@samba.org \
--cc=linkinjeon@kernel.org \
--cc=linux-cifs@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).