On Tue, Sep 21, 2021 at 2:41 PM Namjae Jeon <linkinjeon@xxxxxxxxxx> 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@xxxxxxxxxx>
>
>
> Cc: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
> Cc: Ralph Böhme <slow@xxxxxxxxx>
> Cc: Steve French <smfrench@xxxxxxxxx>
> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
> ---
> 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
>
