| Message ID | 20210921044040.624769-1-linkinjeon@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ksmbd: remove follow symlinks support | expand |
On Tue, Sep 21, 2021 at 2:41 PM Namjae Jeon <linkinjeon@kernel.org> wrote: > > This patch remove symlink support that can be vulnerable and access out > of share, and we re-implement it as reparse point later. Very good. Acked-by: Ronnie Sahlberg <lsahlber@redhat.com> > > > 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> > --- > v2: > - reorganize path lookup in smb2_open to call only one > ksmbd_vfs_kern_path(). > > fs/ksmbd/smb2pdu.c | 60 ++++++++++------------------------------------ > fs/ksmbd/vfs.c | 50 ++++++++------------------------------ > 2 files changed, 22 insertions(+), 88 deletions(-) > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > index e08c6b26b6f0..de044802fc5b 100644 > --- a/fs/ksmbd/smb2pdu.c > +++ b/fs/ksmbd/smb2pdu.c > @@ -2648,19 +2648,9 @@ int smb2_open(struct ksmbd_work *work) > goto err_out1; > } > > - 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); > - } > - > - if (!rc) { > + rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1); > + if (!rc) { > + if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) { > /* > * If file exists with under flags, return access > * denied error. > @@ -2679,26 +2669,10 @@ int smb2_open(struct ksmbd_work *work) > path_put(&path); > goto err_out; > } > - } > - } 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; > - } > + } else if (d_is_symlink(path.dentry)) { > + rc = -EACCES; > + path_put(&path); > + goto err_out; > } > } > > @@ -4781,12 +4755,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; > @@ -5293,7 +5263,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); > @@ -5362,11 +5332,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 > @@ -5416,7 +5383,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; > > ksmbd_debug(SMB, "setting FILE_LINK_INFORMATION\n"); > pathname = kmalloc(PATH_MAX, GFP_KERNEL); > @@ -5439,11 +5406,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); > -- > 2.25.1 >
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index e08c6b26b6f0..de044802fc5b 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -2648,19 +2648,9 @@ int smb2_open(struct ksmbd_work *work) goto err_out1; } - 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); - } - - if (!rc) { + rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1); + if (!rc) { + if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) { /* * If file exists with under flags, return access * denied error. @@ -2679,26 +2669,10 @@ int smb2_open(struct ksmbd_work *work) path_put(&path); goto err_out; } - } - } 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; - } + } else if (d_is_symlink(path.dentry)) { + rc = -EACCES; + path_put(&path); + goto err_out; } } @@ -4781,12 +4755,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; @@ -5293,7 +5263,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); @@ -5362,11 +5332,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 @@ -5416,7 +5383,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; ksmbd_debug(SMB, "setting FILE_LINK_INFORMATION\n"); pathname = kmalloc(PATH_MAX, GFP_KERNEL); @@ -5439,11 +5406,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 access out of share, 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> --- v2: - reorganize path lookup in smb2_open to call only one ksmbd_vfs_kern_path(). fs/ksmbd/smb2pdu.c | 60 ++++++++++------------------------------------ fs/ksmbd/vfs.c | 50 ++++++++------------------------------ 2 files changed, 22 insertions(+), 88 deletions(-)