Received-SPF: pass (google.com: domain of linux-kernel+bounces-55762-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 7 Feb 2024 00:32:24 +0000
From: Justin Stitt <justinstitt@google.com>
To: Kees Cook <keescook@chromium.org>
Cc: Andy Shevchenko <andy@kernel.org>, linux-hardening@vger.kernel.org,
	Richard Weinberger <richard@nod.at>,
	Anton Ivanov <anton.ivanov@cambridgegreys.com>,
	Johannes Berg <johannes@sipsolutions.net>,
	Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
	Jason Wang <jasowang@redhat.com>, kernel test robot <lkp@intel.com>,
	Nathan Chancellor <nathan@kernel.org>,
	Azeem Shaikh <azeemshaikh38@gmail.com>,
	linux-kernel@vger.kernel.org, linux-um@lists.infradead.org
Subject: Re: [PATCH v3 1/4] string: Redefine strscpy_pad() as a macro
Message-ID: <20240207003224.gzatchm47gy3aihv@google.com>
References: <20240206142027.make.107-kees@kernel.org>
 <20240206142221.2208763-1-keescook@chromium.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240206142221.2208763-1-keescook@chromium.org>

Hi,

On Tue, Feb 06, 2024 at 06:22:16AM -0800, Kees Cook wrote:
> In preparation for making strscpy_pad()'s 3rd argument optional, redefine
> it as a macro. This also has the benefit of allowing greater FORITFY
> introspection, as it couldn't see into the strscpy() nor the memset()
> within strscpy_pad().
>
> Cc: Andy Shevchenko <andy@kernel.org>
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/string.h | 33 +++++++++++++++++++++++++++++++--
>  lib/string_helpers.c   | 34 ----------------------------------
>  2 files changed, 31 insertions(+), 36 deletions(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index ab148d8dbfc1..03f59cf7fe72 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -70,8 +70,37 @@ extern char * strncpy(char *,const char *, __kernel_size_t);
>  ssize_t strscpy(char *, const char *, size_t);
>  #endif
>
> -/* Wraps calls to strscpy()/memset(), no arch specific code required */
> -ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> +/**
> + * strscpy_pad() - Copy a C-string into a sized buffer
> + * @dest: Where to copy the string to
> + * @src: Where to copy the string from
> + * @count: Size of destination buffer
> + *
> + * Copy the string, or as much of it as fits, into the dest buffer. The
> + * behavior is undefined if the string buffers overlap. The destination
> + * buffer is always %NUL terminated, unless it's zero-sized.
> + *
> + * If the source string is shorter than the destination buffer, zeros
> + * the tail of the destination buffer.
> + *
> + * For full explanation of why you may want to consider using the
> + * 'strscpy' functions please see the function docstring for strscpy().
> + *
> + * Returns:
> + * * The number of characters copied (not including the trailing %NULs)
> + * * -E2BIG if count is 0 or @src was truncated.
> + */
> +#define strscpy_pad(dest, src, count)	({			\
> +	char *__dst = (dest);						\
> +	const char *__src = (src);					\
> +	const size_t __count = (count);					\
> +	ssize_t __wrote;						\
> +									\
> +	__wrote = strscpy(__dst, __src, __count);			\
> +	if (__wrote >= 0 && __wrote < __count)				\
> +		memset(__dst + __wrote + 1, 0, __count - __wrote - 1);	\
> +	__wrote;							\
> +})
>
>  #ifndef __HAVE_ARCH_STRCAT
>  extern char * strcat(char *, const char *);
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 7713f73e66b0..606c3099013f 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -825,40 +825,6 @@ char **devm_kasprintf_strarray(struct device *dev, const char *prefix, size_t n)
>  }
>  EXPORT_SYMBOL_GPL(devm_kasprintf_strarray);
>
> -/**
> - * strscpy_pad() - Copy a C-string into a sized buffer
> - * @dest: Where to copy the string to
> - * @src: Where to copy the string from
> - * @count: Size of destination buffer
> - *
> - * Copy the string, or as much of it as fits, into the dest buffer.  The
> - * behavior is undefined if the string buffers overlap.  The destination
> - * buffer is always %NUL terminated, unless it's zero-sized.
> - *
> - * If the source string is shorter than the destination buffer, zeros
> - * the tail of the destination buffer.
> - *
> - * For full explanation of why you may want to consider using the
> - * 'strscpy' functions please see the function docstring for strscpy().
> - *
> - * Returns:
> - * * The number of characters copied (not including the trailing %NUL)
> - * * -E2BIG if count is 0 or @src was truncated.
> - */
> -ssize_t strscpy_pad(char *dest, const char *src, size_t count)
> -{
> -	ssize_t written;
> -
> -	written = strscpy(dest, src, count);
> -	if (written < 0 || written == count - 1)
> -		return written;
> -
> -	memset(dest + written + 1, 0, count - written - 1);
> -
> -	return written;
> -}
> -EXPORT_SYMBOL(strscpy_pad);

Yep, looks good. This is reminiscent of strtomem and strtomem_pad.

Reviewed-by: Justin Stitt <justinstitt@google.com>

> -
>  /**
>   * skip_spaces - Removes leading whitespace from @str.
>   * @str: The string to be stripped.
> --
> 2.34.1
>

Thanks
Justin