diff mbox series

cifs: avoid dup prefix path in dfs_get_automount_devname()

Message ID 20230416183828.18174-1-pc@manguebit.com (mailing list archive)
State New, archived
Headers show
Series cifs: avoid dup prefix path in dfs_get_automount_devname() | expand

Commit Message

Paulo Alcantara April 16, 2023, 6:38 p.m. UTC
@server->origin_fullpath already contains the tree name + optional
prefix, so avoid calling __build_path_from_dentry_optional_prefix() as
it might end up duplicating prefix path from @cifs_sb->prepath into
final full path.

Instead, generate DFS full path by simply merging
@server->origin_fullpath with dentry's path.

This fixes the following case

	mount.cifs //root/dfs/dir /mnt/ -o ...
	ls /mnt/link

where cifs_dfs_do_automount() will call smb3_parse_devname() with
@devname set to "//root/dfs/dir/link" instead of
"//root/dfs/dir/dir/link".

Fixes: 7ad54b98fc1f ("cifs: use origin fullpath for automounts")
Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
---
FYI, updated DFS tests to include the above case and avoid regressions.

 fs/cifs/cifs_dfs_ref.c |  2 --
 fs/cifs/dfs.h          | 22 ++++++++++++++++++----
 2 files changed, 18 insertions(+), 6 deletions(-)

Comments

Steve French April 16, 2023, 7:33 p.m. UTC | #1
tentatively merged into cifs-2.6.git for-next pending additional
review and testing

