* [PATCH] ksmbd: remove follow symlinks support
@ 2021-09-20 6:56 Namjae Jeon
2021-09-20 13:57 ` Ralph Boehme
2021-09-20 14:44 ` Ralph Boehme
0 siblings, 2 replies; 10+ messages in thread
From: Namjae Jeon @ 2021-09-20 6:56 UTC (permalink / raw)
To: linux-cifs; +Cc: Namjae Jeon, Ronnie Sahlberg, Ralph Böhme, Steve French
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;
}
}
@@ -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);
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] ksmbd: remove follow symlinks support
2021-09-20 6:56 [PATCH] ksmbd: remove follow symlinks support Namjae Jeon
@ 2021-09-20 13:57 ` Ralph Boehme
2021-09-20 15:57 ` Namjae Jeon
2021-09-20 14:44 ` Ralph Boehme
1 sibling, 1 reply; 10+ messages in thread
From: Ralph Boehme @ 2021-09-20 13:57 UTC (permalink / raw)
To: Namjae Jeon, linux-cifs; +Cc: Ronnie Sahlberg, Steve French
[-- Attachment #1.1: Type: text/plain, Size: 5224 bytes --]
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
--
Ralph Boehme, Samba Team https://samba.org/
SerNet Samba Team Lead https://sernet.de/en/team-samba
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ksmbd: remove follow symlinks support
2021-09-20 13:57 ` Ralph Boehme
@ 2021-09-20 15:57 ` Namjae Jeon
2021-09-20 16:28 ` Ralph Boehme
0 siblings, 1 reply; 10+ messages in thread
From: Namjae Jeon @ 2021-09-20 15:57 UTC (permalink / raw)
To: Ralph Boehme; +Cc: linux-cifs, Ronnie Sahlberg, Steve French
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
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ksmbd: remove follow symlinks support
2021-09-20 15:57 ` Namjae Jeon
@ 2021-09-20 16:28 ` Ralph Boehme
2021-09-20 16:37 ` Namjae Jeon
0 siblings, 1 reply; 10+ messages in thread
From: Ralph Boehme @ 2021-09-20 16:28 UTC (permalink / raw)
To: Namjae Jeon; +Cc: linux-cifs, Ronnie Sahlberg, Steve French
[-- Attachment #1.1: Type: text/plain, Size: 523 bytes --]
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
--
Ralph Boehme, Samba Team https://samba.org/
SerNet Samba Team Lead https://sernet.de/en/team-samba
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ksmbd: remove follow symlinks support
2021-09-20 16:28 ` Ralph Boehme
@ 2021-09-20 16:37 ` Namjae Jeon
2021-09-21 7:44 ` Ralph Boehme
0 siblings, 1 reply; 10+ messages in thread
From: Namjae Jeon @ 2021-09-20 16:37 UTC (permalink / raw)
To: Ralph Boehme; +Cc: linux-cifs, Ronnie Sahlberg, Steve French
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
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ksmbd: remove follow symlinks support
2021-09-20 6:56 [PATCH] ksmbd: remove follow symlinks support Namjae Jeon
2021-09-20 13:57 ` Ralph Boehme
@ 2021-09-20 14:44 ` Ralph Boehme
2021-09-20 15:19 ` Steve French
2021-09-20 16:03 ` Namjae Jeon
1 sibling, 2 replies; 10+ messages in thread
From: Ralph Boehme @ 2021-09-20 14:44 UTC (permalink / raw)
To: Namjae Jeon, linux-cifs; +Cc: Ronnie Sahlberg, Steve French
[-- Attachment #1.1: Type: text/plain, Size: 386 bytes --]
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
--
Ralph Boehme, Samba Team https://samba.org/
SerNet Samba Team Lead https://sernet.de/en/team-samba
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ksmbd: remove follow symlinks support
2021-09-20 14:44 ` Ralph Boehme
@ 2021-09-20 15:19 ` Steve French
2021-09-20 15:55 ` Ralph Boehme
2021-09-20 16:03 ` Namjae Jeon
1 sibling, 1 reply; 10+ messages in thread
From: Steve French @ 2021-09-20 15:19 UTC (permalink / raw)
To: Ralph Boehme; +Cc: Namjae Jeon, CIFS, Ronnie Sahlberg
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.
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ksmbd: remove follow symlinks support
2021-09-20 15:19 ` Steve French
@ 2021-09-20 15:55 ` Ralph Boehme
0 siblings, 0 replies; 10+ messages in thread
From: Ralph Boehme @ 2021-09-20 15:55 UTC (permalink / raw)
To: Steve French; +Cc: Namjae Jeon, CIFS, Ronnie Sahlberg
[-- Attachment #1.1: Type: text/plain, Size: 1434 bytes --]
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
--
Ralph Boehme, Samba Team https://samba.org/
SerNet Samba Team Lead https://sernet.de/en/team-samba
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ksmbd: remove follow symlinks support
2021-09-20 14:44 ` Ralph Boehme
2021-09-20 15:19 ` Steve French
@ 2021-09-20 16:03 ` Namjae Jeon
1 sibling, 0 replies; 10+ messages in thread
From: Namjae Jeon @ 2021-09-20 16:03 UTC (permalink / raw)
To: Ralph Boehme; +Cc: linux-cifs, Ronnie Sahlberg, Steve French
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
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-09-21 7:44 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-20 6:56 [PATCH] ksmbd: remove follow symlinks support Namjae Jeon
2021-09-20 13:57 ` Ralph Boehme
2021-09-20 15:57 ` Namjae Jeon
2021-09-20 16:28 ` Ralph Boehme
2021-09-20 16:37 ` Namjae Jeon
2021-09-21 7:44 ` Ralph Boehme
2021-09-20 14:44 ` Ralph Boehme
2021-09-20 15:19 ` Steve French
2021-09-20 15:55 ` Ralph Boehme
2021-09-20 16:03 ` Namjae Jeon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).