All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hyunchul Lee <hyc.lee@gmail.com>
To: Namjae Jeon <linkinjeon@kernel.org>
Cc: linux-cifs <linux-cifs@vger.kernel.org>,
	"Tom Talpey" <tom@talpey.com>,
	"Ronnie Sahlberg" <ronniesahlberg@gmail.com>,
	"Ralph Böhme" <slow@samba.org>,
	"Steve French" <smfrench@gmail.com>,
	"Sergey Senozhatsky" <senozhatsky@chromium.org>
Subject: Re: [PATCH 2/7] ksmbd: add request buffer validation in smb2_set_info
Date: Sat, 25 Sep 2021 17:13:56 +0900	[thread overview]
Message-ID: <CANFS6bYot0-tRYUdBctwmwLn3OUsYOWO_C3a3aZu74tXG2k=Mw@mail.gmail.com> (raw)
In-Reply-To: <20210924021254.27096-3-linkinjeon@kernel.org>

2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@kernel.org>님이 작성:

>
> Add buffer validation in smb2_set_info, and remove unused variable
> in set_file_basic_info. and smb2_set_info infolevel functions take
> structure pointer argument.
>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>  fs/ksmbd/smb2pdu.c | 149 ++++++++++++++++++++++++++++++++-------------
>  fs/ksmbd/smb2pdu.h |   9 +++
>  2 files changed, 116 insertions(+), 42 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index b22d4207c077..a930838fd6ac 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -2107,16 +2107,22 @@ static noinline int create_smb2_pipe(struct ksmbd_work *work)
>   * smb2_set_ea() - handler for setting extended attributes using set
>   *             info command
>   * @eabuf:     set info command buffer
> + * @buf_len:   set info command buffer length
>   * @path:      dentry path for get ea
>   *
>   * Return:     0 on success, otherwise error
>   */
> -static int smb2_set_ea(struct smb2_ea_info *eabuf, struct path *path)
> +static int smb2_set_ea(struct smb2_ea_info *eabuf, unsigned int buf_len,
> +                      struct path *path)
>  {
>         struct user_namespace *user_ns = mnt_user_ns(path->mnt);
>         char *attr_name = NULL, *value;
>         int rc = 0;
> -       int next = 0;
> +       unsigned int next = 0;
> +
> +       if (buf_len < sizeof(struct smb2_ea_info) + eabuf->EaNameLength +
> +                       le16_to_cpu(eabuf->EaValueLength))
> +               return -EINVAL;
>
>         attr_name = kmalloc(XATTR_NAME_MAX + 1, GFP_KERNEL);
>         if (!attr_name)
> @@ -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);
> @@ -2771,7 +2783,15 @@ int smb2_open(struct ksmbd_work *work)
>                 created = true;
>                 user_ns = mnt_user_ns(path.mnt);
>                 if (ea_buf) {
> -                       rc = smb2_set_ea(&ea_buf->ea, &path);
> +                       if (le32_to_cpu(ea_buf->ccontext.DataLength) <
> +                           sizeof(struct smb2_ea_info)) {
> +                               rc = -EINVAL;
> +                               goto err_out;
> +                       }
> +
> +                       rc = smb2_set_ea(&ea_buf->ea,
> +                                        le32_to_cpu(ea_buf->ccontext.DataLength),
> +                                        &path);
>                         if (rc == -EOPNOTSUPP)
>                                 rc = 0;
>                         else if (rc)
> @@ -5354,7 +5374,7 @@ static int smb2_rename(struct ksmbd_work *work,
>  static int smb2_create_link(struct ksmbd_work *work,
>                             struct ksmbd_share_config *share,
>                             struct smb2_file_link_info *file_info,
> -                           struct file *filp,
> +                           unsigned int buf_len, struct file *filp,
>                             struct nls_table *local_nls)
>  {
>         char *link_name = NULL, *target_name = NULL, *pathname = NULL;
> @@ -5362,6 +5382,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))

Do we need to cast this to u64 or check FileNameLength to prevent
32-bit overflow?

> +               return -EINVAL;
> +
>         ksmbd_debug(SMB, "setting FILE_LINK_INFORMATION\n");
>         pathname = kmalloc(PATH_MAX, GFP_KERNEL);
>         if (!pathname)
> @@ -5418,10 +5442,10 @@ static int smb2_create_link(struct ksmbd_work *work,
>         return rc;
>  }
>
> -static int set_file_basic_info(struct ksmbd_file *fp, char *buf,
> +static int set_file_basic_info(struct ksmbd_file *fp,
> +                              struct smb2_file_basic_info *file_info,
>                                struct ksmbd_share_config *share)
>  {
> -       struct smb2_file_all_info *file_info;
>         struct iattr attrs;
>         struct timespec64 ctime;
>         struct file *filp;
> @@ -5432,7 +5456,6 @@ static int set_file_basic_info(struct ksmbd_file *fp, char *buf,
>         if (!(fp->daccess & FILE_WRITE_ATTRIBUTES_LE))
>                 return -EACCES;
>
> -       file_info = (struct smb2_file_all_info *)buf;
>         attrs.ia_valid = 0;
>         filp = fp->filp;
>         inode = file_inode(filp);
> @@ -5509,7 +5532,8 @@ static int set_file_basic_info(struct ksmbd_file *fp, char *buf,
>  }
>
>  static int set_file_allocation_info(struct ksmbd_work *work,
> -                                   struct ksmbd_file *fp, char *buf)
> +                                   struct ksmbd_file *fp,
> +                                   struct smb2_file_alloc_info *file_alloc_info)
>  {
>         /*
>          * TODO : It's working fine only when store dos attributes
> @@ -5517,7 +5541,6 @@ static int set_file_allocation_info(struct ksmbd_work *work,
>          * properly with any smb.conf option
>          */
>
> -       struct smb2_file_alloc_info *file_alloc_info;
>         loff_t alloc_blks;
>         struct inode *inode;
>         int rc;
> @@ -5525,7 +5548,6 @@ static int set_file_allocation_info(struct ksmbd_work *work,
>         if (!(fp->daccess & FILE_WRITE_DATA_LE))
>                 return -EACCES;
>
> -       file_alloc_info = (struct smb2_file_alloc_info *)buf;
>         alloc_blks = (le64_to_cpu(file_alloc_info->AllocationSize) + 511) >> 9;
>         inode = file_inode(fp->filp);
>
> @@ -5561,9 +5583,8 @@ static int set_file_allocation_info(struct ksmbd_work *work,
>  }
>
>  static int set_end_of_file_info(struct ksmbd_work *work, struct ksmbd_file *fp,
> -                               char *buf)
> +                               struct smb2_file_eof_info *file_eof_info)
>  {
> -       struct smb2_file_eof_info *file_eof_info;
>         loff_t newsize;
>         struct inode *inode;
>         int rc;
> @@ -5571,7 +5592,6 @@ static int set_end_of_file_info(struct ksmbd_work *work, struct ksmbd_file *fp,
>         if (!(fp->daccess & FILE_WRITE_DATA_LE))
>                 return -EACCES;
>
> -       file_eof_info = (struct smb2_file_eof_info *)buf;
>         newsize = le64_to_cpu(file_eof_info->EndOfFile);
>         inode = file_inode(fp->filp);
>
> @@ -5598,7 +5618,8 @@ static int set_end_of_file_info(struct ksmbd_work *work, struct ksmbd_file *fp,
>  }
>
>  static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
> -                          char *buf)
> +                          struct smb2_file_rename_info *rename_info,
> +                          unsigned int buf_len)
>  {
>         struct user_namespace *user_ns;
>         struct ksmbd_file *parent_fp;
> @@ -5611,6 +5632,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))

Also we need to cast or check FileNameLength?
Looks good to me except these.

> +               return -EINVAL;
> +
>         user_ns = file_mnt_user_ns(fp->filp);
>         if (ksmbd_stream_fd(fp))
>                 goto next;
> @@ -5633,14 +5658,13 @@ static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
>                 }
>         }
>  next:
> -       return smb2_rename(work, fp, user_ns,
> -                          (struct smb2_file_rename_info *)buf,
> +       return smb2_rename(work, fp, user_ns, rename_info,
>                            work->sess->conn->local_nls);
>  }
>
> -static int set_file_disposition_info(struct ksmbd_file *fp, char *buf)
> +static int set_file_disposition_info(struct ksmbd_file *fp,
> +                                    struct smb2_file_disposition_info *file_info)
>  {
> -       struct smb2_file_disposition_info *file_info;
>         struct inode *inode;
>
>         if (!(fp->daccess & FILE_DELETE_LE)) {
> @@ -5649,7 +5673,6 @@ static int set_file_disposition_info(struct ksmbd_file *fp, char *buf)
>         }
>
>         inode = file_inode(fp->filp);
> -       file_info = (struct smb2_file_disposition_info *)buf;
>         if (file_info->DeletePending) {
>                 if (S_ISDIR(inode->i_mode) &&
>                     ksmbd_vfs_empty_dir(fp) == -ENOTEMPTY)
> @@ -5661,15 +5684,14 @@ static int set_file_disposition_info(struct ksmbd_file *fp, char *buf)
>         return 0;
>  }
>
> -static int set_file_position_info(struct ksmbd_file *fp, char *buf)
> +static int set_file_position_info(struct ksmbd_file *fp,
> +                                 struct smb2_file_pos_info *file_info)
>  {
> -       struct smb2_file_pos_info *file_info;
>         loff_t current_byte_offset;
>         unsigned long sector_size;
>         struct inode *inode;
>
>         inode = file_inode(fp->filp);
> -       file_info = (struct smb2_file_pos_info *)buf;
>         current_byte_offset = le64_to_cpu(file_info->CurrentByteOffset);
>         sector_size = inode->i_sb->s_blocksize;
>
> @@ -5685,12 +5707,11 @@ static int set_file_position_info(struct ksmbd_file *fp, char *buf)
>         return 0;
>  }
>
> -static int set_file_mode_info(struct ksmbd_file *fp, char *buf)
> +static int set_file_mode_info(struct ksmbd_file *fp,
> +                             struct smb2_file_mode_info *file_info)
>  {
> -       struct smb2_file_mode_info *file_info;
>         __le32 mode;
>
> -       file_info = (struct smb2_file_mode_info *)buf;
>         mode = file_info->Mode;
>
>         if ((mode & ~FILE_MODE_INFO_MASK) ||
> @@ -5720,40 +5741,74 @@ static int set_file_mode_info(struct ksmbd_file *fp, char *buf)
>   * TODO: need to implement an error handling for STATUS_INFO_LENGTH_MISMATCH
>   */
>  static int smb2_set_info_file(struct ksmbd_work *work, struct ksmbd_file *fp,
> -                             int info_class, char *buf,
> +                             struct smb2_set_info_req *req,
>                               struct ksmbd_share_config *share)
>  {
> -       switch (info_class) {
> +       unsigned int buf_len = le32_to_cpu(req->BufferLength);
> +
> +       switch (req->FileInfoClass) {
>         case FILE_BASIC_INFORMATION:
> -               return set_file_basic_info(fp, buf, share);
> +       {
> +               if (buf_len < sizeof(struct smb2_file_basic_info))
> +                       return -EINVAL;
>
> +               return set_file_basic_info(fp, (struct smb2_file_basic_info *)req->Buffer, share);
> +       }
>         case FILE_ALLOCATION_INFORMATION:
> -               return set_file_allocation_info(work, fp, buf);
> +       {
> +               if (buf_len < sizeof(struct smb2_file_alloc_info))
> +                       return -EINVAL;
>
> +               return set_file_allocation_info(work, fp,
> +                                               (struct smb2_file_alloc_info *)req->Buffer);
> +       }
>         case FILE_END_OF_FILE_INFORMATION:
> -               return set_end_of_file_info(work, fp, buf);
> +       {
> +               if (buf_len < sizeof(struct smb2_file_eof_info))
> +                       return -EINVAL;
>
> +               return set_end_of_file_info(work, fp,
> +                                           (struct smb2_file_eof_info *)req->Buffer);
> +       }
>         case FILE_RENAME_INFORMATION:
> +       {
>                 if (!test_tree_conn_flag(work->tcon, KSMBD_TREE_CONN_FLAG_WRITABLE)) {
>                         ksmbd_debug(SMB,
>                                     "User does not have write permission\n");
>                         return -EACCES;
>                 }
> -               return set_rename_info(work, fp, buf);
>
> +               if (buf_len < sizeof(struct smb2_file_rename_info))
> +                       return -EINVAL;
> +
> +               return set_rename_info(work, fp,
> +                                      (struct smb2_file_rename_info *)req->Buffer,
> +                                      buf_len);
> +       }
>         case FILE_LINK_INFORMATION:
> +       {
> +               if (buf_len < sizeof(struct smb2_file_link_info))
> +                       return -EINVAL;
> +
>                 return smb2_create_link(work, work->tcon->share_conf,
> -                                       (struct smb2_file_link_info *)buf, fp->filp,
> +                                       (struct smb2_file_link_info *)req->Buffer,
> +                                       buf_len, fp->filp,
>                                         work->sess->conn->local_nls);
> -
> +       }
>         case FILE_DISPOSITION_INFORMATION:
> +       {
>                 if (!test_tree_conn_flag(work->tcon, KSMBD_TREE_CONN_FLAG_WRITABLE)) {
>                         ksmbd_debug(SMB,
>                                     "User does not have write permission\n");
>                         return -EACCES;
>                 }
> -               return set_file_disposition_info(fp, buf);
>
> +               if (buf_len < sizeof(struct smb2_file_disposition_info))
> +                       return -EINVAL;
> +
> +               return set_file_disposition_info(fp,
> +                                                (struct smb2_file_disposition_info *)req->Buffer);
> +       }
>         case FILE_FULL_EA_INFORMATION:
>         {
>                 if (!(fp->daccess & FILE_WRITE_EA_LE)) {
> @@ -5762,18 +5817,29 @@ static int smb2_set_info_file(struct ksmbd_work *work, struct ksmbd_file *fp,
>                         return -EACCES;
>                 }
>
> -               return smb2_set_ea((struct smb2_ea_info *)buf,
> -                                  &fp->filp->f_path);
> -       }
> +               if (buf_len < sizeof(struct smb2_ea_info))
> +                       return -EINVAL;
>
> +               return smb2_set_ea((struct smb2_ea_info *)req->Buffer,
> +                                  buf_len, &fp->filp->f_path);
> +       }
>         case FILE_POSITION_INFORMATION:
> -               return set_file_position_info(fp, buf);
> +       {
> +               if (buf_len < sizeof(struct smb2_file_pos_info))
> +                       return -EINVAL;
>
> +               return set_file_position_info(fp, (struct smb2_file_pos_info *)req->Buffer);
> +       }
>         case FILE_MODE_INFORMATION:
> -               return set_file_mode_info(fp, buf);
> +       {
> +               if (buf_len < sizeof(struct smb2_file_mode_info))
> +                       return -EINVAL;
> +
> +               return set_file_mode_info(fp, (struct smb2_file_mode_info *)req->Buffer);
> +       }
>         }
>
> -       pr_err("Unimplemented Fileinfoclass :%d\n", info_class);
> +       pr_err("Unimplemented Fileinfoclass :%d\n", req->FileInfoClass);
>         return -EOPNOTSUPP;
>  }
>
> @@ -5834,8 +5900,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
>


--
Thanks,
Hyunchul

  reply	other threads:[~2021-09-25  8:22 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-24  2:12 [PATCH 0/7] a bunch of patches that have not yet been reviewed Namjae Jeon
2021-09-24  2:12 ` [PATCH 1/7] ksmbd: add validation in smb2_ioctl Namjae Jeon
2021-09-25 10:16   ` Hyunchul Lee
2021-09-25 10:44     ` Namjae Jeon
2021-09-24  2:12 ` [PATCH 2/7] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
2021-09-25  8:13   ` Hyunchul Lee [this message]
2021-09-25  9:19     ` Namjae Jeon
2021-09-24  2:12 ` [PATCH 3/7] ksmbd: remove RFC1002 check in smb2 request Namjae Jeon
2021-09-25  8:18   ` Hyunchul Lee
2021-09-24  2:12 ` [PATCH 4/7] ksmbd: check strictly data area in ksmbd_smb2_check_message() Namjae Jeon
2021-09-25 10:27   ` Hyunchul Lee
2021-09-25 10:46     ` Namjae Jeon
2021-09-24  2:12 ` [PATCH 5/7] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value Namjae Jeon
2021-09-25  8:41   ` Hyunchul Lee
2021-09-25  9:24     ` Namjae Jeon
2021-09-24  2:12 ` [PATCH 6/7] ksmbd: fix invalid request buffer access in compound Namjae Jeon
2021-09-25  9:41   ` Hyunchul Lee
2021-09-24  2:12 ` [PATCH 7/7] ksmbd: add validation in smb2 negotiate Namjae Jeon
2021-09-24  4:58   ` Namjae Jeon

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='CANFS6bYot0-tRYUdBctwmwLn3OUsYOWO_C3a3aZu74tXG2k=Mw@mail.gmail.com' \
    --to=hyc.lee@gmail.com \
    --cc=linkinjeon@kernel.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=ronniesahlberg@gmail.com \
    --cc=senozhatsky@chromium.org \
    --cc=slow@samba.org \
    --cc=smfrench@gmail.com \
    --cc=tom@talpey.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.