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 > Cc: Ralph Böhme > Cc: Steve French > Signed-off-by: Namjae Jeon > --- > 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