hfsplus: refactor copy_name to not use strncpy

Commit Message

Justin Stitt March 21, 2024, 11:46 p.m. UTC

strncpy() is deprecated with NUL-terminated destination strings [1].

The copy_name() method does a lot of manual buffer manipulation to
eventually arrive with its desired string. If we don't know the
namespace this attr has or belongs to we want to prepend "osx." to our
final string. Following this, we're copying xattr_name and doing a
bizarre manual NUL-byte assignment with a memset where n=1.

Really, we can use some more obvious string APIs to acomplish this,
improving readability and security. Following the same control flow as
before: if we don't know the namespace let's use scnprintf() to form our
prefix + xattr_name pairing (while NUL-terminating too!). Otherwise, use
strscpy() to return the number of bytes copied into our buffer.

Note that strscpy() _can_ return -E2BIG but this is already handled by
all callsites:

In both hfsplus_listxattr_finder_info() and hfsplus_listxattr(), ret is
already type ssize_t so we can change the return type of copy_name() to
match (understanding that scnprintf()'s return type is different yet
fully representable by ssize_t). Furthermore, listxattr() in fs/xattr.c
is well-equipped to handle a potential -E2BIG return result from
vfs_listxattr():
|	ssize_t error;
...
|	error = vfs_listxattr(d, klist, size);
|	if (error > 0) {
|		if (size && copy_to_user(list, klist, error))
|			error = -EFAULT;
|	} else if (error == -ERANGE && size >= XATTR_LIST_MAX) {
|		/* The file system tried to returned a list bigger
|			than XATTR_LIST_MAX bytes. Not possible. */
|		error = -E2BIG;
|	}
... our error can potentially already be -E2BIG, skipping this else-if
and ending up at the same state as other errors.

This whole copy_name() function could really be a one-line with some
ternary statements embedded into a scnprintf() arg-list but I've opted
to maintain some semblance of readability.

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
Link: https://github.com/KSPP/linux/issues/90
Cc: [e-mail]
Signed-off-by: Justin Stitt <[e-mail]>
---
---
 fs/hfsplus/xattr.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)


---
base-commit: 241590e5a1d1b6219c8d3045c167f2fbcc076cbb
change-id: 20240321-strncpy-fs-hfsplus-xattr-c-4ebfe67f4c6d

Best regards,
--
Justin Stitt <[e-mail]>

Comments

Kees Cook March 29, 2024, 4:03 a.m. UTC | #1