On Sun, Apr 16, 2023 at 1:38 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> @server->origin_fullpath already contains the tree name + optional
> prefix, so avoid calling __build_path_from_dentry_optional_prefix() as
> it might end up duplicating prefix path from @cifs_sb->prepath into
> final full path.
>
> Instead, generate DFS full path by simply merging
> @server->origin_fullpath with dentry's path.
>
> This fixes the following case
>
>         mount.cifs //root/dfs/dir /mnt/ -o ...
>         ls /mnt/link
>
> where cifs_dfs_do_automount() will call smb3_parse_devname() with
> @devname set to "//root/dfs/dir/link" instead of
> "//root/dfs/dir/dir/link".
>
> Fixes: 7ad54b98fc1f ("cifs: use origin fullpath for automounts")
> Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
> ---
> FYI, updated DFS tests to include the above case and avoid regressions.
>
>  fs/cifs/cifs_dfs_ref.c |  2 --
>  fs/cifs/dfs.h          | 22 ++++++++++++++++++----
>  2 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
> index cb40074feb3e..0329a907bdfe 100644
> --- a/fs/cifs/cifs_dfs_ref.c
> +++ b/fs/cifs/cifs_dfs_ref.c
> @@ -171,8 +171,6 @@ static struct vfsmount *cifs_dfs_do_automount(struct path *path)
>                 mnt = ERR_CAST(full_path);
>                 goto out;
>         }
> -
> -       convert_delimiter(full_path, '/');
>         cifs_dbg(FYI, "%s: full_path: %s\n", __func__, full_path);
>
>         tmp = *cur_ctx;
> diff --git a/fs/cifs/dfs.h b/fs/cifs/dfs.h
> index 13f26e01f7b9..0b8cbf721fff 100644
> --- a/fs/cifs/dfs.h
> +++ b/fs/cifs/dfs.h
> @@ -34,19 +34,33 @@ static inline int dfs_get_referral(struct cifs_mount_ctx *mnt_ctx, const char *p
>                               cifs_remap(cifs_sb), path, ref, tl);
>  }
>
> +/* Return DFS full path out of a dentry set for automount */
>  static inline char *dfs_get_automount_devname(struct dentry *dentry, void *page)
>  {
>         struct cifs_sb_info *cifs_sb = CIFS_SB(dentry->d_sb);
>         struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
>         struct TCP_Server_Info *server = tcon->ses->server;
> +       size_t len;
> +       char *s;
>
>         if (unlikely(!server->origin_fullpath))
>                 return ERR_PTR(-EREMOTE);
>
> -       return __build_path_from_dentry_optional_prefix(dentry, page,
> -                                                       server->origin_fullpath,
> -                                                       strlen(server->origin_fullpath),
> -                                                       true);
> +       s = dentry_path_raw(dentry, page, PATH_MAX);
> +       if (IS_ERR(s))
> +               return s;
> +       /* for root, we want "" */
> +       if (!s[1])
> +               s++;
> +
> +       len = strlen(server->origin_fullpath);
> +       if (s < (char *)page + len)
> +               return ERR_PTR(-ENAMETOOLONG);
> +
> +       s -= len;
> +       memcpy(s, server->origin_fullpath, len);
> +       convert_delimiter(s, '/');
> +       return s;
>  }
>
>  static inline void dfs_put_root_smb_sessions(struct list_head *head)
> --
> 2.40.0
>
Tom Talpey April 17, 2023, 3:29 p.m. UTC | #2
On 4/16/2023 2:38 PM, Paulo Alcantara wrote:
> @server->origin_fullpath already contains the tree name + optional
> prefix, so avoid calling __build_path_from_dentry_optional_prefix() as
> it might end up duplicating prefix path from @cifs_sb->prepath into
> final full path.
> 
> Instead, generate DFS full path by simply merging
> @server->origin_fullpath with dentry's path.
> 
> This fixes the following case
> 
> 	mount.cifs //root/dfs/dir /mnt/ -o ...
> 	ls /mnt/link
> 
> where cifs_dfs_do_automount() will call smb3_parse_devname() with
> @devname set to "//root/dfs/dir/link" instead of
> "//root/dfs/dir/dir/link".
> 
> Fixes: 7ad54b98fc1f ("cifs: use origin fullpath for automounts")
> Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
> ---
> FYI, updated DFS tests to include the above case and avoid regressions.
> 
>   fs/cifs/cifs_dfs_ref.c |  2 --
>   fs/cifs/dfs.h          | 22 ++++++++++++++++++----
>   2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
> index cb40074feb3e..0329a907bdfe 100644
> --- a/fs/cifs/cifs_dfs_ref.c
> +++ b/fs/cifs/cifs_dfs_ref.c
> @@ -171,8 +171,6 @@ static struct vfsmount *cifs_dfs_do_automount(struct path *path)
>   		mnt = ERR_CAST(full_path);
>   		goto out;
>   	}
> -
> -	convert_delimiter(full_path, '/');
>   	cifs_dbg(FYI, "%s: full_path: %s\n", __func__, full_path);
>   
>   	tmp = *cur_ctx;
> diff --git a/fs/cifs/dfs.h b/fs/cifs/dfs.h
> index 13f26e01f7b9..0b8cbf721fff 100644
> --- a/fs/cifs/dfs.h
> +++ b/fs/cifs/dfs.h
> @@ -34,19 +34,33 @@ static inline int dfs_get_referral(struct cifs_mount_ctx *mnt_ctx, const char *p
>   			      cifs_remap(cifs_sb), path, ref, tl);
>   }
>   
> +/* Return DFS full path out of a dentry set for automount */
>   static inline char *dfs_get_automount_devname(struct dentry *dentry, void *page)
>   {
>   	struct cifs_sb_info *cifs_sb = CIFS_SB(dentry->d_sb);
>   	struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
>   	struct TCP_Server_Info *server = tcon->ses->server;
> +	size_t len;
> +	char *s;
>   
>   	if (unlikely(!server->origin_fullpath))
>   		return ERR_PTR(-EREMOTE);
>   
> -	return __build_path_from_dentry_optional_prefix(dentry, page,
> -							server->origin_fullpath,
> -							strlen(server->origin_fullpath),
> -							true);
> +	s = dentry_path_raw(dentry, page, PATH_MAX);
> +	if (IS_ERR(s))
> +		return s;
> +	/* for root, we want "" */
> +	if (!s[1])
> +		s++;

The above pointer increment is really hard to understand, given the
comment. So, if the result is a single-character path, presumably "/",
advance the pointer so it becomes a null string? It's not obvious from
this code and comment.

> +	len = strlen(server->origin_fullpath);
> +	if (s < (char *)page + len)
> +		return ERR_PTR(-ENAMETOOLONG);
> +
> +	s -= len;

This looks doubly dangerous. What prevents the pointer from moving
backwards to ahead of the buffer? Especially in light of the above
root-only adjustment?

> +	memcpy(s, server->origin_fullpath, len);
> +	convert_delimiter(s, '/');
> +	return s;
>   }
>   
>   static inline void dfs_put_root_smb_sessions(struct list_head *head)

