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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> ---
> 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 <[email protected]>
>
--
Kees Cook
