All of lore.kernel.org
 help / color / mirror / Atom feed
From: Namjae Jeon <linkinjeon@kernel.org>
To: Hyunchul Lee <hyc.lee@gmail.com>
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 18:19:33 +0900	[thread overview]
Message-ID: <CAKYAXd8a7ZbZ2GHJXba8853-ysrpA==K15M=4wKbsjVwEJaDqA@mail.gmail.com> (raw)
In-Reply-To: <CANFS6bYot0-tRYUdBctwmwLn3OUsYOWO_C3a3aZu74tXG2k=Mw@mail.gmail.com>

2021-09-25 17:13 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> 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?
Yes, I will fix it on next version.

>
>> +               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?
Yes.
> Looks good to me except these.
Thanks for your review!
>
>> +               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  9:19 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
2021-09-25  9:19     ` Namjae Jeon [this message]
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='CAKYAXd8a7ZbZ2GHJXba8853-ysrpA==K15M=4wKbsjVwEJaDqA@mail.gmail.com' \
    --to=linkinjeon@kernel.org \
    --cc=hyc.lee@gmail.com \
    --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 \
    --subject='Re: [PATCH 2/7] 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 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.