2021-09-21 0:38 GMT+09:00, Ralph Boehme <slow@xxxxxxxxx>:
> Hi Namjae,
>
> patch looks great, a few nitpicks inline...
Okay, I have checked the below your comments. I will fix it on v2.
Thanks for your review!
>
> 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
>
>
