From: Kees Cook <keescook@chromium.org>
To: Justin Stitt <justinstitt@google.com>
Cc: Mike Marshall <hubcap@omnibond.com>,
Martin Brandenburg <martin@omnibond.com>,
devel@lists.orangefs.org, linux-kernel@vger.kernel.org,
Nathan Chancellor <nathan@kernel.org>
Subject: Re: [PATCH] orangefs: replace strncpy with strscpy
Date: Wed, 26 Jul 2023 15:47:14 -0700 [thread overview]
Message-ID: <202307261545.E6827CBAD2@keescook> (raw)
In-Reply-To: <CAFhGd8qRyq=Fz++1SHxhLkj5Z6UkE97a-UobuUq2hfEwE=K=0w@mail.gmail.com>
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
prev parent reply other threads:[~2023-07-26 22:47 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202307261545.E6827CBAD2@keescook \
--to=keescook@chromium.org \
--cc=devel@lists.orangefs.org \
--cc=hubcap@omnibond.com \
--cc=justinstitt@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=martin@omnibond.com \
--cc=nathan@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.