| Message ID | 20230416183828.18174-1-pc@manguebit.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: avoid dup prefix path in dfs_get_automount_devname() | expand |
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 >
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.
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 --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)
@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(-)