Re: [PATCH] scsi: target: Replace deprecated strncpy() with strscpy()

[Kees Cook <kees@kernel.org>]

On Wed, Feb 26, 2025 at 01:10:03PM +0100, Thorsten Blum wrote:
> Since strncpy() is deprecated for NUL-terminated destination buffers,
> use strscpy() instead.
> 
> Compile-tested only.
> 
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  drivers/target/target_core_configfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index c40217f44b1b..446682f900e4 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -143,7 +143,7 @@ static ssize_t target_core_item_dbroot_store(struct config_item *item,
>  	}
>  	filp_close(fp, NULL);
>  
> -	strncpy(db_root, db_root_stage, read_bytes);
> +	strscpy(db_root, db_root_stage, read_bytes);
>  	pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
>  
>  	r = read_bytes;

When doing strncpy() to strscpy() conversions, please include details
of your analysis for several things:

- why is it safe to be NUL-terminated?
- why is NUL padding needed/not needed?
- why is the maximum length argument correct?

In this case, db_root is used with "%s" format strings, so we know it is
expected to be NUL-terminated. Additionally, it is _only_ ever used with
"5s" format strings, so padding is not needed.

As for length, read_bytes will always be less than DB_ROOT_LEN because
it is explicitly tested for against "count" which, enforced by the
SETATTR API, will always be the number of valid bytes in "page", and
snprintf() will be limited to DB_ROOT_LEN. (snprintf may return more
than its length argument, but because of the "count" check, this should
be impossible.)

While you're in this file, though, can you replace the other strncpy
that is used in target_init_dbroot() also?

-Kees

-- 
Kees Cook