strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
There is some care taken to ensure these destination buffers are
NUL-terminated by bounding the strncpy()'s by ORANGEFS_NAME_MAX - 1 or
ORANGEFS_MAX_SERVER_ADDR_LEN - 1. Instead, we can use the new 2-argument
version of strscpy() to guarantee NUL-termination on the destination
buffers while simplifying the code.
Based on usage with printf-likes, we can see these buffers are expected
to be NUL-terminated:
| gossip_debug(GOSSIP_NAME_DEBUG,
| "%s: doing lookup on %s under %pU,%d\n",
| __func__,
| new_op->upcall.req.lookup.d_name,
| &new_op->upcall.req.lookup.parent_refn.khandle,
| new_op->upcall.req.lookup.parent_refn.fs_id);
..
| gossip_debug(GOSSIP_SUPER_DEBUG,
| "Attempting ORANGEFS Remount via host %s\n",
| new_op->upcall.req.fs_mount.orangefs_config_server);
NUL-padding isn't required for any of these destination buffers as
they've all been zero-allocated with op_alloc() or kzalloc().
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
Note: build-tested only.
Found with: $ rg "strncpy\("
---
fs/orangefs/dcache.c | 4 +---
fs/orangefs/namei.c | 26 ++++++++------------------
fs/orangefs/super.c | 17 ++++++-----------
3 files changed, 15 insertions(+), 32 deletions(-)
diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c
index 8bbe9486e3a6..395a00ed8ac7 100644
--- a/fs/orangefs/dcache.c
+++ b/fs/orangefs/dcache.c
@@ -33,9 +33,7 @@ static int orangefs_revalidate_lookup(struct dentry *dentry)
new_op->upcall.req.lookup.sym_follow = ORANGEFS_LOOKUP_LINK_NO_FOLLOW;
new_op->upcall.req.lookup.parent_refn = parent->refn;
- strncpy(new_op->upcall.req.lookup.d_name,
- dentry->d_name.name,
- ORANGEFS_NAME_MAX - 1);
+ strscpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name);
gossip_debug(GOSSIP_DCACHE_DEBUG,
"%s:%s:%d interrupt flag [%d]\n",
diff --git a/fs/orangefs/namei.c b/fs/orangefs/namei.c
index c9dfd5c6a097..200558ec72f0 100644
--- a/fs/orangefs/namei.c
+++ b/fs/orangefs/namei.c
@@ -41,8 +41,7 @@ static int orangefs_create(struct mnt_idmap *idmap,
fill_default_sys_attrs(new_op->upcall.req.create.attributes,
ORANGEFS_TYPE_METAFILE, mode);
- strncpy(new_op->upcall.req.create.d_name,
- dentry->d_name.name, ORANGEFS_NAME_MAX - 1);
+ strscpy(new_op->upcall.req.create.d_name, dentry->d_name.name);
ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
@@ -137,8 +136,7 @@ static struct dentry *orangefs_lookup(struct inode *dir, struct dentry *dentry,
&parent->refn.khandle);
new_op->upcall.req.lookup.parent_refn = parent->refn;
- strncpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name,
- ORANGEFS_NAME_MAX - 1);
+ strscpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name);
gossip_debug(GOSSIP_NAME_DEBUG,
"%s: doing lookup on %s under %pU,%d\n",
@@ -192,8 +190,7 @@ static int orangefs_unlink(struct inode *dir, struct dentry *dentry)
return -ENOMEM;
new_op->upcall.req.remove.parent_refn = parent->refn;
- strncpy(new_op->upcall.req.remove.d_name, dentry->d_name.name,
- ORANGEFS_NAME_MAX - 1);
+ strscpy(new_op->upcall.req.remove.d_name, dentry->d_name.name);
ret = service_operation(new_op, "orangefs_unlink",
get_interruptible_flag(inode));
@@ -247,10 +244,8 @@ static int orangefs_symlink(struct mnt_idmap *idmap,
ORANGEFS_TYPE_SYMLINK,
mode);
- strncpy(new_op->upcall.req.sym.entry_name,
- dentry->d_name.name,
- ORANGEFS_NAME_MAX - 1);
- strncpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX - 1);
+ strscpy(new_op->upcall.req.sym.entry_name, dentry->d_name.name);
+ strscpy(new_op->upcall.req.sym.target, symname);
ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
@@ -324,8 +319,7 @@ static int orangefs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
fill_default_sys_attrs(new_op->upcall.req.mkdir.attributes,
ORANGEFS_TYPE_DIRECTORY, mode);
- strncpy(new_op->upcall.req.mkdir.d_name,
- dentry->d_name.name, ORANGEFS_NAME_MAX - 1);
+ strscpy(new_op->upcall.req.mkdir.d_name, dentry->d_name.name);
ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
@@ -405,12 +399,8 @@ static int orangefs_rename(struct mnt_idmap *idmap,
new_op->upcall.req.rename.old_parent_refn = ORANGEFS_I(old_dir)->refn;
new_op->upcall.req.rename.new_parent_refn = ORANGEFS_I(new_dir)->refn;
- strncpy(new_op->upcall.req.rename.d_old_name,
- old_dentry->d_name.name,
- ORANGEFS_NAME_MAX - 1);
- strncpy(new_op->upcall.req.rename.d_new_name,
- new_dentry->d_name.name,
- ORANGEFS_NAME_MAX - 1);
+ strscpy(new_op->upcall.req.rename.d_old_name, old_dentry->d_name.name);
+ strscpy(new_op->upcall.req.rename.d_new_name, new_dentry->d_name.name);
ret = service_operation(new_op,
"orangefs_rename",
diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c
index 34849b4a3243..fb4d09c2f531 100644
--- a/fs/orangefs/super.c
+++ b/fs/orangefs/super.c
@@ -253,9 +253,8 @@ int orangefs_remount(struct orangefs_sb_info_s *orangefs_sb)
new_op = op_alloc(ORANGEFS_VFS_OP_FS_MOUNT);
if (!new_op)
return -ENOMEM;
- strncpy(new_op->upcall.req.fs_mount.orangefs_config_server,
- orangefs_sb->devname,
- ORANGEFS_MAX_SERVER_ADDR_LEN);
+ strscpy(new_op->upcall.req.fs_mount.orangefs_config_server,
+ orangefs_sb->devname);
gossip_debug(GOSSIP_SUPER_DEBUG,
"Attempting ORANGEFS Remount via host %s\n",
@@ -400,8 +399,7 @@ static int orangefs_unmount(int id, __s32 fs_id, const char *devname)
return -ENOMEM;
op->upcall.req.fs_umount.id = id;
op->upcall.req.fs_umount.fs_id = fs_id;
- strncpy(op->upcall.req.fs_umount.orangefs_config_server,
- devname, ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
+ strscpy(op->upcall.req.fs_umount.orangefs_config_server, devname);
r = service_operation(op, "orangefs_fs_umount", 0);
/* Not much to do about an error here. */
if (r)
@@ -494,9 +492,7 @@ struct dentry *orangefs_mount(struct file_system_type *fst,
if (!new_op)
return ERR_PTR(-ENOMEM);
- strncpy(new_op->upcall.req.fs_mount.orangefs_config_server,
- devname,
- ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
+ strscpy(new_op->upcall.req.fs_mount.orangefs_config_server, devname);
gossip_debug(GOSSIP_SUPER_DEBUG,
"Attempting ORANGEFS Mount via host %s\n",
@@ -543,9 +539,8 @@ struct dentry *orangefs_mount(struct file_system_type *fst,
* on successful mount, store the devname and data
* used
*/
- strncpy(ORANGEFS_SB(sb)->devname,
- devname,
- ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
+ strscpy(ORANGEFS_SB(sb)->devname, devname);
+
/* mount_pending must be cleared */
ORANGEFS_SB(sb)->mount_pending = 0;
---
base-commit: 241590e5a1d1b6219c8d3045c167f2fbcc076cbb
change-id: 20240322-strncpy-fs-orangefs-dcache-c-9a0cf2d22dae
Best regards,
--
Justin Stitt <[email protected]>
On Fri, Mar 22, 2024 at 09:41:18PM +0000, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
>
> There is some care taken to ensure these destination buffers are
> NUL-terminated by bounding the strncpy()'s by ORANGEFS_NAME_MAX - 1 or
> ORANGEFS_MAX_SERVER_ADDR_LEN - 1. Instead, we can use the new 2-argument
> version of strscpy() to guarantee NUL-termination on the destination
> buffers while simplifying the code.
>
> Based on usage with printf-likes, we can see these buffers are expected
> to be NUL-terminated:
> | gossip_debug(GOSSIP_NAME_DEBUG,
> | "%s: doing lookup on %s under %pU,%d\n",
> | __func__,
> | new_op->upcall.req.lookup.d_name,
> | &new_op->upcall.req.lookup.parent_refn.khandle,
> | new_op->upcall.req.lookup.parent_refn.fs_id);
> ...
> | gossip_debug(GOSSIP_SUPER_DEBUG,
> | "Attempting ORANGEFS Remount via host %s\n",
> | new_op->upcall.req.fs_mount.orangefs_config_server);
>
> NUL-padding isn't required for any of these destination buffers as
> they've all been zero-allocated with op_alloc() or kzalloc().
>
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> Note: build-tested only.
>
> Found with: $ rg "strncpy\("
> ---
> fs/orangefs/dcache.c | 4 +---
> fs/orangefs/namei.c | 26 ++++++++------------------
> fs/orangefs/super.c | 17 ++++++-----------
> 3 files changed, 15 insertions(+), 32 deletions(-)
>
> diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c
> index 8bbe9486e3a6..395a00ed8ac7 100644
> --- a/fs/orangefs/dcache.c
> +++ b/fs/orangefs/dcache.c
> @@ -33,9 +33,7 @@ static int orangefs_revalidate_lookup(struct dentry *dentry)
>
> new_op->upcall.req.lookup.sym_follow = ORANGEFS_LOOKUP_LINK_NO_FOLLOW;
> new_op->upcall.req.lookup.parent_refn = parent->refn;
> - strncpy(new_op->upcall.req.lookup.d_name,
> - dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name);
>
> gossip_debug(GOSSIP_DCACHE_DEBUG,
> "%s:%s:%d interrupt flag [%d]\n",
> diff --git a/fs/orangefs/namei.c b/fs/orangefs/namei.c
> index c9dfd5c6a097..200558ec72f0 100644
> --- a/fs/orangefs/namei.c
> +++ b/fs/orangefs/namei.c
> @@ -41,8 +41,7 @@ static int orangefs_create(struct mnt_idmap *idmap,
> fill_default_sys_attrs(new_op->upcall.req.create.attributes,
> ORANGEFS_TYPE_METAFILE, mode);
>
> - strncpy(new_op->upcall.req.create.d_name,
> - dentry->d_name.name, ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.create.d_name, dentry->d_name.name);
>
> ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
>
> @@ -137,8 +136,7 @@ static struct dentry *orangefs_lookup(struct inode *dir, struct dentry *dentry,
> &parent->refn.khandle);
> new_op->upcall.req.lookup.parent_refn = parent->refn;
>
> - strncpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name);
>
> gossip_debug(GOSSIP_NAME_DEBUG,
> "%s: doing lookup on %s under %pU,%d\n",
> @@ -192,8 +190,7 @@ static int orangefs_unlink(struct inode *dir, struct dentry *dentry)
> return -ENOMEM;
>
> new_op->upcall.req.remove.parent_refn = parent->refn;
> - strncpy(new_op->upcall.req.remove.d_name, dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.remove.d_name, dentry->d_name.name);
>
> ret = service_operation(new_op, "orangefs_unlink",
> get_interruptible_flag(inode));
> @@ -247,10 +244,8 @@ static int orangefs_symlink(struct mnt_idmap *idmap,
> ORANGEFS_TYPE_SYMLINK,
> mode);
>
> - strncpy(new_op->upcall.req.sym.entry_name,
> - dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> - strncpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.sym.entry_name, dentry->d_name.name);
> + strscpy(new_op->upcall.req.sym.target, symname);
>
> ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
>
> @@ -324,8 +319,7 @@ static int orangefs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> fill_default_sys_attrs(new_op->upcall.req.mkdir.attributes,
> ORANGEFS_TYPE_DIRECTORY, mode);
>
> - strncpy(new_op->upcall.req.mkdir.d_name,
> - dentry->d_name.name, ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.mkdir.d_name, dentry->d_name.name);
>
> ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
>
> @@ -405,12 +399,8 @@ static int orangefs_rename(struct mnt_idmap *idmap,
> new_op->upcall.req.rename.old_parent_refn = ORANGEFS_I(old_dir)->refn;
> new_op->upcall.req.rename.new_parent_refn = ORANGEFS_I(new_dir)->refn;
>
> - strncpy(new_op->upcall.req.rename.d_old_name,
> - old_dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> - strncpy(new_op->upcall.req.rename.d_new_name,
> - new_dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.rename.d_old_name, old_dentry->d_name.name);
> + strscpy(new_op->upcall.req.rename.d_new_name, new_dentry->d_name.name);
>
> ret = service_operation(new_op,
> "orangefs_rename",
> diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c
> index 34849b4a3243..fb4d09c2f531 100644
> --- a/fs/orangefs/super.c
> +++ b/fs/orangefs/super.c
> @@ -253,9 +253,8 @@ int orangefs_remount(struct orangefs_sb_info_s *orangefs_sb)
> new_op = op_alloc(ORANGEFS_VFS_OP_FS_MOUNT);
> if (!new_op)
> return -ENOMEM;
> - strncpy(new_op->upcall.req.fs_mount.orangefs_config_server,
> - orangefs_sb->devname,
> - ORANGEFS_MAX_SERVER_ADDR_LEN);
> + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server,
> + orangefs_sb->devname);
This one place doesn't use "ORANGEFS_MAX_SERVER_ADDR_LEN - 1", but this
appears to be an existing (likely unreachable) bug.
Reviewed-by: Kees Cook <[email protected]>
-Kees
>
> gossip_debug(GOSSIP_SUPER_DEBUG,
> "Attempting ORANGEFS Remount via host %s\n",
> @@ -400,8 +399,7 @@ static int orangefs_unmount(int id, __s32 fs_id, const char *devname)
> return -ENOMEM;
> op->upcall.req.fs_umount.id = id;
> op->upcall.req.fs_umount.fs_id = fs_id;
> - strncpy(op->upcall.req.fs_umount.orangefs_config_server,
> - devname, ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
> + strscpy(op->upcall.req.fs_umount.orangefs_config_server, devname);
> r = service_operation(op, "orangefs_fs_umount", 0);
> /* Not much to do about an error here. */
> if (r)
> @@ -494,9 +492,7 @@ struct dentry *orangefs_mount(struct file_system_type *fst,
> if (!new_op)
> return ERR_PTR(-ENOMEM);
>
> - strncpy(new_op->upcall.req.fs_mount.orangefs_config_server,
> - devname,
> - ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
> + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server, devname);
>
> gossip_debug(GOSSIP_SUPER_DEBUG,
> "Attempting ORANGEFS Mount via host %s\n",
> @@ -543,9 +539,8 @@ struct dentry *orangefs_mount(struct file_system_type *fst,
> * on successful mount, store the devname and data
> * used
> */
> - strncpy(ORANGEFS_SB(sb)->devname,
> - devname,
> - ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
> + strscpy(ORANGEFS_SB(sb)->devname, devname);
> +
>
> /* mount_pending must be cleared */
> ORANGEFS_SB(sb)->mount_pending = 0;
>
> ---
> base-commit: 241590e5a1d1b6219c8d3045c167f2fbcc076cbb
> change-id: 20240322-strncpy-fs-orangefs-dcache-c-9a0cf2d22dae
>
> Best regards,
> --
> Justin Stitt <[email protected]>
>
>
--
Kees Cook
On Fri, 22 Mar 2024 21:41:18 +0000, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
>
> There is some care taken to ensure these destination buffers are
> NUL-terminated by bounding the strncpy()'s by ORANGEFS_NAME_MAX - 1 or
> ORANGEFS_MAX_SERVER_ADDR_LEN - 1. Instead, we can use the new 2-argument
> version of strscpy() to guarantee NUL-termination on the destination
> buffers while simplifying the code.
>
> [...]
If this needs to go separately from the vfs trees let me know.
---
Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc
[1/1] orangefs: cleanup uses of strncpy
https://git.kernel.org/vfs/vfs/c/fc10fed37526
Hi...
I b4'd this patch (I love b4) and applied it to 6.9.0-rc2
and ran it through xfstests with no regressions, so please
add a tested-by for [email protected].
Thanks...
-Mike Marshall
On Mon, Apr 1, 2024 at 4:53 AM Christian Brauner <[email protected]> wrote:
>
> On Fri, 22 Mar 2024 21:41:18 +0000, Justin Stitt wrote:
> > strncpy() is deprecated for use on NUL-terminated destination strings
> > [1] and as such we should prefer more robust and less ambiguous string
> > interfaces.
> >
> > There is some care taken to ensure these destination buffers are
> > NUL-terminated by bounding the strncpy()'s by ORANGEFS_NAME_MAX - 1 or
> > ORANGEFS_MAX_SERVER_ADDR_LEN - 1. Instead, we can use the new 2-argument
> > version of strscpy() to guarantee NUL-termination on the destination
> > buffers while simplifying the code.
> >
> > [...]
>
> If this needs to go separately from the vfs trees let me know.
>
> ---
>
> Applied to the vfs.misc branch of the vfs/vfs.git tree.
> Patches in the vfs.misc branch should appear in linux-next soon.
>
> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
>
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
>
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
>
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs.misc
>
> [1/1] orangefs: cleanup uses of strncpy
> https://git.kernel.org/vfs/vfs/c/fc10fed37526