[syzbot] [can?] WARNING in ucan_probe

[syzbot] [can?] WARNING in ucan_probe

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    496659003dac Merge tag 'i2c-for-6.14-rc3' of git://git.ker..
git tree:       upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=11012bf8580000
kernel config:  https://syzkaller.appspot.com/x/.config?x=c776e555cfbdb82d
dashboard link: https://syzkaller.appspot.com/bug?extid=d7d8c418e8317899e88c
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=14f7b9b0580000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=155602e4580000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/c1675d5fc116/disk-49665900.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/0342ce7d0bc9/vmlinux-49665900.xz
kernel image: https://storage.googleapis.com/syzbot-assets/5ce5b4978fc4/bzImage-49665900.xz

The issue was bisected to:

commit b3e40fc85735b787ce65909619fcd173107113c2
Author: Oliver Neukum <oneukum@suse.com>
Date:   Thu May 2 11:51:40 2024 +0000

    USB: usb_parse_endpoint: ignore reserved bits

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=11c65bf8580000
final oops:     https://syzkaller.appspot.com/x/report.txt?x=13c65bf8580000
console output: https://syzkaller.appspot.com/x/log.txt?x=15c65bf8580000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+d7d8c418e8317899e88c@syzkaller.appspotmail.com
Fixes: b3e40fc85735 ("USB: usb_parse_endpoint: ignore reserved bits")

------------[ cut here ]------------
strnlen: detected buffer overflow: 129 byte read of buffer size 128
WARNING: CPU: 0 PID: 9 at lib/string_helpers.c:1033 __fortify_report+0x9d/0xb0 lib/string_helpers.c:1032
Modules linked in:
CPU: 0 UID: 0 PID: 9 Comm: kworker/0:1 Not tainted 6.14.0-rc2-syzkaller-00281-g496659003dac #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 12/27/2024
Workqueue: usb_hub_wq hub_event
RIP: 0010:__fortify_report+0x9d/0xb0 lib/string_helpers.c:1032
Code: 84 ed 48 8b 33 48 c7 c0 a0 ae 80 8c 48 c7 c1 c0 ae 80 8c 48 0f 44 c8 48 c7 c7 20 ac 80 8c 4c 89 fa 4d 89 f0 e8 04 dd 8b fc 90 <0f> 0b 90 90 5b 41 5e 41 5f 5d c3 cc cc cc cc 0f 1f 40 00 90 90 90
RSP: 0018:ffffc900000e6b50 EFLAGS: 00010246
RAX: e8edca93825f5800 RBX: ffffffff8c80ab68 RCX: ffff88801c2f8000
RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000000
RBP: 0000000000000000 R08: ffffffff81817e32 R09: fffffbfff1d3a614
R10: dffffc0000000000 R11: fffffbfff1d3a614 R12: dffffc0000000000
R13: 1ffff9200001cd84 R14: 0000000000000080 R15: 0000000000000081
FS:  0000000000000000(0000) GS:ffff8880b8600000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055d6c3b85e50 CR3: 0000000078508000 CR4: 00000000003526f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 __fortify_panic+0x9/0x10 lib/string_helpers.c:1039
 _Z7strnlenPKcU25pass_dynamic_object_size1m include/linux/fortify-string.h:235 [inline]
 _Z13sized_strscpyPcU25pass_dynamic_object_size1PKcU25pass_dynamic_object_size1m include/linux/fortify-string.h:309 [inline]
 ucan_probe+0x195e/0x1980 drivers/net/can/usb/ucan.c:1535
 usb_probe_interface+0x641/0xbb0 drivers/usb/core/driver.c:396
 really_probe+0x2b9/0xad0 drivers/base/dd.c:658
 __driver_probe_device+0x1a2/0x390 drivers/base/dd.c:800
 driver_probe_device+0x50/0x430 drivers/base/dd.c:830
 __device_attach_driver+0x2d6/0x530 drivers/base/dd.c:958
 bus_for_each_drv+0x24e/0x2e0 drivers/base/bus.c:462
 __device_attach+0x333/0x520 drivers/base/dd.c:1030
 bus_probe_device+0x189/0x260 drivers/base/bus.c:537
 device_add+0x856/0xbf0 drivers/base/core.c:3665
 usb_set_configuration+0x1976/0x1fb0 drivers/usb/core/message.c:2210
 usb_generic_driver_probe+0x88/0x140 drivers/usb/core/generic.c:250
 usb_probe_device+0x1b8/0x380 drivers/usb/core/driver.c:291
 really_probe+0x2b9/0xad0 drivers/base/dd.c:658
 __driver_probe_device+0x1a2/0x390 drivers/base/dd.c:800
 driver_probe_device+0x50/0x430 drivers/base/dd.c:830
 __device_attach_driver+0x2d6/0x530 drivers/base/dd.c:958
 bus_for_each_drv+0x24e/0x2e0 drivers/base/bus.c:462
 __device_attach+0x333/0x520 drivers/base/dd.c:1030
 bus_probe_device+0x189/0x260 drivers/base/bus.c:537
 device_add+0x856/0xbf0 drivers/base/core.c:3665
 usb_new_device+0x104a/0x19a0 drivers/usb/core/hub.c:2652
 hub_port_connect drivers/usb/core/hub.c:5523 [inline]
 hub_port_connect_change drivers/usb/core/hub.c:5663 [inline]
 port_event drivers/usb/core/hub.c:5823 [inline]
 hub_event+0x2d6d/0x5150 drivers/usb/core/hub.c:5905
 process_one_work kernel/workqueue.c:3236 [inline]
 process_scheduled_works+0xabe/0x18e0 kernel/workqueue.c:3317
 worker_thread+0x870/0xd30 kernel/workqueue.c:3398
 kthread+0x7a9/0x920 kernel/kthread.c:464
 ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:148
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
 </TASK>


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

