drivers/net/can/usb/ucan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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).
Signed-off-by: Matt Jan <zoo868e@gmail.com>
Reported-by: syzbot+d7d8c418e8317899e88c@syzkaller.appspotmail.com
Fixes: b3e40fc85735 ("USB: usb_parse_endpoint: ignore reserved bits")
---
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");
}
--
2.25.1On 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@gmail.com>
> Reported-by: syzbot+d7d8c418e8317899e88c@syzkaller.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
On 18.02.2025 11:22:11, Vincent Mailhol wrote:
> 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);
We could also use the:
printf("%.*s", sizeof(up->ctl_msg_buffer->fw_info), up->ctl_msg_buffer->fw_info);
format string trick to only print a limited number of chars of the given
string. But I'm also fine with your solution. Either way, please send a
proper patch :)
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On 18/02/2025 at 16:37, Marc Kleine-Budde wrote:
> On 18.02.2025 11:22:11, Vincent Mailhol wrote:
>> 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);
>
> We could also use the:
>
> printf("%.*s", sizeof(up->ctl_msg_buffer->fw_info), up->ctl_msg_buffer->fw_info);
>
> format string trick to only print a limited number of chars of the given
> string.
Indeed. But after the renaming of ucan_device_request_in() into
ucan_get_fw_info(), it makes slightly more sense to me to have this new
function to handle the string NUL termination logic rather than to
deffer it to the format string.
But thanks for the suggestion.
> But I'm also fine with your solution. Either way, please send a
> proper patch :)
Will do so right now!
Yours sincerely,
Vincent Mailhol
On 18.02.2025 23:26:01, Vincent Mailhol wrote:
> >> @@ -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);
> >
> > We could also use the:
> >
> > printf("%.*s", sizeof(up->ctl_msg_buffer->fw_info), up->ctl_msg_buffer->fw_info);
> >
> > format string trick to only print a limited number of chars of the given
> > string.
>
> Indeed. But after the renaming of ucan_device_request_in() into
> ucan_get_fw_info(), it makes slightly more sense to me to have this new
> function to handle the string NUL termination logic rather than to
> deffer it to the format string.
ACK, makes sense!
> But thanks for the suggestion.
>
> > But I'm also fine with your solution. Either way, please send a
> > proper patch :)
>
> Will do so right now!
Thanks,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
© 2016 - 2025 Red Hat, Inc.