On Thu, Mar 21, 2024 at 11:46:27PM +0000, Justin Stitt wrote:
> strncpy() is deprecated with NUL-terminated destination strings [1].
> 
> The copy_name() method does a lot of manual buffer manipulation to
> eventually arrive with its desired string. If we don't know the
> namespace this attr has or belongs to we want to prepend "osx." to our
> final string. Following this, we're copying xattr_name and doing a
> bizarre manual NUL-byte assignment with a memset where n=1.
> 
> Really, we can use some more obvious string APIs to acomplish this,
> improving readability and security. Following the same control flow as
> before: if we don't know the namespace let's use scnprintf() to form our
> prefix + xattr_name pairing (while NUL-terminating too!). Otherwise, use
> strscpy() to return the number of bytes copied into our buffer.
> 
> Note that strscpy() _can_ return -E2BIG but this is already handled by
> all callsites:
> 
> In both hfsplus_listxattr_finder_info() and hfsplus_listxattr(), ret is
> already type ssize_t so we can change the return type of copy_name() to
> match (understanding that scnprintf()'s return type is different yet
> fully representable by ssize_t). Furthermore, listxattr() in fs/xattr.c
> is well-equipped to handle a potential -E2BIG return result from
> vfs_listxattr():
> |	ssize_t error;
> ...
> |	error = vfs_listxattr(d, klist, size);
> |	if (error > 0) {
> |		if (size && copy_to_user(list, klist, error))
> |			error = -EFAULT;
> |	} else if (error == -ERANGE && size >= XATTR_LIST_MAX) {
> |		/* The file system tried to returned a list bigger
> |			than XATTR_LIST_MAX bytes. Not possible. */
> |		error = -E2BIG;
> |	}
> ... our error can potentially already be -E2BIG, skipping this else-if
> and ending up at the same state as other errors.
> 
> This whole copy_name() function could really be a one-line with some
> ternary statements embedded into a scnprintf() arg-list but I've opted
> to maintain some semblance of readability.
> 
> 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
> Link: https://github.com/KSPP/linux/issues/90
> Cc: [e-mail]
> Signed-off-by: Justin Stitt <[e-mail]>
> ---
> ---
>  fs/hfsplus/xattr.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 9c9ff6b8c6f7..00351f566e9f 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -400,22 +400,13 @@ static int name_len(const char *xattr_name, int xattr_name_len)
>  	return len;
>  }
>  
> -static int copy_name(char *buffer, const char *xattr_name, int name_len)
> +static ssize_t copy_name(char *buffer, const char *xattr_name, int name_len)
>  {
> -	int len = name_len;
> -	int offset = 0;
> -
> -	if (!is_known_namespace(xattr_name)) {
> -		memcpy(buffer, XATTR_MAC_OSX_PREFIX, XATTR_MAC_OSX_PREFIX_LEN);
> -		offset += XATTR_MAC_OSX_PREFIX_LEN;
> -		len += XATTR_MAC_OSX_PREFIX_LEN;
> -	}
> -
> -	strncpy(buffer + offset, xattr_name, name_len);
> -	memset(buffer + offset + name_len, 0, 1);
> -	len += 1;

The old code appears to include the NUL in the count of bytes written.

> +	if (!is_known_namespace(xattr_name))
> +		return scnprintf(buffer, name_len + XATTR_MAC_OSX_PREFIX_LEN,
> +				 "%s%s", XATTR_MAC_OSX_PREFIX, xattr_name);
>  
> -	return len;
> +	return strscpy(buffer, xattr_name, name_len + 1);

Neither scnprintf nor strscpy include the NUL in their return value, so
I think special handling is needed here. Perhaps:

	ssize_t len;
	...
	if (!is_known_namespace(xattr_name))
		len = scnprintf(buffer, name_len + XATTR_MAC_OSX_PREFIX_LEN,
				 "%s%s", XATTR_MAC_OSX_PREFIX, xattr_name);
	else
		len = strscpy(buffer, xattr_name, name_len + 1);

	if (len >= 0)
		len++;
	return len;

-Kees

>  }
>  
>  int hfsplus_setxattr(struct inode *inode, const char *name,
> 
> ---
> base-commit: 241590e5a1d1b6219c8d3045c167f2fbcc076cbb
> change-id: 20240321-strncpy-fs-hfsplus-xattr-c-4ebfe67f4c6d
> 
> Best regards,
> --
> Justin Stitt <[e-mail]>
>

Patch

diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index 9c9ff6b8c6f7..00351f566e9f 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -400,22 +400,13 @@  static int name_len(const char *xattr_name, int xattr_name_len)
 	return len;
 }
 
-static int copy_name(char *buffer, const char *xattr_name, int name_len)
+static ssize_t copy_name(char *buffer, const char *xattr_name, int name_len)
 {
-	int len = name_len;
-	int offset = 0;
-
-	if (!is_known_namespace(xattr_name)) {
-		memcpy(buffer, XATTR_MAC_OSX_PREFIX, XATTR_MAC_OSX_PREFIX_LEN);
-		offset += XATTR_MAC_OSX_PREFIX_LEN;
-		len += XATTR_MAC_OSX_PREFIX_LEN;
-	}
-
-	strncpy(buffer + offset, xattr_name, name_len);
-	memset(buffer + offset + name_len, 0, 1);
-	len += 1;
+	if (!is_known_namespace(xattr_name))
+		return scnprintf(buffer, name_len + XATTR_MAC_OSX_PREFIX_LEN,
+				 "%s%s", XATTR_MAC_OSX_PREFIX, xattr_name);
 
-	return len;
+	return strscpy(buffer, xattr_name, name_len + 1);
 }
 
 int hfsplus_setxattr(struct inode *inode, const char *name,