2021-09-20 22:57 GMT+09:00, Ralph Boehme <slow@xxxxxxxxx>:
> 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@xxxxxxxxx>
>> Cc: Ralph Böhme <slow@xxxxxxxxx>
>> Cc: Steve French <smfrench@xxxxxxxxx>
>> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
>> ---
>> 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
>
>
