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: <20250218-accurate-viridian-manatee-6f2878-mkl@pengutronix.de>
Date: Tue, 18 Feb 2025 08:37:30 +0100
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Vincent Mailhol <mailhol.vincent@...adoo.fr>
Cc: Matt Jan <zoo868e@...il.com>, 
	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 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   |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)