All of lore.kernel.org
 help / color / mirror / Atom feed
From: Namjae Jeon <linkinjeon@kernel.org>
To: Ralph Boehme <slow@samba.org>
Cc: linux-cifs@vger.kernel.org,
	Ronnie Sahlberg <ronniesahlberg@gmail.com>,
	Steve French <smfrench@gmail.com>
Subject: Re: [PATCH] ksmbd: remove follow symlinks support
Date: Tue, 21 Sep 2021 00:57:11 +0900	[thread overview]
Message-ID: <CAKYAXd9Re_iHpwKq4t4GUibKo8g_D3QB1rzBOYiTvv5dLhdvsw@mail.gmail.com> (raw)
In-Reply-To: <6e8b6c79-3394-f39c-8e1b-06b3c2950a28@samba.org>

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
>
>

  reply	other threads:[~2021-09-20 15:57 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKYAXd9Re_iHpwKq4t4GUibKo8g_D3QB1rzBOYiTvv5dLhdvsw@mail.gmail.com \
    --to=linkinjeon@kernel.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=ronniesahlberg@gmail.com \
    --cc=slow@samba.org \
    --cc=smfrench@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.