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@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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
