[PATCH] staging: greybus: fw-management: validate firmware_tag is NUL-terminated

[PATCH] staging: greybus: fw-management: validate firmware_tag is NUL-terminated

[Maritu Hone]

fw_mgmt_ioctl() copies struct fw_mgmt_ioc_get_backend_version,
struct fw_mgmt_ioc_intf_load_and_validate and
struct fw_mgmt_ioc_backend_fw_update directly from userspace via
copy_from_user(). Each of these contains a fixed-size
firmware_tag[GB_FIRMWARE_U_TAG_MAX_SIZE] array that is never checked
for NUL-termination before being used.

While the array is eventually copied into a Greybus request via
strscpy_pad(), which safely bounds its read and always NUL-terminates
its destination, three error paths print the original, unterminated
userspace buffer directly with "%s":

  fw_mgmt_backend_fw_version_operation():
    dev_err(..., "failed to get version of %s backend firmware (%d)\n",
            fw_info->firmware_tag, ret);
    dev_err(..., "Firmware with tag %s is not supported by Interface\n",
            fw_info->firmware_tag);

  fw_mgmt_backend_fw_update_operation():
    dev_err(..., "backend %s firmware update request failed (%d)\n",
            tag, ret);

If a caller supplies exactly GB_FIRMWARE_U_TAG_MAX_SIZE non-NUL bytes,
"%s" has no terminator inside the array and reads past it into
adjacent kernel stack memory until it happens to find a zero byte,
leaking that memory into dmesg.

Reject the ioctl up front when firmware_tag is not NUL-terminated,
mirroring the existing check already used for the equivalent
Greybus-module-supplied field in fw_download_find_firmware().

Fixes: 013e66537273 ("greybus: fw-management: Add firmware-management protocol driver")
Signed-off-by: Maritu Hone <marehone12@gmail.com>
---
 drivers/staging/greybus/fw-management.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c
index 5d01da6b6eb9..2ee609dd67fb 100644
--- a/drivers/staging/greybus/fw-management.c
+++ b/drivers/staging/greybus/fw-management.c
@@ -419,6 +419,11 @@ static int fw_mgmt_ioctl(struct fw_mgmt *fw_mgmt, unsigned int cmd,
 				   sizeof(backend_fw_info)))
 			return -EFAULT;
 
+		if (strnlen(backend_fw_info.firmware_tag,
+			    GB_FIRMWARE_U_TAG_MAX_SIZE) ==
+		    GB_FIRMWARE_U_TAG_MAX_SIZE)
+			return -EINVAL;
+
 		ret = fw_mgmt_backend_fw_version_operation(fw_mgmt,
 							   &backend_fw_info);
 		if (ret)
@@ -433,6 +438,11 @@ static int fw_mgmt_ioctl(struct fw_mgmt *fw_mgmt, unsigned int cmd,
 		if (copy_from_user(&intf_load, buf, sizeof(intf_load)))
 			return -EFAULT;
 
+		if (strnlen(intf_load.firmware_tag,
+			    GB_FIRMWARE_U_TAG_MAX_SIZE) ==
+		    GB_FIRMWARE_U_TAG_MAX_SIZE)
+			return -EINVAL;
+
 		ret = fw_mgmt_load_and_validate_operation(fw_mgmt,
 				intf_load.load_method, intf_load.firmware_tag);
 		if (ret)
@@ -457,6 +467,11 @@ static int fw_mgmt_ioctl(struct fw_mgmt *fw_mgmt, unsigned int cmd,
 				   sizeof(backend_update)))
 			return -EFAULT;
 
+		if (strnlen(backend_update.firmware_tag,
+			    GB_FIRMWARE_U_TAG_MAX_SIZE) ==
+		    GB_FIRMWARE_U_TAG_MAX_SIZE)
+			return -EINVAL;
+
 		ret = fw_mgmt_backend_fw_update_operation(fw_mgmt,
 							  backend_update.firmware_tag);
 		if (ret)
-- 
2.43.0

Re: [PATCH] staging: greybus: fw-management: validate firmware_tag is NUL-terminated

[Dan Carpenter]

On Mon, Nov 02, 2026 at 05:12:21PM +0300, Maritu Hone wrote:
> fw_mgmt_ioctl() copies struct fw_mgmt_ioc_get_backend_version,
> struct fw_mgmt_ioc_intf_load_and_validate and
> struct fw_mgmt_ioc_backend_fw_update directly from userspace via
> copy_from_user(). Each of these contains a fixed-size
> firmware_tag[GB_FIRMWARE_U_TAG_MAX_SIZE] array that is never checked
> for NUL-termination before being used.
> 
> While the array is eventually copied into a Greybus request via
> strscpy_pad(), which safely bounds its read and always NUL-terminates
> its destination, three error paths print the original, unterminated
> userspace buffer directly with "%s":
> 
>   fw_mgmt_backend_fw_version_operation():
>     dev_err(..., "failed to get version of %s backend firmware (%d)\n",
>             fw_info->firmware_tag, ret);

The strscpy_pad() function returns -E2BIG if the string is too large
so if "fw_info->firmware_tag" is too large then it will never reach this
printk().

>     dev_err(..., "Firmware with tag %s is not supported by Interface\n",
>             fw_info->firmware_tag);
> 
>   fw_mgmt_backend_fw_update_operation():
>     dev_err(..., "backend %s firmware update request failed (%d)\n",
>             tag, ret);

Same for this one as well so these are all false positives.

> 
> If a caller supplies exactly GB_FIRMWARE_U_TAG_MAX_SIZE non-NUL bytes,
> "%s" has no terminator inside the array and reads past it into
> adjacent kernel stack memory until it happens to find a zero byte,
> leaking that memory into dmesg.
> 
> Reject the ioctl up front when firmware_tag is not NUL-terminated,
> mirroring the existing check already used for the equivalent
> Greybus-module-supplied field in fw_download_find_firmware().
> 
> Fixes: 013e66537273 ("greybus: fw-management: Add firmware-management protocol driver")
> Signed-off-by: Maritu Hone <marehone12@gmail.com>

If this were a real bug then this wouldn't be the correct Fixes
tag.

regards,
dan carpenter