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: <b174cc40-d08b-42a5-89fc-9fdac2b15ea9@wanadoo.fr>
Date: Tue, 18 Feb 2025 23:26:01 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: Marc Kleine-Budde <mkl@...gutronix.de>
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 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