* [PATCH] orangefs: replace strncpy with strscpy @ 2023-07-25 21:30 justinstitt 2023-07-25 22:01 ` Kees Cook 0 siblings, 1 reply; 4+ messages in thread From: justinstitt @ 2023-07-25 21:30 UTC (permalink / raw) To: Justin Stitt, Mike Marshall, Martin Brandenburg Cc: devel, linux-kernel, Nathan Chancellor, Kees Cook This patch aims to eliminate `strncpy` usage across the orangefs tree. `strncpy` is deprecated for use on NUL-terminated destination strings [1]. A suitable replacement is `strscpy` [2]. Using the `strscpy` api over `strncpy` has a slight wrinkle in the use cases presented within orangefs. There is frequent usage of `...LEN - 1` which is no longer required since `strscpy` will guarantee NUL-termination on its `dest` argument. As per `strscpy`s implementation in `linux/lib/string.c` | /* Hit buffer length without finding a NUL; force NUL-termination. */ | if (res) | dest[res-1] = '\0'; There are some hopes that someday the `strncpy` api could be ripped out due to the vast number of suitable replacements (strscpy, strscpy_pad, strtomem, strtomem_pad, strlcpy) [1]. [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html --- Link: https://github.com/KSPP/linux/issues/90 Signed-off-by: Justin Stitt <justinstitt@google.com> --- fs/orangefs/dcache.c | 4 ++-- fs/orangefs/namei.c | 30 +++++++++++++++--------------- fs/orangefs/super.c | 14 +++++++------- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c index 8bbe9486e3a6..96ed9900f7a9 100644 --- a/fs/orangefs/dcache.c +++ b/fs/orangefs/dcache.c @@ -33,9 +33,9 @@ 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, + strscpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name, - ORANGEFS_NAME_MAX - 1); + ORANGEFS_NAME_MAX); 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 77518e248cf7..503d07769bb4 100644 --- a/fs/orangefs/namei.c +++ b/fs/orangefs/namei.c @@ -41,8 +41,8 @@ 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, ORANGEFS_NAME_MAX); ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); @@ -137,8 +137,8 @@ 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, + ORANGEFS_NAME_MAX); gossip_debug(GOSSIP_NAME_DEBUG, "%s: doing lookup on %s under %pU,%d\n", @@ -192,8 +192,8 @@ 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, + ORANGEFS_NAME_MAX); ret = service_operation(new_op, "orangefs_unlink", get_interruptible_flag(inode)); @@ -247,10 +247,10 @@ static int orangefs_symlink(struct mnt_idmap *idmap, ORANGEFS_TYPE_SYMLINK, mode); - strncpy(new_op->upcall.req.sym.entry_name, + strscpy(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); + ORANGEFS_NAME_MAX); + strscpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX); ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); @@ -324,8 +324,8 @@ 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, ORANGEFS_NAME_MAX); ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); @@ -405,12 +405,12 @@ 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, + strscpy(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, + ORANGEFS_NAME_MAX); + strscpy(new_op->upcall.req.rename.d_new_name, new_dentry->d_name.name, - ORANGEFS_NAME_MAX - 1); + ORANGEFS_NAME_MAX); ret = service_operation(new_op, "orangefs_rename", diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c index 5254256a224d..b4af98b5a216 100644 --- a/fs/orangefs/super.c +++ b/fs/orangefs/super.c @@ -253,7 +253,7 @@ 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, + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server, orangefs_sb->devname, ORANGEFS_MAX_SERVER_ADDR_LEN); @@ -400,8 +400,8 @@ 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, ORANGEFS_MAX_SERVER_ADDR_LEN); r = service_operation(op, "orangefs_fs_umount", 0); /* Not much to do about an error here. */ if (r) @@ -494,9 +494,9 @@ 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, + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server, devname, - ORANGEFS_MAX_SERVER_ADDR_LEN - 1); + ORANGEFS_MAX_SERVER_ADDR_LEN); gossip_debug(GOSSIP_SUPER_DEBUG, "Attempting ORANGEFS Mount via host %s\n", @@ -543,9 +543,9 @@ struct dentry *orangefs_mount(struct file_system_type *fst, * on successful mount, store the devname and data * used */ - strncpy(ORANGEFS_SB(sb)->devname, + strscpy(ORANGEFS_SB(sb)->devname, devname, - ORANGEFS_MAX_SERVER_ADDR_LEN - 1); + ORANGEFS_MAX_SERVER_ADDR_LEN); /* mount_pending must be cleared */ ORANGEFS_SB(sb)->mount_pending = 0; --- base-commit: 0b5547c51827e053cc754db47d3ec3e6c2c451d2 change-id: 20230725-fs-orangefs-remove-deprecated-strncpy-ae0d40124620 Best regards, -- Justin Stitt <justinstitt@google.com> ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] orangefs: replace strncpy with strscpy 2023-07-25 21:30 [PATCH] orangefs: replace strncpy with strscpy justinstitt @ 2023-07-25 22:01 ` Kees Cook 2023-07-25 22:29 ` Justin Stitt 0 siblings, 1 reply; 4+ messages in thread From: Kees Cook @ 2023-07-25 22:01 UTC (permalink / raw) To: justinstitt Cc: Mike Marshall, Martin Brandenburg, devel, linux-kernel, Nathan Chancellor On Tue, Jul 25, 2023 at 09:30:30PM +0000, justinstitt@google.com wrote: > This patch aims to eliminate `strncpy` usage across the orangefs > tree. > > `strncpy` is deprecated for use on NUL-terminated destination strings > [1]. > > A suitable replacement is `strscpy` [2]. > > Using the `strscpy` api over `strncpy` has a slight wrinkle in the use > cases presented within orangefs. There is frequent usage of `...LEN - 1` > which is no longer required since `strscpy` will guarantee > NUL-termination on its `dest` argument. As per `strscpy`s implementation > in `linux/lib/string.c` > > | /* Hit buffer length without finding a NUL; force NUL-termination. */ > | if (res) > | dest[res-1] = '\0'; > > There are some hopes that someday the `strncpy` api could be ripped out > due to the vast number of suitable replacements (strscpy, strscpy_pad, > strtomem, strtomem_pad, strlcpy) [1]. > > [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > > --- > > > Link: https://github.com/KSPP/linux/issues/90 > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > fs/orangefs/dcache.c | 4 ++-- > fs/orangefs/namei.c | 30 +++++++++++++++--------------- > fs/orangefs/super.c | 14 +++++++------- > 3 files changed, 24 insertions(+), 24 deletions(-) > > diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c > index 8bbe9486e3a6..96ed9900f7a9 100644 > --- a/fs/orangefs/dcache.c > +++ b/fs/orangefs/dcache.c > @@ -33,9 +33,9 @@ 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, > + strscpy(new_op->upcall.req.lookup.d_name, > dentry->d_name.name, > - ORANGEFS_NAME_MAX - 1); > + ORANGEFS_NAME_MAX); Looking where new_op comes from, I see that it was already zero-filled, so this isn't also fixing a latent bug. (But I wanted to double-check.) > > 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 77518e248cf7..503d07769bb4 100644 > --- a/fs/orangefs/namei.c > +++ b/fs/orangefs/namei.c > @@ -41,8 +41,8 @@ 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, ORANGEFS_NAME_MAX); > > ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); > > @@ -137,8 +137,8 @@ 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, > + ORANGEFS_NAME_MAX); > > gossip_debug(GOSSIP_NAME_DEBUG, > "%s: doing lookup on %s under %pU,%d\n", > @@ -192,8 +192,8 @@ 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, > + ORANGEFS_NAME_MAX); > > ret = service_operation(new_op, "orangefs_unlink", > get_interruptible_flag(inode)); > @@ -247,10 +247,10 @@ static int orangefs_symlink(struct mnt_idmap *idmap, > ORANGEFS_TYPE_SYMLINK, > mode); > > - strncpy(new_op->upcall.req.sym.entry_name, > + strscpy(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); > + ORANGEFS_NAME_MAX); > + strscpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX); > > ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); > > @@ -324,8 +324,8 @@ 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, ORANGEFS_NAME_MAX); > > ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); > > @@ -405,12 +405,12 @@ 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, > + strscpy(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, > + ORANGEFS_NAME_MAX); > + strscpy(new_op->upcall.req.rename.d_new_name, > new_dentry->d_name.name, > - ORANGEFS_NAME_MAX - 1); > + ORANGEFS_NAME_MAX); > > ret = service_operation(new_op, > "orangefs_rename", > diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c > index 5254256a224d..b4af98b5a216 100644 > --- a/fs/orangefs/super.c > +++ b/fs/orangefs/super.c > @@ -253,7 +253,7 @@ 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, > + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server, > orangefs_sb->devname, > ORANGEFS_MAX_SERVER_ADDR_LEN); Was this a bug? (I think unreachable, both are ORANGEFS_MAX_SERVER_ADDR_LEN long, but devname would already be NUL-terminated.) Also, I wonder if all of these could be converted to: strscpy(dest, source, sizeof(dest)) Which (I think) would be a no-op change, and seems like a more robust code style. > > @@ -400,8 +400,8 @@ 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, ORANGEFS_MAX_SERVER_ADDR_LEN); > r = service_operation(op, "orangefs_fs_umount", 0); > /* Not much to do about an error here. */ > if (r) > @@ -494,9 +494,9 @@ 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, > + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server, > devname, > - ORANGEFS_MAX_SERVER_ADDR_LEN - 1); > + ORANGEFS_MAX_SERVER_ADDR_LEN); > > gossip_debug(GOSSIP_SUPER_DEBUG, > "Attempting ORANGEFS Mount via host %s\n", > @@ -543,9 +543,9 @@ struct dentry *orangefs_mount(struct file_system_type *fst, > * on successful mount, store the devname and data > * used > */ > - strncpy(ORANGEFS_SB(sb)->devname, > + strscpy(ORANGEFS_SB(sb)->devname, > devname, > - ORANGEFS_MAX_SERVER_ADDR_LEN - 1); > + ORANGEFS_MAX_SERVER_ADDR_LEN); > > /* mount_pending must be cleared */ > ORANGEFS_SB(sb)->mount_pending = 0; > > --- > base-commit: 0b5547c51827e053cc754db47d3ec3e6c2c451d2 > change-id: 20230725-fs-orangefs-remove-deprecated-strncpy-ae0d40124620 > > Best regards, > -- > Justin Stitt <justinstitt@google.com> > -- Kees Cook ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] orangefs: replace strncpy with strscpy 2023-07-25 22:01 ` Kees Cook @ 2023-07-25 22:29 ` Justin Stitt 2023-07-26 22:47 ` Kees Cook 0 siblings, 1 reply; 4+ messages in thread From: Justin Stitt @ 2023-07-25 22:29 UTC (permalink / raw) To: Kees Cook Cc: Mike Marshall, Martin Brandenburg, devel, linux-kernel, Nathan Chancellor On Tue, Jul 25, 2023 at 3:01 PM Kees Cook <keescook@chromium.org> wrote: > > On Tue, Jul 25, 2023 at 09:30:30PM +0000, justinstitt@google.com wrote: > > This patch aims to eliminate `strncpy` usage across the orangefs > > tree. > > > > `strncpy` is deprecated for use on NUL-terminated destination strings > > [1]. > > > > A suitable replacement is `strscpy` [2]. > > > > Using the `strscpy` api over `strncpy` has a slight wrinkle in the use > > cases presented within orangefs. There is frequent usage of `...LEN - 1` > > which is no longer required since `strscpy` will guarantee > > NUL-termination on its `dest` argument. As per `strscpy`s implementation > > in `linux/lib/string.c` > > > > | /* Hit buffer length without finding a NUL; force NUL-termination. */ > > | if (res) > > | dest[res-1] = '\0'; > > > > There are some hopes that someday the `strncpy` api could be ripped out > > due to the vast number of suitable replacements (strscpy, strscpy_pad, > > strtomem, strtomem_pad, strlcpy) [1]. > > > > [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > > [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > > > > --- > > > > > > Link: https://github.com/KSPP/linux/issues/90 > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > --- > > fs/orangefs/dcache.c | 4 ++-- > > fs/orangefs/namei.c | 30 +++++++++++++++--------------- > > fs/orangefs/super.c | 14 +++++++------- > > 3 files changed, 24 insertions(+), 24 deletions(-) > > > > diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c > > index 8bbe9486e3a6..96ed9900f7a9 100644 > > --- a/fs/orangefs/dcache.c > > +++ b/fs/orangefs/dcache.c > > @@ -33,9 +33,9 @@ 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, > > + strscpy(new_op->upcall.req.lookup.d_name, > > dentry->d_name.name, > > - ORANGEFS_NAME_MAX - 1); > > + ORANGEFS_NAME_MAX); > > Looking where new_op comes from, I see that it was already zero-filled, > so this isn't also fixing a latent bug. (But I wanted to double-check.) > > > > > 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 77518e248cf7..503d07769bb4 100644 > > --- a/fs/orangefs/namei.c > > +++ b/fs/orangefs/namei.c > > @@ -41,8 +41,8 @@ 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, ORANGEFS_NAME_MAX); > > > > ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); > > > > @@ -137,8 +137,8 @@ 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, > > + ORANGEFS_NAME_MAX); > > > > gossip_debug(GOSSIP_NAME_DEBUG, > > "%s: doing lookup on %s under %pU,%d\n", > > @@ -192,8 +192,8 @@ 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, > > + ORANGEFS_NAME_MAX); > > > > ret = service_operation(new_op, "orangefs_unlink", > > get_interruptible_flag(inode)); > > @@ -247,10 +247,10 @@ static int orangefs_symlink(struct mnt_idmap *idmap, > > ORANGEFS_TYPE_SYMLINK, > > mode); > > > > - strncpy(new_op->upcall.req.sym.entry_name, > > + strscpy(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); > > + ORANGEFS_NAME_MAX); > > + strscpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX); > > > > ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); > > > > @@ -324,8 +324,8 @@ 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, ORANGEFS_NAME_MAX); > > > > ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); > > > > @@ -405,12 +405,12 @@ 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, > > + strscpy(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, > > + ORANGEFS_NAME_MAX); > > + strscpy(new_op->upcall.req.rename.d_new_name, > > new_dentry->d_name.name, > > - ORANGEFS_NAME_MAX - 1); > > + ORANGEFS_NAME_MAX); > > > > ret = service_operation(new_op, > > "orangefs_rename", > > diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c > > index 5254256a224d..b4af98b5a216 100644 > > --- a/fs/orangefs/super.c > > +++ b/fs/orangefs/super.c > > @@ -253,7 +253,7 @@ 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, > > + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server, > > orangefs_sb->devname, > > ORANGEFS_MAX_SERVER_ADDR_LEN); > > Was this a bug? (I think unreachable, both are > ORANGEFS_MAX_SERVER_ADDR_LEN long, but devname would already be > NUL-terminated.) > > Also, I wonder if all of these could be converted to: > > strscpy(dest, source, sizeof(dest)) > I wonder if this idiom is a bit awkward in this context due to `dest` being quite a long series of struct accesses. For reference: | strncpy(new_op->upcall.req.fs_mount.orangefs_config_server, | orangefs_sb->devname, | sizeof(new_op->upcall.req.fs_mount.orangefs_config_server)); The resolution would be creating a temp variable for the purposes of avoiding this long pattern. But that would mean it should probably be done for all instances like this. Is it worth it? Or is the long-winded sizeof(dest) OK? > Which (I think) would be a no-op change, and seems like a more robust > code style. > > > > > @@ -400,8 +400,8 @@ 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, ORANGEFS_MAX_SERVER_ADDR_LEN); > > r = service_operation(op, "orangefs_fs_umount", 0); > > /* Not much to do about an error here. */ > > if (r) > > @@ -494,9 +494,9 @@ 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, > > + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server, > > devname, > > - ORANGEFS_MAX_SERVER_ADDR_LEN - 1); > > + ORANGEFS_MAX_SERVER_ADDR_LEN); > > > > gossip_debug(GOSSIP_SUPER_DEBUG, > > "Attempting ORANGEFS Mount via host %s\n", > > @@ -543,9 +543,9 @@ struct dentry *orangefs_mount(struct file_system_type *fst, > > * on successful mount, store the devname and data > > * used > > */ > > - strncpy(ORANGEFS_SB(sb)->devname, > > + strscpy(ORANGEFS_SB(sb)->devname, > > devname, > > - ORANGEFS_MAX_SERVER_ADDR_LEN - 1); > > + ORANGEFS_MAX_SERVER_ADDR_LEN); > > > > /* mount_pending must be cleared */ > > ORANGEFS_SB(sb)->mount_pending = 0; > > > > --- > > base-commit: 0b5547c51827e053cc754db47d3ec3e6c2c451d2 > > change-id: 20230725-fs-orangefs-remove-deprecated-strncpy-ae0d40124620 > > > > Best regards, > > -- > > Justin Stitt <justinstitt@google.com> > > > > -- > Kees Cook -- Justin ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] orangefs: replace strncpy with strscpy 2023-07-25 22:29 ` Justin Stitt @ 2023-07-26 22:47 ` Kees Cook 0 siblings, 0 replies; 4+ messages in thread From: Kees Cook @ 2023-07-26 22:47 UTC (permalink / raw) To: Justin Stitt Cc: Mike Marshall, Martin Brandenburg, devel, linux-kernel, Nathan Chancellor On Tue, Jul 25, 2023 at 03:29:10PM -0700, Justin Stitt wrote: > On Tue, Jul 25, 2023 at 3:01 PM Kees Cook <keescook@chromium.org> wrote: > > > > On Tue, Jul 25, 2023 at 09:30:30PM +0000, justinstitt@google.com wrote: > > > This patch aims to eliminate `strncpy` usage across the orangefs > > > tree. > > > > > > `strncpy` is deprecated for use on NUL-terminated destination strings > > > [1]. > > > > > > A suitable replacement is `strscpy` [2]. > > > > > > Using the `strscpy` api over `strncpy` has a slight wrinkle in the use > > > cases presented within orangefs. There is frequent usage of `...LEN - 1` > > > which is no longer required since `strscpy` will guarantee > > > NUL-termination on its `dest` argument. As per `strscpy`s implementation > > > in `linux/lib/string.c` > > > > > > | /* Hit buffer length without finding a NUL; force NUL-termination. */ > > > | if (res) > > > | dest[res-1] = '\0'; > > > > > > There are some hopes that someday the `strncpy` api could be ripped out > > > due to the vast number of suitable replacements (strscpy, strscpy_pad, > > > strtomem, strtomem_pad, strlcpy) [1]. > > > > > > [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > > > [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > > > > > > --- > > > > > > > > > Link: https://github.com/KSPP/linux/issues/90 > > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > > --- > > > fs/orangefs/dcache.c | 4 ++-- > > > fs/orangefs/namei.c | 30 +++++++++++++++--------------- > > > fs/orangefs/super.c | 14 +++++++------- > > > 3 files changed, 24 insertions(+), 24 deletions(-) > > > > > > diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c > > > index 8bbe9486e3a6..96ed9900f7a9 100644 > > > --- a/fs/orangefs/dcache.c > > > +++ b/fs/orangefs/dcache.c > > > @@ -33,9 +33,9 @@ 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, > > > + strscpy(new_op->upcall.req.lookup.d_name, > > > dentry->d_name.name, > > > - ORANGEFS_NAME_MAX - 1); > > > + ORANGEFS_NAME_MAX); > > > > Looking where new_op comes from, I see that it was already zero-filled, > > so this isn't also fixing a latent bug. (But I wanted to double-check.) > > > > > > > > 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 77518e248cf7..503d07769bb4 100644 > > > --- a/fs/orangefs/namei.c > > > +++ b/fs/orangefs/namei.c > > > @@ -41,8 +41,8 @@ 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, ORANGEFS_NAME_MAX); > > > > > > ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); > > > > > > @@ -137,8 +137,8 @@ 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, > > > + ORANGEFS_NAME_MAX); > > > > > > gossip_debug(GOSSIP_NAME_DEBUG, > > > "%s: doing lookup on %s under %pU,%d\n", > > > @@ -192,8 +192,8 @@ 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, > > > + ORANGEFS_NAME_MAX); > > > > > > ret = service_operation(new_op, "orangefs_unlink", > > > get_interruptible_flag(inode)); > > > @@ -247,10 +247,10 @@ static int orangefs_symlink(struct mnt_idmap *idmap, > > > ORANGEFS_TYPE_SYMLINK, > > > mode); > > > > > > - strncpy(new_op->upcall.req.sym.entry_name, > > > + strscpy(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); > > > + ORANGEFS_NAME_MAX); > > > + strscpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX); > > > > > > ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); > > > > > > @@ -324,8 +324,8 @@ 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, ORANGEFS_NAME_MAX); > > > > > > ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); > > > > > > @@ -405,12 +405,12 @@ 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, > > > + strscpy(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, > > > + ORANGEFS_NAME_MAX); > > > + strscpy(new_op->upcall.req.rename.d_new_name, > > > new_dentry->d_name.name, > > > - ORANGEFS_NAME_MAX - 1); > > > + ORANGEFS_NAME_MAX); > > > > > > ret = service_operation(new_op, > > > "orangefs_rename", > > > diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c > > > index 5254256a224d..b4af98b5a216 100644 > > > --- a/fs/orangefs/super.c > > > +++ b/fs/orangefs/super.c > > > @@ -253,7 +253,7 @@ 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, > > > + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server, > > > orangefs_sb->devname, > > > ORANGEFS_MAX_SERVER_ADDR_LEN); > > > > Was this a bug? (I think unreachable, both are > > ORANGEFS_MAX_SERVER_ADDR_LEN long, but devname would already be > > NUL-terminated.) > > > > Also, I wonder if all of these could be converted to: > > > > strscpy(dest, source, sizeof(dest)) > > > > I wonder if this idiom is a bit awkward in this context due to `dest` > being quite a long series of struct accesses. > For reference: > | strncpy(new_op->upcall.req.fs_mount.orangefs_config_server, > | orangefs_sb->devname, > | sizeof(new_op->upcall.req.fs_mount.orangefs_config_server)); > > The resolution would be creating a temp variable for the purposes of > avoiding this long pattern. But that would mean it should probably be > done for all instances like this. > > Is it worth it? Or is the long-winded sizeof(dest) OK? I wouldn't add a variable. For this, let's skip the use of sizeof(). I think in the future we might be able to do some kind of treewide change to fix all of these. > > > > Which (I think) would be a no-op change, and seems like a more robust > > code style. > > > > > > > > > @@ -400,8 +400,8 @@ 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, ORANGEFS_MAX_SERVER_ADDR_LEN); > > > r = service_operation(op, "orangefs_fs_umount", 0); > > > /* Not much to do about an error here. */ > > > if (r) > > > @@ -494,9 +494,9 @@ 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, > > > + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server, > > > devname, > > > - ORANGEFS_MAX_SERVER_ADDR_LEN - 1); > > > + ORANGEFS_MAX_SERVER_ADDR_LEN); > > > > > > gossip_debug(GOSSIP_SUPER_DEBUG, > > > "Attempting ORANGEFS Mount via host %s\n", > > > @@ -543,9 +543,9 @@ struct dentry *orangefs_mount(struct file_system_type *fst, > > > * on successful mount, store the devname and data > > > * used > > > */ > > > - strncpy(ORANGEFS_SB(sb)->devname, > > > + strscpy(ORANGEFS_SB(sb)->devname, > > > devname, > > > - ORANGEFS_MAX_SERVER_ADDR_LEN - 1); > > > + ORANGEFS_MAX_SERVER_ADDR_LEN); > > > > > > /* mount_pending must be cleared */ > > > ORANGEFS_SB(sb)->mount_pending = 0; > > > > > > --- > > > base-commit: 0b5547c51827e053cc754db47d3ec3e6c2c451d2 > > > change-id: 20230725-fs-orangefs-remove-deprecated-strncpy-ae0d40124620 > > > > > > Best regards, > > > -- > > > Justin Stitt <justinstitt@google.com> > > > > > > > -- > > Kees Cook > > > > -- > Justin -- Kees Cook ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-07-26 22:47 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-07-25 21:30 [PATCH] orangefs: replace strncpy with strscpy justinstitt 2023-07-25 22:01 ` Kees Cook 2023-07-25 22:29 ` Justin Stitt 2023-07-26 22:47 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox