Re: [PATCH] orangefs: replace strncpy with strscpy

From: Justin Stitt
Date: Tue Jul 25 2023 - 18:38:32 EST

Next message: Aaron Armstrong Skomra: "[PATCH Wacom EKR Battery Fixes 1/3] HID: wacom: remove the battery when the EKR is off"
Previous message: Aaron Armstrong Skomra: "[PATCH Wacom EKR Battery Fixes 2/3] HID: wacom: remove unnecessary 'connected' variable from EKR"
In reply to: Kees Cook: "Re: [PATCH] orangefs: replace strncpy with strscpy"
Next in thread: Kees Cook: "Re: [PATCH] orangefs: replace strncpy with strscpy"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Jul 25, 2023 at 3:01 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Tue, Jul 25, 2023 at 09:30:30PM +0000, justinstitt@xxxxxxxxxx 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
> >
>
> --
> Kees Cook

--
Justin

Next message: Aaron Armstrong Skomra: "[PATCH Wacom EKR Battery Fixes 1/3] HID: wacom: remove the battery when the EKR is off"
Previous message: Aaron Armstrong Skomra: "[PATCH Wacom EKR Battery Fixes 2/3] HID: wacom: remove unnecessary 'connected' variable from EKR"
In reply to: Kees Cook: "Re: [PATCH] orangefs: replace strncpy with strscpy"
Next in thread: Kees Cook: "Re: [PATCH] orangefs: replace strncpy with strscpy"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]