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

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

[Thorsten Blum]

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@linux.dev>
---
 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';
-	}
-
 	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);
 
-	/*
-	 * 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 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);
 
 	/* 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);

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

[Alex Elder]

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@linux.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);

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

[Thorsten Blum]

On 1. Apr 2025, at 01:31, Alex Elder wrote:
> On 3/31/25 1:39 PM, Thorsten Blum wrote:
>> @@ -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.

Should I keep it that way when checking for a truncated firmware tag or
should this also fail like the others?

Thanks,
Thorsten

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

[Alex Elder]

On 4/1/25 2:51 PM, Thorsten Blum wrote:
> On 1. Apr 2025, at 01:31, Alex Elder wrote:
>> On 3/31/25 1:39 PM, Thorsten Blum wrote:
>>> @@ -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.
> 
> Should I keep it that way when checking for a truncated firmware tag or
> should this also fail like the others?
> 
> Thanks,
> Thorsten
> 
I don't know the answer right now, and I don't have time at
the moment to investigate.  Just keep that logic the way it
is, and make your other fix.

					-Alex