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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202502281204.CE15A6BA35@keescook>
Date: Fri, 28 Feb 2025 12:10:43 -0800
From: Kees Cook <kees@...nel.org>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: "Martin K. Petersen" <martin.petersen@...cle.com>,
	linux-hardening@...r.kernel.org, linux-scsi@...r.kernel.org,
	target-devel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] scsi: target: Replace deprecated strncpy() with strscpy()

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@...r.kernel.org
> Signed-off-by: Thorsten Blum <thorsten.blum@...ux.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