linux-kernel - Re: [PATCH] can: ucan: Correct the size parameter

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: <2f33170a-f7bb-47dd-8cb7-15c055dabc83@wanadoo.fr>
Date: Tue, 18 Feb 2025 11:22:11 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: Matt Jan <zoo868e@...il.com>, Marc Kleine-Budde <mkl@...gutronix.de>
Cc: syzbot+d7d8c418e8317899e88c@...kaller.appspotmail.com,
 linux-kernel@...r.kernel.org, linux-can@...r.kernel.org
Subject: Re: [PATCH] can: ucan: Correct the size parameter

On 18/02/2025 at 04:04, Matt Jan wrote:
> According to the comment, the size parameter is only required when
> @dst is not an array, or when the copy needs to be smaller than
> sizeof(@dst). Since the source is a `union ucan_ctl_payload`, the
> correct size should be sizeof(union ucan_ctl_payload).

While this fix is correct, I think that the root cause is that
up->ctl_msg_buffer->raw is not NUL terminated.

Because of that, a local copy was added, just to reintroduce the NUL
terminating byte.

I think it is better to just directly terminate up->ctl_msg_buffer->raw
and get rid of the firmware_str local variable and the string copy.

So, what about this:

diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c
index 39a63b7313a4..268085453d24 100644
--- a/drivers/net/can/usb/ucan.c
+++ b/drivers/net/can/usb/ucan.c
@@ -186,7 +186,7 @@ union ucan_ctl_payload {
         */
        struct ucan_ctl_cmd_get_protocol_version cmd_get_protocol_version;

-       u8 raw[128];
+       char fw_info[128];
 } __packed;

 enum {
@@ -424,18 +424,19 @@ static int ucan_ctrl_command_out(struct ucan_priv *up,
                               UCAN_USB_CTL_PIPE_TIMEOUT);
 }

-static int ucan_device_request_in(struct ucan_priv *up,
-                                 u8 cmd, u16 subcmd, u16 datalen)
+static void ucan_get_fw_info(struct ucan_priv *up, char *fw_info,
size_t size)
 {
-       return usb_control_msg(up->udev,
-                              usb_rcvctrlpipe(up->udev, 0),
-                              cmd,
-                              USB_DIR_IN | USB_TYPE_VENDOR |
USB_RECIP_DEVICE,
-                              subcmd,
-                              0,
-                              up->ctl_msg_buffer,
-                              datalen,
-                              UCAN_USB_CTL_PIPE_TIMEOUT);
+       int ret;
+
+       ret = usb_control_msg(up->udev, usb_rcvctrlpipe(up->udev, 0),
+                             UCAN_DEVICE_GET_FW_STRING,
+                             USB_DIR_IN | USB_TYPE_VENDOR |
USB_RECIP_DEVICE,
+                             0, 0, fw_info, size - 1,
+                             UCAN_USB_CTL_PIPE_TIMEOUT);
+       if (ret > 0)
+               fw_info[ret] = '\0';
+       else
+               strcpy(fw_info, "unknown");
 }

 /* Parse the device information structure reported by the device and
@@ -1314,7 +1315,6 @@ static int ucan_probe(struct usb_interface *intf,
        u8 in_ep_addr;
        u8 out_ep_addr;
        union ucan_ctl_payload *ctl_msg_buffer;
-       char firmware_str[sizeof(union ucan_ctl_payload) + 1];

        udev = interface_to_usbdev(intf);

@@ -1527,17 +1527,6 @@ static int ucan_probe(struct usb_interface *intf,
         */
        ucan_parse_device_info(up, &ctl_msg_buffer->cmd_get_device_info);

-       /* just print some device information - if available */
-       ret = ucan_device_request_in(up, UCAN_DEVICE_GET_FW_STRING, 0,
-                                    sizeof(union ucan_ctl_payload));
-       if (ret > 0) {
-               /* copy string while ensuring zero termination */
-               strscpy(firmware_str, up->ctl_msg_buffer->raw,
-                       sizeof(union ucan_ctl_payload) + 1);
-       } else {
-               strcpy(firmware_str, "unknown");
-       }
-
        /* device is compatible, reset it */
        ret = ucan_ctrl_command_out(up, UCAN_COMMAND_RESET, 0, 0);
        if (ret < 0)
@@ -1555,7 +1544,10 @@ static int ucan_probe(struct usb_interface *intf,

        /* initialisation complete, log device info */
        netdev_info(up->netdev, "registered device\n");
-       netdev_info(up->netdev, "firmware string: %s\n", firmware_str);
+       ucan_get_fw_info(up, up->ctl_msg_buffer->fw_info,
+                        sizeof(up->ctl_msg_buffer->fw_info));
+       netdev_info(up->netdev, "firmware string: %s\n",
+                   up->ctl_msg_buffer->fw_info);

        /* success */
        return 0;


> Signed-off-by: Matt Jan <zoo868e@...il.com>
> Reported-by: syzbot+d7d8c418e8317899e88c@...kaller.appspotmail.com
> Fixes: b3e40fc85735 ("USB: usb_parse_endpoint: ignore reserved bits")

I saw that the bot bisected it to this commit, but I doubt that this is
correct. In


https://lore.kernel.org/linux-can/20250217-spectral-cordial-booby-968731-mkl@pengutronix.de/

Marc pointed out that the issue came from 7fdaf8966aae ("can: ucan: use
strscpy() to instead of strncpy()"). And I agree with Marc's analysis.

> ---
>  drivers/net/can/usb/ucan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c
> index 39a63b7313a4..1ccef00388ae 100644
> --- a/drivers/net/can/usb/ucan.c
> +++ b/drivers/net/can/usb/ucan.c
> @@ -1533,7 +1533,7 @@ static int ucan_probe(struct usb_interface *intf,
>  	if (ret > 0) {
>  		/* copy string while ensuring zero termination */
>  		strscpy(firmware_str, up->ctl_msg_buffer->raw,
> -			sizeof(union ucan_ctl_payload) + 1);
> +			sizeof(union ucan_ctl_payload));
>  	} else {
>  		strcpy(firmware_str, "unknown");
>  	}

Yours sincerely,
Vincent Mailhol