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);