Re: [PATCH v2 5/5] can: peak_usb: harden PCAN-USB message validation

["Stéphane Grosjean" <stephane.grosjean@free.fr>]

----- Mail original -----
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 (2) · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to
> each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let
> patchwork
> know by including "pw-bot: cr" as a separate line at the end of your
> reply
> (one such reply per series is enough).
> 
> - [High] The patch says it rejects PCAN-USB messages that would cause
>   reads outside the received USB data, but one path is still
>   unchecked:…
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but the
> encoder
>   and decoder disagree on the wire layout for RTR frames with SRR
>   set.
> - [Medium] This problem wasn't introduced by this patch, but when the
>   decoder rejects a message, the diagnostic dump prints the whole rx…
> 
> --
> 
> can: peak_usb: harden PCAN-USB message validation
> 
> This patch adds bounds checks to the PCAN-USB rx decoder. The
> status_len
> byte at the start of each record and the SRR private-id byte after a
> data
> frame are now consumed only if they fit inside the received USB
> buffer.
> Malformed messages are rejected with -EINVAL.
> 
> > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c
> > b/drivers/net/can/usb/peak_usb/pcan_usb.c
> > index 169c00d93463c..56ddd134bae0f 100644
> > --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> > +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> > @@ -696,8 +696,11 @@ static int pcan_usb_decode_data(struct
> > pcan_usb_msg_context *mc, u8 status_len)
> >  		mc->ptr += rec_len;
> >  
> >  		/* Ignore next byte (client private id) if SRR bit is set */
> > -		if (can_id_flags & PCAN_USB_TX_SRR)
> > +		if (can_id_flags & PCAN_USB_TX_SRR) {
> > +			if ((mc->ptr + 1) > mc->end)
> > +				goto decode_failed;
> >  			mc->ptr++;
> > +		}
> 
> [Severity: Medium]
> This is a pre-existing issue, but is the private-id byte also present
> for
> RTR frames?
> 
> In pcan_usb_decode_data(), this skip and the new check exist only in
> the
> non-RTR branch:
> 
> 	if (status_len & PCAN_USB_STATUSLEN_RTR) {
> 		cf->can_id |= CAN_RTR_FLAG;
> 	} else {
> 		...
> 		if (can_id_flags & PCAN_USB_TX_SRR) {
> 
> But pcan_usb_encode_msg() appends the writer id for RTR frames as
> well:
> 
> 	/* can data */
> 	if (!(cf->can_id & CAN_RTR_FLAG)) {
> 		memcpy(pc, cf->data, cf->len);
> 		pc += cf->len;
> 	}
> 
> 	/* SRR bit needs a writer id (useless here) */
> 	if (can_id_flags & PCAN_USB_TX_SRR)
> 		*pc++ = 0x80;
> 
> SRR is set whenever CAN_CTRLMODE_LOOPBACK is enabled. Suppose the
> firmware's
> loopback echo of an RTR frame includes that byte. Then mc->ptr is
> left
> pointing at it, and pcan_usb_decode_msg() reads 0x80 as the next
> status_len
> byte.
> 
> PCAN_USB_STATUSLEN_INTERNAL is clear in 0x80, so the byte would be
> decoded
> as a bogus standard-ID data frame. Every later record in the URB
> would then
> be parsed out of phase. That gives phantom frames, or an early
> -EINVAL that
> drops the rest of the URB.
> 
> The real RTR echo format can't be confirmed from the source. Both the
> encoder and decoder lines came from the same commit 3a7939495ce8, so
> the
> difference may be intentional. The later reads are bounds-checked, so
> no
> out-of-bounds access results.
> 
> Could the firmware behaviour be checked either way?
> 
> > @@ -733,14 +736,19 @@ static int pcan_usb_decode_msg(struct
> > peak_usb_device *dev, u8 *ibuf, u32 lbuf)
> >  	int err;
> >  
> >  	for (err = 0; mc.rec_idx < mc.rec_cnt && !err; mc.rec_idx++) {
> > -		u8 sl = *mc.ptr++;
> > -
> > -		/* handle status and error frames here */
> > -		if (sl & PCAN_USB_STATUSLEN_INTERNAL) {
> > -			err = pcan_usb_decode_status(&mc, sl);
> > -		/* handle normal can frames here */
> > +		/* check if status_len byte can be read next */
> > +		if (mc.ptr >= mc.end) {
> > +			err = -EINVAL;
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but this new -EINVAL path
> (and
> the truncated SRR one above) leads to the diagnostic dump in
> peak_usb_read_bulk_callback():
> 
> 	err = dev->adapter->dev_decode_buf(dev, urb);
> 	if (err)
> 		pcan_dump_mem("received usb message",
> 			      urb->transfer_buffer,
> 			      urb->transfer_buffer_length);
> 
> Doesn't this dump the full rx_buffer_size allocation instead of only
> the
> urb->actual_length bytes the device sent?
> 
> The rx buffers come from kmalloc() in peak_usb_start() and are not
> zeroed.
> After a short malformed transfer such as [00 01 00] (truncated CAN
> id), the
> bytes past the received data are uninitialized slab contents.
> 
> pcan_dump_mem() prints them at KERN_INFO with print_hex_dump(), with
> no rate
> limit and no debug-only guard. Earlier decode failures could already
> reach
> this dump.
> 
> Would it make sense to pass urb->actual_length here instead?
> 
> >  		} else {
> > -			err = pcan_usb_decode_data(&mc, sl);
> > +			u8 sl = *mc.ptr++;
> > +
> > +			/* handle status and error frames here */
> > +			if (sl & PCAN_USB_STATUSLEN_INTERNAL) {
> > +				err = pcan_usb_decode_status(&mc, sl);
> 
> [Severity: High]
> Can a truncated bus event record still cause an out-of-bounds read
> here?
> 
> pcan_usb_decode_status() only checks that the function and number
> bytes
> fit, and then advances past them:
> 
> 	if ((mc->ptr + 2) > mc->end)
> 		return -EINVAL;
> 
> 	f = mc->ptr[PCAN_USB_CMD_FUNC];
> 	n = mc->ptr[PCAN_USB_CMD_NUM];
> 	mc->ptr += PCAN_USB_CMD_ARGS;
> 
> At this point mc->ptr can equal mc->end. Suppose the timestamp bit is
> clear,
> f is PCAN_USB_REC_BUSEVT, and n is PCAN_USB_ERR_CNT_DEC or
> PCAN_USB_ERR_CNT_INC. Then the call goes straight to
> pcan_usb_handle_bus_evt():
> 
> 		pdev->bec.rxerr = mc->ptr[1];
> 		pdev->bec.txerr = mc->ptr[2];
> 
> These read mc->end + 1 and mc->end + 2 with no check. The later
> (mc->ptr + rec_len) > mc->end check in pcan_usb_decode_status() runs
> only
> after these reads. rec_len also comes from the device and can be 0.
> 
> For example, the 5 byte transfer [00 01 40 05 00] passes the new
> status_len
> check. It then reads ibuf[6] and ibuf[7], past actual_length, and is
> reported as successfully decoded.
> 
> Now take a full 64 byte transfer whose last record is a bus event
> header
> ending at the last byte. The reads land on ibuf[65] and ibuf[66],
> which are
> outside the kmalloc-64 rx buffer allocated in peak_usb_start().
> 
> Those bytes are stored in pdev->bec. Userspace can read them through
> pcan_usb_get_berr_counter() (IFLA_CAN_BERR_COUNTER) and in the CAN
> error
> frames built by pcan_usb_decode_error().
> 
> The commit message says the patch rejects buffers "whose contents
> would
> otherwise lead to accesses outside the received USB data area".
> Should
> pcan_usb_handle_bus_evt() check something like (mc->ptr + 3) >
> mc->end
> before reading these bytes, rather than relying on rec_len from the
> device?
> 
> >  			/* handle normal can frames here */
> > +			} else {
> > +				err = pcan_usb_decode_data(&mc, sl);
> > +			}
> >  		}
> >  	}
> 
> [ ... ]
> 
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-canfd_check_channel_idx-v2-0-f7b772929791%40peak-system.fr
> 

Thanks for the review. All the issues reported here are addressed in v3,
which also adds fixes for related issues in the PCAN-USB Pro driver and
in the core probe/RX paths.

pw-bot: cr