| Message ID | 20210919021315.642856-1-linkinjeon@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/4] ksmbd: add request buffer validation in smb2_set_info | expand |
Am 19.09.21 um 04:13 schrieb Namjae Jeon: > Use LOOKUP_NO_SYMLINKS flags for default lookup to prohibit the > middle of symlink component lookup. maybe this patch should be squashed with the "ksmbd: remove follow symlinks support" patch? -slow
Am 20.09.21 um 16:45 schrieb Ralph Boehme: > Am 19.09.21 um 04:13 schrieb Namjae Jeon: >> Use LOOKUP_NO_SYMLINKS flags for default lookup to prohibit the >> middle of symlink component lookup. > > maybe this patch should be squashed with the "ksmbd: remove follow > symlinks support" patch? also, I noticed that the patches are already included in ksmbd-for-next. Did I miss Steve's ack on the ML? I wonder why the patches are already included in ksmbd-for-next without a proper review, I just started to look at the patches and wanted to raise several issues. -slow
On Mon, Sep 20, 2021 at 10:03 AM Ralph Boehme <slow@samba.org> wrote: > > Am 20.09.21 um 16:45 schrieb Ralph Boehme: > > Am 19.09.21 um 04:13 schrieb Namjae Jeon: > >> Use LOOKUP_NO_SYMLINKS flags for default lookup to prohibit the > >> middle of symlink component lookup. > > > > maybe this patch should be squashed with the "ksmbd: remove follow > > symlinks support" patch? > > also, I noticed that the patches are already included in ksmbd-for-next. > Did I miss Steve's ack on the ML? > > I wonder why the patches are already included in ksmbd-for-next without > a proper review, I just started to look at the patches and wanted to > raise several issues. I included them at Namjae's request in for-next to allow the automated tests to run on them (e.g. the Intel test robot etc.) - those automated bots can be useful ... but I had done some review of all of them, and detailed review of most, and had run the automated tests (buildbot) on them (which passed, even after adding more subtests), and the smbtorture tests were also automatically run (it is triggered in Namjae's github setup). Of the 8 patches in for-next, these 3 are the remaining ones that I am looking at in more detail now: 24f0f4fc5f76 ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup 1ec1e6928354 ksmbd: add buffer validation for SMB2_CREATE_CONTEXT e2cd5c814442 ksmbd: add validation in smb2_ioctl
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@gmail.com> > Cc: Ralph Böhme <slow@samba.org> > Cc: Steve French <smfrench@gmail.com> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > --- > 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
Am 20.09.21 um 17:10 schrieb Steve French: > On Mon, Sep 20, 2021 at 10:03 AM Ralph Boehme <slow@samba.org> wrote: >> >> Am 20.09.21 um 16:45 schrieb Ralph Boehme: >>> Am 19.09.21 um 04:13 schrieb Namjae Jeon: >>>> Use LOOKUP_NO_SYMLINKS flags for default lookup to prohibit the >>>> middle of symlink component lookup. >>> >>> maybe this patch should be squashed with the "ksmbd: remove follow >>> symlinks support" patch? >> >> also, I noticed that the patches are already included in ksmbd-for-next. >> Did I miss Steve's ack on the ML? >> >> I wonder why the patches are already included in ksmbd-for-next without >> a proper review, I just started to look at the patches and wanted to >> raise several issues. > > I included them at Namjae's request in for-next to allow the automated > tests to run on them (e.g. the Intel test robot etc.) - those > automated bots can be useful ... but I had done some review of all of > them, and detailed review of most, and had run the automated tests > (buildbot) on them (which passed, even after adding more subtests), > and the smbtorture tests were also automatically run (it is triggered > in Namjae's github setup). > > Of the 8 patches in for-next, these 3 are the remaining ones that I am > looking at in more detail now: > > 24f0f4fc5f76 ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup > 1ec1e6928354 ksmbd: add buffer validation for SMB2_CREATE_CONTEXT > e2cd5c814442 ksmbd: add validation in smb2_ioctl ok, thanks for explaining. To be honest, I'm still trying to make sense of the patch workflow. Hopefully I get there eventually. How can I detect if a patch is already reviewed and queued for upstrea merge, so it's "too late" for me to do a review? -slow
2021-09-21 0:38 GMT+09:00, Ralph Boehme <slow@samba.org>: > 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@gmail.com> >> Cc: Ralph Böhme <slow@samba.org> >> Cc: Steve French <smfrench@gmail.com> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >> --- >> 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 > >
On Mon, Sep 20, 2021 at 11:11 AM Ralph Boehme <slow@samba.org> wrote: > > Am 20.09.21 um 17:10 schrieb Steve French: > > On Mon, Sep 20, 2021 at 10:03 AM Ralph Boehme <slow@samba.org> wrote: > >> > >> Am 20.09.21 um 16:45 schrieb Ralph Boehme: > >>> Am 19.09.21 um 04:13 schrieb Namjae Jeon: > >>>> Use LOOKUP_NO_SYMLINKS flags for default lookup to prohibit the > >>>> middle of symlink component lookup. > >>> > >>> maybe this patch should be squashed with the "ksmbd: remove follow > >>> symlinks support" patch? > >> > >> also, I noticed that the patches are already included in ksmbd-for-next. > >> Did I miss Steve's ack on the ML? > >> > >> I wonder why the patches are already included in ksmbd-for-next without > >> a proper review, I just started to look at the patches and wanted to > >> raise several issues. > > > > I included them at Namjae's request in for-next to allow the automated > > tests to run on them (e.g. the Intel test robot etc.) - those > > automated bots can be useful ... but I had done some review of all of > > them, and detailed review of most, and had run the automated tests > > (buildbot) on them (which passed, even after adding more subtests), > > and the smbtorture tests were also automatically run (it is triggered > > in Namjae's github setup). > > > > Of the 8 patches in for-next, these 3 are the remaining ones that I am > > looking at in more detail now: > > > > 24f0f4fc5f76 ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup > > 1ec1e6928354 ksmbd: add buffer validation for SMB2_CREATE_CONTEXT > > e2cd5c814442 ksmbd: add validation in smb2_ioctl > > ok, thanks for explaining. > > To be honest, I'm still trying to make sense of the patch workflow. > Hopefully I get there eventually. > > How can I detect if a patch is already reviewed and queued for upstrea > merge, so it's "too late" for me to do a review? It is not too late to do review of any of these 8. Given the importance of security, the more reviews the better. Earliest we would send them (the larger set of 8) upstream would be in a few days. I typically like to have them sit in for-next for 48 hours (although in some cases make exceptions, e.g. for important bug fixes I will shorten this if later in the week so they make it in time for the next rc)
Am 20.09.21 um 18:20 schrieb Steve French: > It is not too late to do review of any of these 8. Given the > importance of security, the more reviews the better. Earliest we > would send them (the larger set of 8) upstream would be in a few days. > I typically like to have them sit in for-next for 48 hours (although > in some cases make exceptions, e.g. for important bug fixes I will > shorten this if later in the week so they make it in time for the next > rc) which "for-next"? git://git.samba.org/ksmbd.git/ksmbd-for-next ? Thanks! -slow
On 9/18/2021 10:13 PM, Namjae Jeon wrote: > Add buffer validation in smb2_set_info. > > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> > Cc: Ralph Böhme <slow@samba.org> > Cc: Steve French <smfrench@gmail.com> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > --- > 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; How certain is it that EaNameLength and EaValueLength are sane? One might imagine a forged packet with various combinations of invalid values, which arithmetically satisfy the above check... > + > 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; Because buf_len is unsigned int and next is signed int, these compares are risky. I think "next" should also be unsigned, but if not, testing it for negative values before these checks is important. In the many changes below, buf_len is passed in as signed. Those should be consistent with the same criteria. It pays to be paranoid everywhere! Tom. > 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); Again, is DataLength verified? > 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; > 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; > 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); > + } > 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); > + } > 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); > + } > 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); > + } > 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); > + } > 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); > + } > } > > - pr_err("Unimplemented Fileinfoclass :%d\n", info_class); > + pr_err("Unimplemented Fileinfoclass :%d\n", req->FileInfoClass); > return -EOPNOTSUPP; > } > > @@ -5855,8 +5919,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]; >
2021-09-21 23:23 GMT+09:00, Tom Talpey <tom@talpey.com>: > On 9/18/2021 10:13 PM, Namjae Jeon wrote: >> Add buffer validation in smb2_set_info. >> >> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> >> Cc: Ralph Böhme <slow@samba.org> >> Cc: Steve French <smfrench@gmail.com> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >> --- >> 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; > > How certain is it that EaNameLength and EaValueLength are sane? One > might imagine a forged packet with various combinations of invalid > values, which arithmetically satisfy the above check... Sorry, I didn't fully understand what you pointed out. Could you please elaborate more ? > >> + >> 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; > > Because buf_len is unsigned int and next is signed int, these compares > are risky. I think "next" should also be unsigned, but if not, testing > it for negative values before these checks is important. > > In the many changes below, buf_len is passed in as signed. Those should > be consistent with the same criteria. It pays to be paranoid everywhere! Okay, I will update it on next version. > > Tom. > >> 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); > > Again, is DataLength verified? This field is checked by create context validation patch. See: https://marc.info/?l=linux-cifs&m=163227401430586&w=2 > >> 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; >> 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; >> 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); >> + } >> 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); >> + } >> 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); >> + } >> 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); >> + } >> 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); >> + } >> 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); >> + } >> } >> >> - pr_err("Unimplemented Fileinfoclass :%d\n", info_class); >> + pr_err("Unimplemented Fileinfoclass :%d\n", req->FileInfoClass); >> return -EOPNOTSUPP; >> } >> >> @@ -5855,8 +5919,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]; >> >
2021-09-22 11:31 GMT+09:00, Namjae Jeon <linkinjeon@kernel.org>: > 2021-09-21 23:23 GMT+09:00, Tom Talpey <tom@talpey.com>: >> On 9/18/2021 10:13 PM, Namjae Jeon wrote: >>> Add buffer validation in smb2_set_info. >>> >>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> >>> Cc: Ralph Böhme <slow@samba.org> >>> Cc: Steve French <smfrench@gmail.com> >>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >>> --- >>> 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; >> >> How certain is it that EaNameLength and EaValueLength are sane? One >> might imagine a forged packet with various combinations of invalid >> values, which arithmetically satisfy the above check... > Sorry, I didn't fully understand what you pointed out. Could you > please elaborate more ? Maybe, You are saying we need the below check? @@ -2577,6 +2581,12 @@ int smb2_open(struct ksmbd_work *work) goto err_out1; } else if (context) { ea_buf = (struct create_ea_buf_req *)context; + if (le16_to_cpu(context->DataOffset) + + le32_to_cpu(context->DataLength) < + sizeof(struct create_ea_buf_req)) { + rc = -EINVAL; + goto err_out1; + } This check is in create context patch. (https://marc.info/?l=linux-cifs&m=163227401430586&w=2) > >> >>> + >>> 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; >> >> Because buf_len is unsigned int and next is signed int, these compares >> are risky. I think "next" should also be unsigned, but if not, testing >> it for negative values before these checks is important. >> >> In the many changes below, buf_len is passed in as signed. Those should >> be consistent with the same criteria. It pays to be paranoid everywhere! > Okay, I will update it on next version. >> >> Tom. >> >>> 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); >> >> Again, is DataLength verified? > This field is checked by create context validation patch. > See: https://marc.info/?l=linux-cifs&m=163227401430586&w=2 >> >>> 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; >>> 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; >>> 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); >>> + } >>> 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); >>> + } >>> 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); >>> + } >>> 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); >>> + } >>> 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); >>> + } >>> 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); >>> + } >>> } >>> >>> - pr_err("Unimplemented Fileinfoclass :%d\n", info_class); >>> + pr_err("Unimplemented Fileinfoclass :%d\n", req->FileInfoClass); >>> return -EOPNOTSUPP; >>> } >>> >>> @@ -5855,8 +5919,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]; >>> >> >
On 9/21/2021 11:40 PM, Namjae Jeon wrote: > 2021-09-22 11:31 GMT+09:00, Namjae Jeon <linkinjeon@kernel.org>: >> 2021-09-21 23:23 GMT+09:00, Tom Talpey <tom@talpey.com>: >>> On 9/18/2021 10:13 PM, Namjae Jeon wrote: >>>> Add buffer validation in smb2_set_info. >>>> >>>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> >>>> Cc: Ralph Böhme <slow@samba.org> >>>> Cc: Steve French <smfrench@gmail.com> >>>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >>>> --- >>>> 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; >>> >>> How certain is it that EaNameLength and EaValueLength are sane? One >>> might imagine a forged packet with various combinations of invalid >>> values, which arithmetically satisfy the above check... >> Sorry, I didn't fully understand what you pointed out. Could you >> please elaborate more ? > > Maybe, You are saying we need the below check? > @@ -2577,6 +2581,12 @@ int smb2_open(struct ksmbd_work *work) > goto err_out1; > } else if (context) { > ea_buf = (struct create_ea_buf_req *)context; > + if (le16_to_cpu(context->DataOffset) + > + le32_to_cpu(context->DataLength) < > + sizeof(struct create_ea_buf_req)) { > + rc = -EINVAL; > + goto err_out1; > + } > > This check is in create context patch. > (https://marc.info/?l=linux-cifs&m=163227401430586&w=2) Ah, yes something like that. I'll look over the other patch thanks. Tom. Tom.
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; 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; 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); + } 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); + } 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); + } 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); + } 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); + } 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); + } } - pr_err("Unimplemented Fileinfoclass :%d\n", info_class); + pr_err("Unimplemented Fileinfoclass :%d\n", req->FileInfoClass); return -EOPNOTSUPP; } @@ -5855,8 +5919,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];
Add buffer validation in smb2_set_info. Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> Cc: Ralph Böhme <slow@samba.org> Cc: Steve French <smfrench@gmail.com> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> --- fs/ksmbd/smb2pdu.c | 113 +++++++++++++++++++++++++++++++++++---------- fs/ksmbd/smb2pdu.h | 9 ++++ 2 files changed, 97 insertions(+), 25 deletions(-)