Tom.
Paulo Alcantara April 17, 2023, 4:56 p.m. UTC | #3
Tom Talpey <tom@talpey.com> writes:

> On 4/16/2023 2:38 PM, Paulo Alcantara wrote:
>> @@ -34,19 +34,33 @@ static inline int dfs_get_referral(struct cifs_mount_ctx *mnt_ctx, const char *p
>>   			      cifs_remap(cifs_sb), path, ref, tl);
>>   }
>>   
>> +/* Return DFS full path out of a dentry set for automount */
>>   static inline char *dfs_get_automount_devname(struct dentry *dentry, void *page)
>>   {
>>   	struct cifs_sb_info *cifs_sb = CIFS_SB(dentry->d_sb);
>>   	struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
>>   	struct TCP_Server_Info *server = tcon->ses->server;
>> +	size_t len;
>> +	char *s;
>>   
>>   	if (unlikely(!server->origin_fullpath))
>>   		return ERR_PTR(-EREMOTE);
>>   
>> -	return __build_path_from_dentry_optional_prefix(dentry, page,
>> -							server->origin_fullpath,
>> -							strlen(server->origin_fullpath),
>> -							true);
>> +	s = dentry_path_raw(dentry, page, PATH_MAX);
>> +	if (IS_ERR(s))
>> +		return s;
>> +	/* for root, we want "" */
>> +	if (!s[1])
>> +		s++;
>
> The above pointer increment is really hard to understand, given the
> comment. So, if the result is a single-character path, presumably "/",
> advance the pointer so it becomes a null string? It's not obvious from
> this code and comment.

I'll improve the comment to mention "/".

>> +	len = strlen(server->origin_fullpath);
>> +	if (s < (char *)page + len)
>> +		return ERR_PTR(-ENAMETOOLONG);
>> +
>> +	s -= len;
>
> This looks doubly dangerous. What prevents the pointer from moving
> backwards to ahead of the buffer? Especially in light of the above
> root-only adjustment?

dentry_path_raw() places the path name at the _end_ of provided @page
buffer and returns a pointer to it.  The above if check should prevent
such case of happening.
diff mbox series

Patch

diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
index cb40074feb3e..0329a907bdfe 100644
--- a/fs/cifs/cifs_dfs_ref.c
+++ b/fs/cifs/cifs_dfs_ref.c
@@ -171,8 +171,6 @@  static struct vfsmount *cifs_dfs_do_automount(struct path *path)
 		mnt = ERR_CAST(full_path);
 		goto out;
 	}
-
-	convert_delimiter(full_path, '/');
 	cifs_dbg(FYI, "%s: full_path: %s\n", __func__, full_path);
 
 	tmp = *cur_ctx;
diff --git a/fs/cifs/dfs.h b/fs/cifs/dfs.h
index 13f26e01f7b9..0b8cbf721fff 100644
--- a/fs/cifs/dfs.h
+++ b/fs/cifs/dfs.h
@@ -34,19 +34,33 @@  static inline int dfs_get_referral(struct cifs_mount_ctx *mnt_ctx, const char *p
 			      cifs_remap(cifs_sb), path, ref, tl);
 }
 
+/* Return DFS full path out of a dentry set for automount */
 static inline char *dfs_get_automount_devname(struct dentry *dentry, void *page)
 {
 	struct cifs_sb_info *cifs_sb = CIFS_SB(dentry->d_sb);
 	struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
 	struct TCP_Server_Info *server = tcon->ses->server;
+	size_t len;
+	char *s;
 
 	if (unlikely(!server->origin_fullpath))
 		return ERR_PTR(-EREMOTE);
 
-	return __build_path_from_dentry_optional_prefix(dentry, page,
-							server->origin_fullpath,
-							strlen(server->origin_fullpath),
-							true);
+	s = dentry_path_raw(dentry, page, PATH_MAX);
+	if (IS_ERR(s))
+		return s;
+	/* for root, we want "" */
+	if (!s[1])
+		s++;
+
+	len = strlen(server->origin_fullpath);
+	if (s < (char *)page + len)
+		return ERR_PTR(-ENAMETOOLONG);
+
+	s -= len;
+	memcpy(s, server->origin_fullpath, len);
+	convert_delimiter(s, '/');
+	return s;
 }
 
 static inline void dfs_put_root_smb_sessions(struct list_head *head)