2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@xxxxxxxxxx>님이 작성:
>
> 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@xxxxxxxxxx>
> Cc: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
> Cc: Ralph Böhme <slow@xxxxxxxxx>
> Cc: Steve French <smfrench@xxxxxxxxx>
> Cc: Hyunchul Lee <hyc.lee@xxxxxxxxx>
> Cc: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
> ---
> 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
