| Message ID | 20210920065613.5506-1-linkinjeon@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | ksmbd: remove follow symlinks support | expand |
Hi Namjae, Am 20.09.21 um 08:56 schrieb Namjae Jeon: > This patch remove symlink support that can be vulnerable, and we > re-implement it as reparse point later. > > 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 | 55 ++++++++++------------------------------------ > fs/ksmbd/vfs.c | 50 +++++++++-------------------------------- > 2 files changed, 21 insertions(+), 84 deletions(-) > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > index afc508c2656d..911c5e97678d 100644 > --- a/fs/ksmbd/smb2pdu.c > +++ b/fs/ksmbd/smb2pdu.c > @@ -2661,17 +2661,7 @@ int smb2_open(struct ksmbd_work *work) > } > > if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) { > - if (test_share_config_flag(work->tcon->share_conf, > - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) { > - /* > - * On delete request, instead of following up, need to > - * look the current entity > - */ > - rc = ksmbd_vfs_kern_path(name, 0, &path, 1); > - } else { > - rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1); > - } > - > + rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1); > if (!rc) { > /* > * If file exists with under flags, return access > @@ -2693,24 +2683,11 @@ int smb2_open(struct ksmbd_work *work) > } > } > } else { > - if (test_share_config_flag(work->tcon->share_conf, > - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) { > - /* > - * Use LOOKUP_FOLLOW to follow the path of > - * symlink in path buildup > - */ > - rc = ksmbd_vfs_kern_path(name, LOOKUP_FOLLOW, &path, 1); > - if (rc) { /* Case for broken link ?*/ > - rc = ksmbd_vfs_kern_path(name, 0, &path, 1); > - } > - } else { > - rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, > - &path, 1); > - if (!rc && d_is_symlink(path.dentry)) { > - rc = -EACCES; > - path_put(&path); > - goto err_out; > - } > + rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1); > + if (!rc && d_is_symlink(path.dentry)) { > + rc = -EACCES; > + path_put(&path); > + goto err_out; why the the check for d_is_symlink()? Wouldn't ksmbd_vfs_kern_path() just return -ELOOP if a path component is a symlink? Hence I guess the below if (rc) where we handle EACCESS should be expanded to handle ELOOP. I guess I would also refactor the if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) logic in a previous commit to change the codeflow so there's only one call to ksmbd_vfs_kern_path(). > } > } > > @@ -4795,12 +4772,8 @@ static int smb2_get_info_filesystem(struct ksmbd_work *work, > struct path path; > int rc = 0, len; > int fs_infoclass_size = 0; > - int lookup_flags = LOOKUP_NO_SYMLINKS; > > - if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) > - lookup_flags = LOOKUP_FOLLOW; > - > - rc = ksmbd_vfs_kern_path(share->path, lookup_flags, &path, 0); > + rc = ksmbd_vfs_kern_path(share->path, LOOKUP_NO_SYMLINKS, &path, 0); > if (rc) { > pr_err("cannot create vfs path\n"); > return -EIO; why doesn't this return rc? > @@ -5307,7 +5280,7 @@ static int smb2_rename(struct ksmbd_work *work, > char *pathname = NULL; > struct path path; > bool file_present = true; > - int rc, lookup_flags = LOOKUP_NO_SYMLINKS; > + int rc; > > ksmbd_debug(SMB, "setting FILE_RENAME_INFO\n"); > pathname = kmalloc(PATH_MAX, GFP_KERNEL); > @@ -5376,11 +5349,8 @@ static int smb2_rename(struct ksmbd_work *work, > goto out; > } > > - if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) > - lookup_flags = LOOKUP_FOLLOW; > - > ksmbd_debug(SMB, "new name %s\n", new_name); > - rc = ksmbd_vfs_kern_path(new_name, lookup_flags, &path, 1); > + rc = ksmbd_vfs_kern_path(new_name, LOOKUP_NO_SYMLINKS, &path, 1); > if (rc) > file_present = false; I guess this should handle ELOOP? > else > @@ -5430,7 +5400,7 @@ static int smb2_create_link(struct ksmbd_work *work, > char *link_name = NULL, *target_name = NULL, *pathname = NULL; > struct path path; > bool file_present = true; > - int rc, lookup_flags = LOOKUP_NO_SYMLINKS; > + int rc; > > if (buf_len < sizeof(struct smb2_file_link_info) + > le32_to_cpu(file_info->FileNameLength)) > @@ -5457,11 +5427,8 @@ static int smb2_create_link(struct ksmbd_work *work, > goto out; > } > > - if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) > - lookup_flags = LOOKUP_FOLLOW; > - > ksmbd_debug(SMB, "target name is %s\n", target_name); > - rc = ksmbd_vfs_kern_path(link_name, lookup_flags, &path, 0); > + rc = ksmbd_vfs_kern_path(link_name, LOOKUP_NO_SYMLINKS, &path, 0); > if (rc) > file_present = false; same here? Other then that: lgtm. Oh, besides, guess this needs an accomanying change to ksmbd-tools? Cheers! -slow
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
On Mon, Sep 20, 2021 at 9:44 AM Ralph Boehme <slow@samba.org> wrote: > > 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? These two could be merged if it makes review easier or less likely to cause merge conflicts later (in this case that may be true since they both touch smb2_open), but that assumes that removing ability to follow all symlinks is agreed upon. Removing the ability to follow symlinks may be preferable, but I can imagine cases where the admin is exporting only via SMB3 or only read only where symlinks could be of value inside a share and safe (if remote users can't create symlinks outside the share). I don't have a strong opinion but also can imagine cases where symlinks could be required (share exported by both nfs and smb3 e.g.) but obviously checked to avoid escaping from the share.
Am 20.09.21 um 17:19 schrieb Steve French: > On Mon, Sep 20, 2021 at 9:44 AM Ralph Boehme <slow@samba.org> wrote: >> >> 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? > > These two could be merged if it makes review easier or less likely > to cause merge conflicts later (in this case that may be true since > they both touch smb2_open), from a high level perspective having both patches in the history is at least confusing and should be avoided. The first patch changes the semantics of "follow symlinks" and the second one then changes it again by basically removing the option, enforcing "never follow symlinks" behaviour. > but that assumes that removing ability to follow all symlinks is > agreed upon. Well, as discussed you could use LOOKUP_BENEATH. The only downside would be that symlinks with absolute paths are not allowed. Note that with the current WIP patches we either a) don't support symlink at all ("follow symlinks = yes") b) have no protection against follow symlinks outside of a configured share ("follow symlinks = no") -slow
2021-09-20 22:57 GMT+09:00, Ralph Boehme <slow@samba.org>: > Hi Namjae, > > Am 20.09.21 um 08:56 schrieb Namjae Jeon: >> This patch remove symlink support that can be vulnerable, and we >> re-implement it as reparse point later. >> >> 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 | 55 ++++++++++------------------------------------ >> fs/ksmbd/vfs.c | 50 +++++++++-------------------------------- >> 2 files changed, 21 insertions(+), 84 deletions(-) >> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> index afc508c2656d..911c5e97678d 100644 >> --- a/fs/ksmbd/smb2pdu.c >> +++ b/fs/ksmbd/smb2pdu.c >> @@ -2661,17 +2661,7 @@ int smb2_open(struct ksmbd_work *work) >> } >> >> if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) { >> - if (test_share_config_flag(work->tcon->share_conf, >> - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) { >> - /* >> - * On delete request, instead of following up, need to >> - * look the current entity >> - */ >> - rc = ksmbd_vfs_kern_path(name, 0, &path, 1); >> - } else { >> - rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1); >> - } >> - >> + rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1); >> if (!rc) { >> /* >> * If file exists with under flags, return access >> @@ -2693,24 +2683,11 @@ int smb2_open(struct ksmbd_work *work) >> } >> } >> } else { >> - if (test_share_config_flag(work->tcon->share_conf, >> - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) { >> - /* >> - * Use LOOKUP_FOLLOW to follow the path of >> - * symlink in path buildup >> - */ >> - rc = ksmbd_vfs_kern_path(name, LOOKUP_FOLLOW, &path, 1); >> - if (rc) { /* Case for broken link ?*/ >> - rc = ksmbd_vfs_kern_path(name, 0, &path, 1); >> - } >> - } else { >> - rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, >> - &path, 1); >> - if (!rc && d_is_symlink(path.dentry)) { >> - rc = -EACCES; >> - path_put(&path); >> - goto err_out; >> - } >> + rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1); >> + if (!rc && d_is_symlink(path.dentry)) { >> + rc = -EACCES; >> + path_put(&path); >> + goto err_out; > > why the the check for d_is_symlink()? Wouldn't ksmbd_vfs_kern_path() > just return -ELOOP if a path component is a symlink? Hence I guess the > below if (rc) where we handle EACCESS should be expanded to handle ELOOP. ksmbd_vfs_kern_path() doesn't return -ELOOP if last component is a symlink. So need to check it using d_is_symlink(). > > I guess I would also refactor the > > if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) > > logic in a previous commit to change the codeflow so there's only one > call to ksmbd_vfs_kern_path(). Right. Will do it on v2. > >> } >> } >> >> @@ -4795,12 +4772,8 @@ static int smb2_get_info_filesystem(struct >> ksmbd_work *work, >> struct path path; >> int rc = 0, len; >> int fs_infoclass_size = 0; >> - int lookup_flags = LOOKUP_NO_SYMLINKS; >> >> - if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) >> - lookup_flags = LOOKUP_FOLLOW; >> - >> - rc = ksmbd_vfs_kern_path(share->path, lookup_flags, &path, 0); >> + rc = ksmbd_vfs_kern_path(share->path, LOOKUP_NO_SYMLINKS, &path, 0); >> if (rc) { >> pr_err("cannot create vfs path\n"); >> return -EIO; > > why doesn't this return rc? This is not related with this patch. If needed, we can fix it on another patch. As I remember, To return STATUS_UNEXPECTED_IO_ERROR error? > >> @@ -5307,7 +5280,7 @@ static int smb2_rename(struct ksmbd_work *work, >> char *pathname = NULL; >> struct path path; >> bool file_present = true; >> - int rc, lookup_flags = LOOKUP_NO_SYMLINKS; >> + int rc; >> >> ksmbd_debug(SMB, "setting FILE_RENAME_INFO\n"); >> pathname = kmalloc(PATH_MAX, GFP_KERNEL); >> @@ -5376,11 +5349,8 @@ static int smb2_rename(struct ksmbd_work *work, >> goto out; >> } >> >> - if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) >> - lookup_flags = LOOKUP_FOLLOW; >> - >> ksmbd_debug(SMB, "new name %s\n", new_name); >> - rc = ksmbd_vfs_kern_path(new_name, lookup_flags, &path, 1); >> + rc = ksmbd_vfs_kern_path(new_name, LOOKUP_NO_SYMLINKS, &path, 1); >> if (rc) >> file_present = false; > > I guess this should handle ELOOP? I have answered above. ksmbd_vfs_kern_path doesn't return it. > >> else >> @@ -5430,7 +5400,7 @@ static int smb2_create_link(struct ksmbd_work >> *work, >> char *link_name = NULL, *target_name = NULL, *pathname = NULL; >> struct path path; >> bool file_present = true; >> - int rc, lookup_flags = LOOKUP_NO_SYMLINKS; >> + int rc; >> >> if (buf_len < sizeof(struct smb2_file_link_info) + >> le32_to_cpu(file_info->FileNameLength)) >> @@ -5457,11 +5427,8 @@ static int smb2_create_link(struct ksmbd_work >> *work, >> goto out; >> } >> >> - if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) >> - lookup_flags = LOOKUP_FOLLOW; >> - >> ksmbd_debug(SMB, "target name is %s\n", target_name); >> - rc = ksmbd_vfs_kern_path(link_name, lookup_flags, &path, 0); >> + rc = ksmbd_vfs_kern_path(link_name, LOOKUP_NO_SYMLINKS, &path, 0); >> if (rc) >> file_present = false; > > same here? ditto. > > Other then that: lgtm. Oh, besides, guess this needs an accomanying > change to ksmbd-tools? Yes. It is needed, but I will change ksmbd-tools also after this patch is applied. Thanks for your review! > > Cheers! > -slow > > -- > Ralph Boehme, Samba Team https://samba.org/ > SerNet Samba Team Lead https://sernet.de/en/team-samba > >
2021-09-20 23:44 GMT+09:00, Ralph Boehme <slow@samba.org>: > 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? I think that the subject is slightly different. 1. prohibit the middle of symlinks component on path. 2. remove LOOKUP_FOLLOW and "follow symlink" parameter check. Thanks! > > -slow > > -- > Ralph Boehme, Samba Team https://samba.org/ > SerNet Samba Team Lead https://sernet.de/en/team-samba > >
Am 20.09.21 um 17:57 schrieb Namjae Jeon: > ksmbd_vfs_kern_path() doesn't return -ELOOP if last component is a > symlink. So need to check it using d_is_symlink(). Really? I missed that. Is that a behaviour of kern_path() when passing LOOKUP_NO_SYMLINKS? I don't see the behaviour expressed inside ksmbd_vfs_kern_path(), but maybe I'm looking at the wrong branch+patchset? -slow
2021-09-21 1:28 GMT+09:00, Ralph Boehme <slow@samba.org>: > Am 20.09.21 um 17:57 schrieb Namjae Jeon: >> ksmbd_vfs_kern_path() doesn't return -ELOOP if last component is a >> symlink. So need to check it using d_is_symlink(). > > Really? I missed that. Is that a behaviour of kern_path() when passing > LOOKUP_NO_SYMLINKS? Yes. > I don't see the behaviour expressed inside > ksmbd_vfs_kern_path(), but maybe I'm looking at the wrong branch+patchset? I think that you checked correct branch and patch-set. > > -slow > > -- > Ralph Boehme, Samba Team https://samba.org/ > SerNet Samba Team Lead https://sernet.de/en/team-samba > >
Am 20.09.21 um 18:37 schrieb Namjae Jeon: > 2021-09-21 1:28 GMT+09:00, Ralph Boehme <slow@samba.org>: >> Am 20.09.21 um 17:57 schrieb Namjae Jeon: >>> ksmbd_vfs_kern_path() doesn't return -ELOOP if last component is a >>> symlink. So need to check it using d_is_symlink(). >> >> Really? I missed that. Is that a behaviour of kern_path() when passing >> LOOKUP_NO_SYMLINKS? > Yes. d'oh! Got it. We're yet to make use of those flags in Samba, so I hadn't really wrapped my head fully around this to understand how it works in detail, so I messed up the semantics a bit. :) -slow
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index afc508c2656d..911c5e97678d 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -2661,17 +2661,7 @@ int smb2_open(struct ksmbd_work *work) } if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) { - if (test_share_config_flag(work->tcon->share_conf, - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) { - /* - * On delete request, instead of following up, need to - * look the current entity - */ - rc = ksmbd_vfs_kern_path(name, 0, &path, 1); - } else { - rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1); - } - + rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1); if (!rc) { /* * If file exists with under flags, return access @@ -2693,24 +2683,11 @@ int smb2_open(struct ksmbd_work *work) } } } else { - if (test_share_config_flag(work->tcon->share_conf, - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) { - /* - * Use LOOKUP_FOLLOW to follow the path of - * symlink in path buildup - */ - rc = ksmbd_vfs_kern_path(name, LOOKUP_FOLLOW, &path, 1); - if (rc) { /* Case for broken link ?*/ - rc = ksmbd_vfs_kern_path(name, 0, &path, 1); - } - } else { - rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, - &path, 1); - if (!rc && d_is_symlink(path.dentry)) { - rc = -EACCES; - path_put(&path); - goto err_out; - } + rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1); + if (!rc && d_is_symlink(path.dentry)) { + rc = -EACCES; + path_put(&path); + goto err_out; } } @@ -4795,12 +4772,8 @@ static int smb2_get_info_filesystem(struct ksmbd_work *work, struct path path; int rc = 0, len; int fs_infoclass_size = 0; - int lookup_flags = LOOKUP_NO_SYMLINKS; - if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) - lookup_flags = LOOKUP_FOLLOW; - - rc = ksmbd_vfs_kern_path(share->path, lookup_flags, &path, 0); + rc = ksmbd_vfs_kern_path(share->path, LOOKUP_NO_SYMLINKS, &path, 0); if (rc) { pr_err("cannot create vfs path\n"); return -EIO; @@ -5307,7 +5280,7 @@ static int smb2_rename(struct ksmbd_work *work, char *pathname = NULL; struct path path; bool file_present = true; - int rc, lookup_flags = LOOKUP_NO_SYMLINKS; + int rc; ksmbd_debug(SMB, "setting FILE_RENAME_INFO\n"); pathname = kmalloc(PATH_MAX, GFP_KERNEL); @@ -5376,11 +5349,8 @@ static int smb2_rename(struct ksmbd_work *work, goto out; } - if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) - lookup_flags = LOOKUP_FOLLOW; - ksmbd_debug(SMB, "new name %s\n", new_name); - rc = ksmbd_vfs_kern_path(new_name, lookup_flags, &path, 1); + rc = ksmbd_vfs_kern_path(new_name, LOOKUP_NO_SYMLINKS, &path, 1); if (rc) file_present = false; else @@ -5430,7 +5400,7 @@ static int smb2_create_link(struct ksmbd_work *work, char *link_name = NULL, *target_name = NULL, *pathname = NULL; struct path path; bool file_present = true; - int rc, lookup_flags = LOOKUP_NO_SYMLINKS; + int rc; if (buf_len < sizeof(struct smb2_file_link_info) + le32_to_cpu(file_info->FileNameLength)) @@ -5457,11 +5427,8 @@ static int smb2_create_link(struct ksmbd_work *work, goto out; } - if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) - lookup_flags = LOOKUP_FOLLOW; - ksmbd_debug(SMB, "target name is %s\n", target_name); - rc = ksmbd_vfs_kern_path(link_name, lookup_flags, &path, 0); + rc = ksmbd_vfs_kern_path(link_name, LOOKUP_NO_SYMLINKS, &path, 0); if (rc) file_present = false; else diff --git a/fs/ksmbd/vfs.c b/fs/ksmbd/vfs.c index 53047f013371..3733e4944c1d 100644 --- a/fs/ksmbd/vfs.c +++ b/fs/ksmbd/vfs.c @@ -164,13 +164,9 @@ int ksmbd_vfs_create(struct ksmbd_work *work, const char *name, umode_t mode) { struct path path; struct dentry *dentry; - int err, lookup_flags = LOOKUP_NO_SYMLINKS; - - if (test_share_config_flag(work->tcon->share_conf, - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) - lookup_flags = LOOKUP_FOLLOW; + int err; - dentry = kern_path_create(AT_FDCWD, name, &path, lookup_flags); + dentry = kern_path_create(AT_FDCWD, name, &path, LOOKUP_NO_SYMLINKS); if (IS_ERR(dentry)) { err = PTR_ERR(dentry); if (err != -ENOENT) @@ -205,14 +201,10 @@ int ksmbd_vfs_mkdir(struct ksmbd_work *work, const char *name, umode_t mode) struct user_namespace *user_ns; struct path path; struct dentry *dentry; - int err, lookup_flags = LOOKUP_NO_SYMLINKS; - - if (test_share_config_flag(work->tcon->share_conf, - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) - lookup_flags = LOOKUP_FOLLOW; + int err; dentry = kern_path_create(AT_FDCWD, name, &path, - lookup_flags | LOOKUP_DIRECTORY); + LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY); if (IS_ERR(dentry)) { err = PTR_ERR(dentry); if (err != -EEXIST) @@ -597,16 +589,11 @@ int ksmbd_vfs_remove_file(struct ksmbd_work *work, char *name) struct path path; struct dentry *parent; int err; - int flags = LOOKUP_NO_SYMLINKS; if (ksmbd_override_fsids(work)) return -ENOMEM; - if (test_share_config_flag(work->tcon->share_conf, - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) - flags = LOOKUP_FOLLOW; - - err = kern_path(name, flags, &path); + err = kern_path(name, LOOKUP_NO_SYMLINKS, &path); if (err) { ksmbd_debug(VFS, "can't get %s, err %d\n", name, err); ksmbd_revert_fsids(work); @@ -661,16 +648,11 @@ int ksmbd_vfs_link(struct ksmbd_work *work, const char *oldname, struct path oldpath, newpath; struct dentry *dentry; int err; - int flags = LOOKUP_NO_SYMLINKS; if (ksmbd_override_fsids(work)) return -ENOMEM; - if (test_share_config_flag(work->tcon->share_conf, - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) - flags = LOOKUP_FOLLOW; - - err = kern_path(oldname, flags, &oldpath); + err = kern_path(oldname, LOOKUP_NO_SYMLINKS, &oldpath); if (err) { pr_err("cannot get linux path for %s, err = %d\n", oldname, err); @@ -678,7 +660,7 @@ int ksmbd_vfs_link(struct ksmbd_work *work, const char *oldname, } dentry = kern_path_create(AT_FDCWD, newname, &newpath, - flags | LOOKUP_REVAL); + LOOKUP_NO_SYMLINKS | LOOKUP_REVAL); if (IS_ERR(dentry)) { err = PTR_ERR(dentry); pr_err("path create err for %s, err %d\n", newname, err); @@ -797,7 +779,6 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp, struct dentry *src_dent, *trap_dent, *src_child; char *dst_name; int err; - int flags = LOOKUP_NO_SYMLINKS; dst_name = extract_last_component(newname); if (!dst_name) @@ -806,13 +787,8 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp, src_dent_parent = dget_parent(fp->filp->f_path.dentry); src_dent = fp->filp->f_path.dentry; - flags = LOOKUP_DIRECTORY; - if (test_share_config_flag(work->tcon->share_conf, - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) - flags = LOOKUP_FOLLOW; - flags |= LOOKUP_DIRECTORY; - - err = kern_path(newname, flags, &dst_path); + err = kern_path(newname, LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY, + &dst_path); if (err) { ksmbd_debug(VFS, "Cannot get path for %s [%d]\n", newname, err); goto out; @@ -871,13 +847,7 @@ int ksmbd_vfs_truncate(struct ksmbd_work *work, const char *name, int err = 0; if (name) { - int flags = LOOKUP_NO_SYMLINKS; - - if (test_share_config_flag(work->tcon->share_conf, - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) - flags = LOOKUP_FOLLOW; - - err = kern_path(name, flags, &path); + err = kern_path(name, LOOKUP_NO_SYMLINKS, &path); if (err) { pr_err("cannot get linux path for %s, err %d\n", name, err);
This patch remove symlink support that can be vulnerable, and we re-implement it as reparse point later. 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 | 55 ++++++++++------------------------------------ fs/ksmbd/vfs.c | 50 +++++++++-------------------------------- 2 files changed, 21 insertions(+), 84 deletions(-)