diff mbox series

xfs: cleanup deprecated uses of strncpy

Message ID 20240401-strncpy-fs-xfs-xfs_ioctl-c-v1-1-02b9feb1989b@google.com (mailing list archive)
State New, archived
Headers show
Series xfs: cleanup deprecated uses of strncpy | expand

Commit Message

Justin Stitt April 1, 2024, 11:01 p.m. UTC 
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@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
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(-)


---
base-commit: 928a87efa42302a23bb9554be081a28058495f22
change-id: 20240401-strncpy-fs-xfs-xfs_ioctl-c-8af7a895bff0

Best regards,
--
Justin Stitt <justinstitt@google.com>

Comments

Darrick J. Wong April 3, 2024, 4:11 a.m. UTC | #1 
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@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> 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@kernel.org>

--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@google.com>
> 
>
Christoph Hellwig April 3, 2024, 5:07 a.m. UTC | #2 
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.
diff mbox series

Patch

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;