Re: [PATCH v3] ksmbd: remove follow symlinks support

[Ralph Boehme <slow@samba.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3344 bytes --]

Hi Namjae,

excellent! One issue below.

Am 21.09.21 um 07:19 schrieb Namjae Jeon:
> Use  LOOKUP_NO_SYMLINKS flags for default lookup to prohibit the middle of
> symlink component lookup and remove follow symlinks parameter support.
> We re-implement it as reparse point later.
> 
> Test result:
> smbclient -Ulinkinjeon%1234 //172.30.1.42/share -c
> "get hacked/passwd passwd"
> NT_STATUS_OBJECT_NAME_NOT_FOUND opening remote file \hacked\passwd
> 
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Acked-by: Ronnie Sahlberg <lsahlber@redhat.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>   v2:
>    - reorganize path lookup in smb2_open to call only one
>      ksmbd_vfs_kern_path().
>   v3:
>    - combine "ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup"
>      patch into this patch.
> 
>   fs/ksmbd/smb2pdu.c | 43 ++++++++++---------------------------------
>   fs/ksmbd/vfs.c     | 32 +++++++++-----------------------
>   2 files changed, 19 insertions(+), 56 deletions(-)
> 
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 4399c399284b..baf7ce31d557 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -2660,13 +2660,9 @@ int smb2_open(struct ksmbd_work *work)
>   		goto err_out1;
>   	}
>   
> -	if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) {
> -		/*
> -		 * On delete request, instead of following up, need to
> -		 * look the current entity
> -		 */
> -		rc = ksmbd_vfs_kern_path(name, 0, &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.
> @@ -2685,25 +2681,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, 0, &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;

I wonder whether the d_is_symlink() check should be done *inside* 
ksmbd_vfs_kern_path()? The idea being having one function where the 
required semantics are implemented without bleeding logic stuff in the 
callers. ksmbd_vfs_kern_path() could simply return -ELOOP if *any* path 
component is a symlink.

Then of course the question is how to handle this in some callers to 
make the decision what how to present the directory entry to the caller.

For example in ksmbd_vfs_readdir_name() I'm not sure if we return file 
metadata from the link target.

Thanks!
-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 --]