On Mon, Apr 01, 2024 at 11:01:38PM +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.
>
> In xfs_ioctl.c:
> The current code has taken care NUL-termination by memset()'ing @label.
> This is followed by a strncpy() to perform the string copy.
>
> Use strscpy_pad() to get both 1) NUL-termination and 2) NUL-padding
> which may be needed as this is copied out to userspace.
>
> Note that this patch uses the new 2-argument version of strscpy_pad
> introduced in Commit e6584c3964f2f ("string: Allow 2-argument
> strscpy()").
>
> In xfs_xattr.c:
> There's a lot of manual memory management to get a prefix and name into
> a string. Let's use an easier to understand and more robust interface in
> scnprintf() to accomplish the same task.
>
> 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: linux-hardening@xxxxxxxxxxxxxxx
> Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx>
> ---
> Note: build-tested only.
fstested would be better. ;)
Anyway I guess that looks ok so let's find out:
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
>
> Found with: $ rg "strncpy\("
> ---
> fs/xfs/xfs_ioctl.c | 4 +---
> fs/xfs/xfs_xattr.c | 6 +-----
> 2 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index d0e2cec6210d..abef9707a433 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1755,10 +1755,8 @@ xfs_ioc_getlabel(
> /* Paranoia */
> BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX);
>
> - /* 1 larger than sb_fname, so this ensures a trailing NUL char */
> - memset(label, 0, sizeof(label));
> spin_lock(&mp->m_sb_lock);
> - strncpy(label, sbp->sb_fname, XFSLABEL_MAX);
> + strscpy_pad(label, sbp->sb_fname);
> spin_unlock(&mp->m_sb_lock);
>
> if (copy_to_user(user_label, label, sizeof(label)))
> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> index 364104e1b38a..b9256988830f 100644
> --- a/fs/xfs/xfs_xattr.c
> +++ b/fs/xfs/xfs_xattr.c
> @@ -220,11 +220,7 @@ __xfs_xattr_put_listent(
> return;
> }
> offset = context->buffer + context->count;
> - memcpy(offset, prefix, prefix_len);
> - offset += prefix_len;
> - strncpy(offset, (char *)name, namelen); /* real name */
> - offset += namelen;
> - *offset = '\0';
> + scnprintf(offset, prefix_len + namelen + 1, "%s%s", prefix, name);
>
> compute_size:
> context->count += prefix_len + namelen + 1;
>
> ---
> base-commit: 928a87efa42302a23bb9554be081a28058495f22
> change-id: 20240401-strncpy-fs-xfs-xfs_ioctl-c-8af7a895bff0
>
> Best regards,
> --
> Justin Stitt <justinstitt@xxxxxxxxxx>
>
>