Re: [syzbot] [can?] WARNING in ucan_probe

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]

On 17.02.2025 03:55:16, syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    496659003dac Merge tag 'i2c-for-6.14-rc3' of git://git.ker..
> git tree:       upstream
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=11012bf8580000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=c776e555cfbdb82d
> dashboard link: https://syzkaller.appspot.com/bug?extid=d7d8c418e8317899e88c
> compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=14f7b9b0580000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=155602e4580000
> 
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/c1675d5fc116/disk-49665900.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/0342ce7d0bc9/vmlinux-49665900.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/5ce5b4978fc4/bzImage-49665900.xz
> 
> The issue was bisected to:
> 
> commit b3e40fc85735b787ce65909619fcd173107113c2
> Author: Oliver Neukum <oneukum@suse.com>
> Date:   Thu May 2 11:51:40 2024 +0000
> 
>     USB: usb_parse_endpoint: ignore reserved bits

I think the issue was introduced in: 7fdaf8966aae ("can: ucan: use
strscpy() to instead of strncpy()"). I'm preparing a fix.

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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH] can: ucan: Correct the size parameter

[Matt Jan]

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.1

Re: [PATCH] can: ucan: Correct the size parameter

[Vincent Mailhol]

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@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

Re: [PATCH] can: ucan: Correct the size parameter

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4816 bytes --]

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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: ucan: Correct the size parameter

[Vincent Mailhol]

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

Re: [PATCH] can: ucan: Correct the size parameter

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1530 bytes --]

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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH] can: ucan: fix out of bound read in strscpy() source

[Vincent Mailhol]

Commit 7fdaf8966aae ("can: ucan: use strscpy() to instead of strncpy()")
unintentionally introduced a one byte out of bound read on strscpy()'s
source argument (which is kind of ironic knowing that strscpy() is meant
to be a more secure alternative :)).

Let's consider below buffers:

  dest[len + 1]; /* will be NUL terminated */
  src[len]; /* may not be NUL terminated */

When doing:

  strncpy(dest, src, len);
  dest[len] = '\0';

strncpy() will read up to len bytes from src.

On the other hand:

  strscpy(dest, src, len + 1);

