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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
Note: build-tested only.
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 <[email protected]>
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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> Note: build-tested only.
fstested would be better. ;)
Anyway I guess that looks ok so let's find out:
Reviewed-by: Darrick J. Wong <[email protected]>
--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 <[email protected]>
>
>
On Mon, Apr 01, 2024 at 11:01:38PM +0000, Justin Stitt wrote:
> +++ 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);
The change looks fine, but the 1 larger information is useful and
should be kept. Maybe move it up to where the label variable s
defined?
> 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);
If we're using scnprintf we should probably also check that it doesn't
get truncated while we're at it.
Also please split the label and ioctl and the xatte changes as they
aren't related at all.