linux-kernel - Re: [PATCH] staging: greybus: Remove unnecessary NUL-termination checks

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: <8a68ab78-cf18-4937-a8b7-fb0fa41c9d53@ieee.org>
Date: Mon, 31 Mar 2025 18:31:15 -0500
From: Alex Elder <elder@...e.org>
To: Thorsten Blum <thorsten.blum@...ux.dev>, Viresh Kumar
 <vireshk@...nel.org>, Johan Hovold <johan@...nel.org>,
 Alex Elder <elder@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: greybus-dev@...ts.linaro.org, linux-staging@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: greybus: Remove unnecessary NUL-termination
 checks

On 3/31/25 1:39 PM, Thorsten Blum wrote:
> Commit 18f44de63f88 ("staging: greybus: change strncpy() to
> strscpy_pad()") didn't remove the now unnecessary NUL-termination
> checks. Unlike strncpy(), strscpy_pad() guarantees that the destination
> buffer is NUL-terminated, making the checks obsolete. Remove them.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@...ux.dev>

This looks good!  Although the NUL-termination check isn't
needed, it isn't safe to ignore the return value of strscpy_pad().
More below.

In all cases, it looks like strscpy_pad() (and not just strscpy())
is the correct thing to call, because the pad bytes are passed
either to user space, or supplied as part of a Greybus request
message.

> ---
>   drivers/staging/greybus/fw-management.c | 39 +------------------------
>   1 file changed, 1 insertion(+), 38 deletions(-)
> 
> diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c
> index a47385175582..852c0830261f 100644
> --- a/drivers/staging/greybus/fw-management.c
> +++ b/drivers/staging/greybus/fw-management.c
> @@ -125,16 +125,6 @@ static int fw_mgmt_interface_fw_version_operation(struct fw_mgmt *fw_mgmt,
>   
>   	strscpy_pad(fw_info->firmware_tag, response.firmware_tag);
>   
> -	/*
> -	 * The firmware-tag should be NULL terminated, otherwise throw error but
> -	 * don't fail.
> -	 */
> -	if (fw_info->firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
> -		dev_err(fw_mgmt->parent,
> -			"fw-version: firmware-tag is not NULL terminated\n");
> -		fw_info->firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] = '\0';
> -	}

Interesting this didn't return an error, while others below did.

The sizes of the arrays passed to strscpy_pad() are not necessarily
the same, so you should check for its return value.
fw_info->firmware_tag is GB_FIRMWARE_U_TAG_MAX_SIZE=10 bytes
response.firmware_tag is GB_FIRMWARE_TAG_MAX_SIZE=10 bytes also,
but these could theoretically change independently.

> -
>   	return 0;
>   }
>   
> @@ -154,15 +144,6 @@ static int fw_mgmt_load_and_validate_operation(struct fw_mgmt *fw_mgmt,
>   	request.load_method = load_method;
>   	strscpy_pad(request.firmware_tag, tag);
>   

Here the maximum length of the tag is GB_FIRMWARE_U_TAG_MAX_SIZE
bytes, and it may or may not be NUL-terminated.  The size of
request.firmware_tag is GB_FIRMWARE_TAG_MAX_SIZE.  Again you
can't be sure they're the same, and even if they are, the source
could be truncated.

> -	/*
> -	 * The firmware-tag should be NULL terminated, otherwise throw error and
> -	 * fail.
> -	 */
> -	if (request.firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
> -		dev_err(fw_mgmt->parent, "load-and-validate: firmware-tag is not NULL terminated\n");
> -		return -EINVAL;
> -	}
> -
>   	/* Allocate ids from 1 to 255 (u8-max), 0 is an invalid id */
>   	ret = ida_alloc_range(&fw_mgmt->id_map, 1, 255, GFP_KERNEL);
>   	if (ret < 0) {
> @@ -250,15 +231,6 @@ static int fw_mgmt_backend_fw_version_operation(struct fw_mgmt *fw_mgmt,
>   
>   	strscpy_pad(request.firmware_tag, fw_info->firmware_tag);
>   

The size of request.firmware_tag is GB_FIRMWARE_TAG_MAX_SIZE bytes.
The size of fw_info->firmware_tag is GB_FIRMWARE_U_TAG_MAX_SIZE bytes.
Check the return value for -E2BIG.

> -	/*
> -	 * The firmware-tag should be NULL terminated, otherwise throw error and
> -	 * fail.
> -	 */
> -	if (request.firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
> -		dev_err(fw_mgmt->parent, "backend-version: firmware-tag is not NULL terminated\n");
> -		return -EINVAL;
> -	}
> -
>   	ret = gb_operation_sync(connection,
>   				GB_FW_MGMT_TYPE_BACKEND_FW_VERSION, &request,
>   				sizeof(request), &response, sizeof(response));
> @@ -301,16 +273,7 @@ static int fw_mgmt_backend_fw_update_operation(struct fw_mgmt *fw_mgmt,
>   	struct gb_fw_mgmt_backend_fw_update_request request;
>   	int ret;
>   
> -	ret = strscpy_pad(request.firmware_tag, tag);
> -
> -	/*
> -	 * The firmware-tag should be NULL terminated, otherwise throw error and
> -	 * fail.
> -	 */
> -	if (ret == -E2BIG) {
> -		dev_err(fw_mgmt->parent, "backend-update: firmware-tag is not NULL terminated\n");
> -		return -EINVAL;
> -	}
> +	strscpy_pad(request.firmware_tag, tag);

The size of request.firmware_tag is GB_FIRMWARE_TAG_MAX_SIZE bytes.
The maximum size of tag is GB_FIRMWARE_U_TAG_MAX_SIZE bytes, and it
may or may not be NUL-terminated.  So this case should stay as-is,
and check for -E2BIG.

					-Alex

>   	/* Allocate ids from 1 to 255 (u8-max), 0 is an invalid id */
>   	ret = ida_alloc_range(&fw_mgmt->id_map, 1, 255, GFP_KERNEL);