will read up to len + 1 bytes from src, that is to say, an out of bound
read of one byte will occur on src if it is not NUL terminated. Note
that the src[len] byte is never copied, but strscpy() still needs to
read it to check whether a truncation occurred or not.

This exact pattern happened in ucan.

The root cause is that the source is not NUL terminated. Instead of
doing a copy in a local buffer, directly NUL terminate it as soon as
usb_control_msg() returns. With this, the local firmware_str[] variable
can be removed.

On top of this do a couple refactors:

  - ucan_ctl_payload->raw is only used for the firmware string, so
    rename it to ucan_ctl_payload->fw_str and change its type from u8 to
    char.

  - ucan_device_request_in() is only used to retrieve the firmware
    string, so rename it to ucan_get_fw_str() and refactor it to make it
    directly handle all the string termination logic.

Reported-by: syzbot+d7d8c418e8317899e88c@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-can/67b323a4.050a0220.173698.002b.GAE@google.com/
Fixes: 7fdaf8966aae ("can: ucan: use strscpy() to instead of strncpy()")
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/usb/ucan.c | 43 ++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c
index 39a63b7313a4..07406daf7c88 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];
+	u8 fw_str[128];
 } __packed;
 
 enum {
@@ -424,18 +424,20 @@ 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_str(struct ucan_priv *up, char *fw_str, 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_str, size - 1,
+			      UCAN_USB_CTL_PIPE_TIMEOUT);
+	if (ret > 0)
+		fw_str[ret] = '\0';
+	else
+		strscpy(fw_str, "unknown", size);
 }
 
 /* Parse the device information structure reported by the device and
@@ -1314,7 +1316,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 +1528,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 +1545,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_str(up, up->ctl_msg_buffer->fw_str,
+			sizeof(up->ctl_msg_buffer->fw_str));
+	netdev_info(up->netdev, "firmware string: %s\n",
+		    up->ctl_msg_buffer->fw_str);
 
 	/* success */
 	return 0;
-- 
2.45.3

Re: [PATCH] can: ucan: fix out of bound read in strscpy() source

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2313 bytes --]

On 18.02.2025 23:32:28, Vincent Mailhol wrote:
> Commit 7fdaf8966aae ("can: ucan: use strscpy() to instead of strncpy()")
> unintentionally introduced a one byte out of bound read on strscpy()'s
> source argument (which is kind of ironic knowing that strscpy() is meant
> to be a more secure alternative :)).
> 
> Let's consider below buffers:
> 
>   dest[len + 1]; /* will be NUL terminated */
>   src[len]; /* may not be NUL terminated */
> 
> When doing:
> 
>   strncpy(dest, src, len);
>   dest[len] = '\0';
> 
> strncpy() will read up to len bytes from src.
> 
> On the other hand:
> 
>   strscpy(dest, src, len + 1);
> 
> will read up to len + 1 bytes from src, that is to say, an out of bound
> read of one byte will occur on src if it is not NUL terminated. Note
> that the src[len] byte is never copied, but strscpy() still needs to
> read it to check whether a truncation occurred or not.
> 
> This exact pattern happened in ucan.
> 
> The root cause is that the source is not NUL terminated. Instead of
> doing a copy in a local buffer, directly NUL terminate it as soon as
> usb_control_msg() returns. With this, the local firmware_str[] variable
> can be removed.
> 
> On top of this do a couple refactors:
> 
>   - ucan_ctl_payload->raw is only used for the firmware string, so
>     rename it to ucan_ctl_payload->fw_str and change its type from u8 to
>     char.
> 
>   - ucan_device_request_in() is only used to retrieve the firmware
>     string, so rename it to ucan_get_fw_str() and refactor it to make it
>     directly handle all the string termination logic.
> 
> Reported-by: syzbot+d7d8c418e8317899e88c@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-can/67b323a4.050a0220.173698.002b.GAE@google.com/
> Fixes: 7fdaf8966aae ("can: ucan: use strscpy() to instead of strncpy()")
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

Wow! Thanks for the detailed analysis and description of the problem!

Applied to linux-can.

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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]