[PATCH] can: peak_usb: validate response and receive record extents

[PATCH] can: peak_usb: validate response and receive record extents

[Pengpeng Hou]

Both PCAN-USB Pro record walkers read data_type before proving that a
record header remains, and the synchronous response walker advances by a
declared record size without bounding it by actual_length.

Require the type byte and complete declared record in both command-
response and receive paths before dispatch or pointer advancement.

Fixes: d8a199355f8f ("can: usb: PEAK-System Technik PCAN-USB Pro specific part")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index b6be8c19e537f..aaa5f4f7db191 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -244,6 +244,7 @@ static int pcan_usb_pro_wait_rsp(struct peak_usb_device *dev,
 	for (i = 0; !err && i < PCAN_USBPRO_RSP_SUBMIT_MAX; i++) {
 		struct pcan_usb_pro_msg rsp;
 		union pcan_usb_pro_rec *pr;
+		u8 *msg_end;
 		u32 r, rec_cnt;
 		u16 rec_len;
 		u8 *pc;
@@ -270,11 +271,18 @@ static int pcan_usb_pro_wait_rsp(struct peak_usb_device *dev,
 
 		pc = pcan_msg_init(&rsp, pum->u.rec_buffer,
 			actual_length);
+		msg_end = pum->u.rec_buffer + actual_length;
 
 		rec_cnt = le32_to_cpu(*rsp.u.rec_cnt);
 
 		/* loop on records stored into message */
 		for (r = 0; r < rec_cnt; r++) {
+			if (pc >= msg_end) {
+				netdev_err(dev->netdev,
+					   "record header exceeds response\n");
+				break;
+			}
+
 			pr = (union pcan_usb_pro_rec *)pc;
 			rec_len = pcan_usb_pro_sizeof_rec[pr->data_type];
 			if (!rec_len) {
@@ -284,6 +292,11 @@ static int pcan_usb_pro_wait_rsp(struct peak_usb_device *dev,
 					      actual_length);
 				break;
 			}
+			if (rec_len > msg_end - pc) {
+				netdev_err(dev->netdev,
+					   "record exceeds response\n");
+				break;
+			}
 
 			/* check if response corresponds to request */
 			if (pr->data_type != req_data_type)
@@ -727,7 +740,14 @@ static int pcan_usb_pro_decode_buf(struct peak_usb_device *dev, struct urb *urb)
 	msg_end = urb->transfer_buffer + urb->actual_length;
 	rec_cnt = le16_to_cpu(*usb_msg.u.rec_cnt_rd);
 	for (; rec_cnt > 0; rec_cnt--) {
-		union pcan_usb_pro_rec *pr = (union pcan_usb_pro_rec *)rec_ptr;
+		union pcan_usb_pro_rec *pr;
+
+		if (rec_ptr >= msg_end) {
+			err = -EBADMSG;
+			break;
+		}
+
+		pr = (union pcan_usb_pro_rec *)rec_ptr;
 		u16 sizeof_rec = pcan_usb_pro_sizeof_rec[pr->data_type];
 
 		if (!sizeof_rec) {

base-commit: 08dbfad3f5040f5bdb6c529da20d6d4e81fefd72
-- 
2.50.1

Re: [PATCH] can: peak_usb: validate response and receive record extents

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] OOB Read/Write due to unvalidated DLC in CAN frame parsing.
--

commit 3a2ad6057d438a3e936d788c2a9ec4229e3c427c
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
can: peak_usb: validate response and receive record extents

This commit adds validation to ensure that both record headers and complete
declared records are present in the response and receive paths before
processing them. This prevents out-of-bounds access when parsing USB message
buffers.

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> index b6be8c19e537f..aaa5f4f7db191 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c

[ ... ]

> @@ -727,7 +740,14 @@ static int pcan_usb_pro_decode_buf(struct peak_usb_device *dev, struct urb *urb)
>  	msg_end = urb->transfer_buffer + urb->actual_length;
>  	rec_cnt = le16_to_cpu(*usb_msg.u.rec_cnt_rd);
>  	for (; rec_cnt > 0; rec_cnt--) {
> -		union pcan_usb_pro_rec *pr = (union pcan_usb_pro_rec *)rec_ptr;
> +		union pcan_usb_pro_rec *pr;
> +
> +		if (rec_ptr >= msg_end) {
> +			err = -EBADMSG;
> +			break;
> +		}
> +
> +		pr = (union pcan_usb_pro_rec *)rec_ptr;

[Severity: High]
This is a pre-existing issue, but there appears to be an out-of-bounds read
and write in how these records are processed later in
pcan_usb_pro_handle_canmsg().

When handling a CAN message record from an untrusted USB device:

drivers/net/can/usb/peak_usb/pcan_usb_pro.c:pcan_usb_pro_handle_canmsg() {
    ...
    can_frame->len = rx->len & 0x0f;
    ...
    } else {
        memcpy(can_frame->data, rx->data, can_frame->len);
    ...
}

If the device sends a message where rx->len & 0x0f is greater than 8, can
this result in an out-of-bounds write since can_frame->data is exactly 8
bytes long?

Could this also pass a malformed frame to userspace with a length greater
than 8, violating the classic CAN API contract?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830135942.19449-1-pengpeng@iscas.ac.cn?part=1