[PATCH] can: kvaser_usb: leaf: Fix infinite loop on zero-length cmd

[PATCH] can: kvaser_usb: leaf: Fix infinite loop on zero-length cmd

[pip-izony]

From: Seungjin Bae <eeodqql09@gmail.com>

The `kvaser_usb_leaf_read_bulk_callback()` function parse received
command buffers from the device. The firmware may insert zero-length
placeholder commands to handle alignment with the USB endpoint's
wMaxPacketSize.

The driver attempts to skip these placeholders by aligning the buffer
position `pos` to the next packet boundary using `round_up()` function.

However, if zero-length command is found exactly on a packet boundary
(i.e., `pos` is a multiple of wMaxPacketSize, including 0), `round_up`
function will return the unchanged value of `pos`. This prevents `pos`
to be increased, causing an infinite loop in the parsing logic.

I fixed this in the function by using `pos + 1` instead. This ensures
that even if `pos` is on a boundary, the calculation is based on
`pos + 1`, forcing `round_up()` to always return the next aligned
boundary.

Fixes: 7259124eac7d ("can: kvaser_usb: Split driver into kvaser_usb_core.c and kvaser_usb_leaf.c")
Signed-off-by: Seungjin Bae <eeodqql09@gmail.com>
---
 drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index c29828a94ad0..4da6d4ba4e1e 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -1732,7 +1732,7 @@ static void kvaser_usb_leaf_read_bulk_callback(struct kvaser_usb *dev,
 		 * number of events in case of a heavy rx load on the bus.
 		 */
 		if (cmd->len == 0) {
-			pos = round_up(pos, le16_to_cpu
+			pos = round_up(pos + 1, le16_to_cpu
 						(dev->bulk_in->wMaxPacketSize));
 			continue;
 		}
-- 
2.43.0

Re: [PATCH] can: kvaser_usb: leaf: Fix infinite loop on zero-length cmd

[Marc Kleine-Budde]

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

On 22.10.2025 20:39:09, pip-izony wrote:
> From: Seungjin Bae <eeodqql09@gmail.com>
> 
> The `kvaser_usb_leaf_read_bulk_callback()` function parse received
> command buffers from the device. The firmware may insert zero-length
> placeholder commands to handle alignment with the USB endpoint's
> wMaxPacketSize.
> 
> The driver attempts to skip these placeholders by aligning the buffer
> position `pos` to the next packet boundary using `round_up()`
> function.

What about the round_up() in kvaser_usb_leaf_wait_cmd()? Is it also
affected by this problem?

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 v2] can: kvaser_usb: leaf: Fix potential infinite loop in command parsers

[pip-izony]

From: Seungjin Bae <eeodqql09@gmail.com>

`kvaser_usb_leaf_wait_cmd()` and `kvaser_usb_leaf_read_bulk_callback`
functions contain logic to handle zero-length commands. These commands
are used to align data to the USB endpoint's wMaxPacketSize boundary.

The driver attempts to skip these placeholders by aligning the buffer
position `pos` to the next packet boundary using `round_up()` function.

However, if zero-length command is found exactly on a packet boundary
(i.e., `pos` is a multiple of wMaxPacketSize, including 0), `round_up`
function will return the unchanged value of `pos`. This prevents `pos`
to be increased, causing an infinite loop in the parsing logic.

This patch fixes this in the function by using `pos + 1` instead.
This ensures that even if `pos` is on a boundary, the calculation is
based on `pos + 1`, forcing `round_up()` to always return the next
aligned boundary.

Fixes: 7259124eac7d ("can: kvaser_usb: Split driver into kvaser_usb_core.c and kvaser_usb_leaf.c")
Signed-off-by: Seungjin Bae <eeodqql09@gmail.com>
---
 v1 -> v2: Apply the same infinite loop fix to kvaser_usb_leaf_wait_cmd()
 
 drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index c29828a94ad0..1167d38344f1 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -685,7 +685,7 @@ static int kvaser_usb_leaf_wait_cmd(const struct kvaser_usb *dev, u8 id,
 			 * for further details.
 			 */
 			if (tmp->len == 0) {
-				pos = round_up(pos,
+				pos = round_up(pos + 1,
 					       le16_to_cpu
 						(dev->bulk_in->wMaxPacketSize));
 				continue;
@@ -1732,7 +1732,7 @@ static void kvaser_usb_leaf_read_bulk_callback(struct kvaser_usb *dev,
 		 * number of events in case of a heavy rx load on the bus.
 		 */
 		if (cmd->len == 0) {
-			pos = round_up(pos, le16_to_cpu
+			pos = round_up(pos + 1, le16_to_cpu
 						(dev->bulk_in->wMaxPacketSize));
 			continue;
 		}
-- 
2.43.0

[PATCH v2] can: kvaser_usb: leaf: Fix potential infinite loop in command parsers

[pip-izony]

From: Seungjin Bae <eeodqql09@gmail.com>

The `kvaser_usb_leaf_wait_cmd()` and `kvaser_usb_leaf_read_bulk_callback`
functions contain logic to zero-length commands. These commands are used
to align data to the USB endpoint's wMaxPacketSize boundary.

The driver attempts to skip these placeholders by aligning the buffer
position `pos` to the next packet boundary using `round_up()` function.

However, if zero-length command is found exactly on a packet boundary
(i.e., `pos` is a multiple of wMaxPacketSize, including 0), `round_up`
function will return the unchanged value of `pos`. This prevents `pos`
to be increased, causing an infinite loop in the parsing logic.

This patch fixes this in the function by using `pos + 1` instead.
This ensures that even if `pos` is on a boundary, the calculation is
based on `pos + 1`, forcing `round_up()` to always return the next
aligned boundary.

Fixes: 7259124eac7d ("can: kvaser_usb: Split driver into kvaser_usb_core.c and kvaser_usb_leaf.c")
Signed-off-by: Seungjin Bae <eeodqql09@gmail.com>
---
 v1 -> v2: Apply the same infinite loop fix to kvaser_usb_leaf_wait_cmd()
 
 drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index c29828a94ad0..1167d38344f1 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -685,7 +685,7 @@ static int kvaser_usb_leaf_wait_cmd(const struct kvaser_usb *dev, u8 id,
 			 * for further details.
 			 */
 			if (tmp->len == 0) {
-				pos = round_up(pos,
+				pos = round_up(pos + 1,
 					       le16_to_cpu
 						(dev->bulk_in->wMaxPacketSize));
 				continue;
@@ -1732,7 +1732,7 @@ static void kvaser_usb_leaf_read_bulk_callback(struct kvaser_usb *dev,
 		 * number of events in case of a heavy rx load on the bus.
 		 */
 		if (cmd->len == 0) {
-			pos = round_up(pos, le16_to_cpu
+			pos = round_up(pos + 1, le16_to_cpu
 						(dev->bulk_in->wMaxPacketSize));
 			continue;
 		}
-- 
2.43.0

Re: [PATCH v2] can: kvaser_usb: leaf: Fix potential infinite loop in command parsers

[Jimmy Assarsson]

Hi Seungjin,

Thanks for fixing this!
I'll do some testing in the beginning of next week.
Which Kvaser device did you use when you discovered the problem?

Best regards,
jimmy

On 10/23/25 18:27, pip-izony wrote:
> From: Seungjin Bae <eeodqql09@gmail.com>
> 
> The `kvaser_usb_leaf_wait_cmd()` and `kvaser_usb_leaf_read_bulk_callback`
> functions contain logic to zero-length commands. These commands are used
> to align data to the USB endpoint's wMaxPacketSize boundary.
> 
> The driver attempts to skip these placeholders by aligning the buffer
> position `pos` to the next packet boundary using `round_up()` function.
> 
> However, if zero-length command is found exactly on a packet boundary
> (i.e., `pos` is a multiple of wMaxPacketSize, including 0), `round_up`
> function will return the unchanged value of `pos`. This prevents `pos`
> to be increased, causing an infinite loop in the parsing logic.
> 
> This patch fixes this in the function by using `pos + 1` instead.
> This ensures that even if `pos` is on a boundary, the calculation is
> based on `pos + 1`, forcing `round_up()` to always return the next
> aligned boundary.
> 
> Fixes: 7259124eac7d ("can: kvaser_usb: Split driver into kvaser_usb_core.c and kvaser_usb_leaf.c")
> Signed-off-by: Seungjin Bae <eeodqql09@gmail.com>
> ---
>   v1 -> v2: Apply the same infinite loop fix to kvaser_usb_leaf_wait_cmd()
>   
>   drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> index c29828a94ad0..1167d38344f1 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> @@ -685,7 +685,7 @@ static int kvaser_usb_leaf_wait_cmd(const struct kvaser_usb *dev, u8 id,
>   			 * for further details.
>   			 */
>   			if (tmp->len == 0) {
> -				pos = round_up(pos,
> +				pos = round_up(pos + 1,
>   					       le16_to_cpu
>   						(dev->bulk_in->wMaxPacketSize));
>   				continue;
> @@ -1732,7 +1732,7 @@ static void kvaser_usb_leaf_read_bulk_callback(struct kvaser_usb *dev,
>   		 * number of events in case of a heavy rx load on the bus.
>   		 */
>   		if (cmd->len == 0) {
> -			pos = round_up(pos, le16_to_cpu
> +			pos = round_up(pos + 1, le16_to_cpu
>   						(dev->bulk_in->wMaxPacketSize));
>   			continue;
>   		}

Re: [PATCH v2] can: kvaser_usb: leaf: Fix potential infinite loop in command parsers

[Jimmy Assarsson]

On 10/26/25 14:26, Jimmy Assarsson wrote:
> Hi Seungjin,
> 
> Thanks for fixing this!
> I'll do some testing in the beginning of next week.
> Which Kvaser device did you use when you discovered the problem?
> 
> Best regards,
> jimmy

Hi Seungjin,

I've not been able to reproduce this problem, when testing with the
latest firmware on multiple different devices.

If the next command in the firmware packet queue, doesn't fit within the
current endpoint transaction (wMaxPacketSize), the firmware will
terminate the transaction with a zero byte. The driver then interprets
this as a zero-length command, and skip to the next transaction.

The firmware is responsible to insert a "zero termination byte" only
when there is already one or more packets in the current transaction.
Since all commands have even lengths (4,8,10,12,16,20,24,30,32 bytes)
and the wMaxPacketSize is also even (64 bytes or 512 bytes, depending on
the device), I cannot see a situation where the zero termination byte
would be inserted exactly at the wMaxPacketSize boundary.

Can you please provide which Kvaser device and firmware you use:
   lsusb -d 0bfd:
   ethtool -i can0

Best regards,
jimmy

> On 10/23/25 18:27, pip-izony wrote:
>> From: Seungjin Bae <eeodqql09@gmail.com>
>>
>> The `kvaser_usb_leaf_wait_cmd()` and `kvaser_usb_leaf_read_bulk_callback`
>> functions contain logic to zero-length commands. These commands are used
>> to align data to the USB endpoint's wMaxPacketSize boundary.
>>
>> The driver attempts to skip these placeholders by aligning the buffer
>> position `pos` to the next packet boundary using `round_up()` function.
>>
>> However, if zero-length command is found exactly on a packet boundary
>> (i.e., `pos` is a multiple of wMaxPacketSize, including 0), `round_up`
>> function will return the unchanged value of `pos`. This prevents `pos`
>> to be increased, causing an infinite loop in the parsing logic.
>>
>> This patch fixes this in the function by using `pos + 1` instead.
>> This ensures that even if `pos` is on a boundary, the calculation is
>> based on `pos + 1`, forcing `round_up()` to always return the next
>> aligned boundary.
>>
>> Fixes: 7259124eac7d ("can: kvaser_usb: Split driver into 
>> kvaser_usb_core.c and kvaser_usb_leaf.c")
>> Signed-off-by: Seungjin Bae <eeodqql09@gmail.com>
>> ---
>>   v1 -> v2: Apply the same infinite loop fix to 
>> kvaser_usb_leaf_wait_cmd()
>>   drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/ 
>> drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
>> index c29828a94ad0..1167d38344f1 100644
>> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
>> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
>> @@ -685,7 +685,7 @@ static int kvaser_usb_leaf_wait_cmd(const struct 
>> kvaser_usb *dev, u8 id,
>>                * for further details.
>>                */
>>               if (tmp->len == 0) {
>> -                pos = round_up(pos,
>> +                pos = round_up(pos + 1,
>>                              le16_to_cpu
>>                           (dev->bulk_in->wMaxPacketSize));
>>                   continue;
>> @@ -1732,7 +1732,7 @@ static void 
>> kvaser_usb_leaf_read_bulk_callback(struct kvaser_usb *dev,
>>            * number of events in case of a heavy rx load on the bus.
>>            */
>>           if (cmd->len == 0) {
>> -            pos = round_up(pos, le16_to_cpu
>> +            pos = round_up(pos + 1, le16_to_cpu
>>                           (dev->bulk_in->wMaxPacketSize));
>>               continue;
>>           }
> 
> 

Re: [PATCH v2] can: kvaser_usb: leaf: Fix potential infinite loop in command parsers

[Jimmy Assarsson]

On 10/29/25 20:06, Seungjin Bae wrote:
> Hello Jimmy,
> 
> Thank you for trying to reproduce the issue and for the detailed
> explanation of the firmware logic.
> 
> You are correct that this issue would not occur with the standard Kvaser
> firmware. The vulnerability I reported wasn’t found by running code
> on an actual Kvaser device.
> 
> Instead, I discovered this issue by performing analysis on the driver
> code using a symbolic execution tool. This tool demonstrated that
> the driver contains a vulnerable code path.
> 
> My goal was to show that if a malicious device is connected—one that
> sends a specifically crafted packet sequence not normally produced by
> the firmware—the driver code will process it incorrectly and cause
> the kernel panic.
> 
> So regretfully, I don’t have any hardware information yet.

Sorry, I overlooked the title, and assumed this was a problem you had
encountered with a real device.

Patch LGTM and tested OK with actual Kvaser devices:
Reviewed-by: Jimmy Assarsson <extja@kvaser.com>
Tested-by: Jimmy Assarsson <extja@kvaser.com>

Thanks!
/jimmy


> Thank you for your precious time.
> 
> Best,
> Seungjin Bae
> 
> 2025년 10월 29일 (수) 오전 6:53, Jimmy Assarsson <extja@kvaser.com 
> <mailto:extja@kvaser.com>>님이 작성:
> 
>     On 10/26/25 14:26, Jimmy Assarsson wrote:
>      > Hi Seungjin,
>      >
>      > Thanks for fixing this!
>      > I'll do some testing in the beginning of next week.
>      > Which Kvaser device did you use when you discovered the problem?
>      >
>      > Best regards,
>      > jimmy
> 
>     Hi Seungjin,
> 
>     I've not been able to reproduce this problem, when testing with the
>     latest firmware on multiple different devices.
> 
>     If the next command in the firmware packet queue, doesn't fit within the
>     current endpoint transaction (wMaxPacketSize), the firmware will
>     terminate the transaction with a zero byte. The driver then interprets
>     this as a zero-length command, and skip to the next transaction.
> 
>     The firmware is responsible to insert a "zero termination byte" only
>     when there is already one or more packets in the current transaction.
>     Since all commands have even lengths (4,8,10,12,16,20,24,30,32 bytes)
>     and the wMaxPacketSize is also even (64 bytes or 512 bytes, depending on
>     the device), I cannot see a situation where the zero termination byte
>     would be inserted exactly at the wMaxPacketSize boundary.
> 
>     Can you please provide which Kvaser device and firmware you use:
>         lsusb -d 0bfd:
>         ethtool -i can0
> 
>     Best regards,
>     jimmy