From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp5-g21.free.fr (smtp5-g21.free.fr [212.27.42.5]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9B9F443CE6F for ; Wed, 30 Sep 2026 07:49:25 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=212.27.42.5 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790754570; cv=none; b=P1pf8zqZG/qHzehPtOCcoYwt6wiwglv2R8DNo94ObRw91AB7Nmek3xFQB7m0Brus5lFa/cymWDCEZ+P40jne0F4xVwBR9i+rJMshPQdfE3aaKEggxVVg8YCjnYRsNbRciv/fjY0Mxcr8hjhrYGUVeUZLhQXHX4jNB11lC59TM7w= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790754570; c=relaxed/simple; bh=Rx1obGIWo1Vd6vwECSscJejE0rio0wrwDP9A5YuYQkA=; h=Date:From:To:Cc:Message-ID:In-Reply-To:Subject:MIME-Version: Content-Type; b=TCjODa7l+CkZwfxaHbserE7Ylw5C6veOaxaw5D56vmrcXqACo46QKITjYeQaZnL4w2akU8pZ62kJ+FEK2oWgYsv8nZoK44t+82PA6BEv5yXo7fEN/V20JxX5LFtBHb5Z3k2NJwaVPJQvZdf9Glbn9Fe4P9lrUiqqJOg/d06lTxo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=free.fr; spf=pass smtp.mailfrom=free.fr; dkim=pass (2048-bit key) header.d=free.fr header.i=@free.fr header.b=jKqbi5hd; arc=none smtp.client-ip=212.27.42.5 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=free.fr Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=free.fr Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=free.fr header.i=@free.fr header.b="jKqbi5hd" Received: from zimbra65-e11.priv.proxad.net (unknown [172.20.243.215]) by smtp5-g21.free.fr (Postfix) with ESMTP id 793656013F; Wed, 30 Sep 2026 09:49:20 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=free.fr; s=smtp-20201208; t=1790754560; bh=Rx1obGIWo1Vd6vwECSscJejE0rio0wrwDP9A5YuYQkA=; h=Date:From:To:Cc:In-Reply-To:Subject:From; b=jKqbi5hdyGsfbC1n+WMP9qymGH1xuvg6H4Baqpi+b87pWLRVNIT+gWRNEAkgcfyRN DbsTvXVpPaykfXRBJMbnKELRLMWO234G+ovsOCLNVMX40AG9YSKTsQPOqkrLqc01ly YHgs52IN6rJu8KO+XWMMkqfd/6oNsaXPUbPAo4+GF4S4c53LHPb8x/z+1dX4FRn2w3 3+v0oX4MKaRg3XgUisQN5xf/3iLCsd2nflHKNPNe9OnkuEpPVz0+zmipggPZ0PQ1Mz oPr2fmoHtRBaPI9R+/JEIrRMn4FWKrsqeXDuEOly7Y1a/0hEokipv2XKof4CGBOWWV 9UEq1RDrbAcow== Date: Wed, 30 Sep 2026 09:49:19 +0200 (CEST) From: =?utf-8?Q?St=C3=A9phane?= Grosjean To: Vincent Mailhol , Oliver Hartkopp , Marc Kleine-Budde Cc: linux-can@vger.kernel.org, Oleksij Rempel , sashiko-reviews@lists.linux.dev Message-ID: <559842824.808060158.1790754559351.JavaMail.root@zimbra65-e11.priv.proxad.net> In-Reply-To: <20260929145401.6FD0C1F000FF@smtp.kernel.org> Subject: Re: [PATCH v2] can: peak_usb: Add bus error reporting for the PCAN-USB FD family Precedence: bulk X-Mailing-List: linux-can@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Mailer: Zimbra 7.2.0-GA2598 (ZimbraWebClient - GC151 (Linux)/7.2.0-GA2598) X-Authenticated-User: stephane.grosjean@free.fr Hi, The NULL check was added to protect against a malformed PUCAN_MSG_ERROR record reporting channel 1 on a single-channel adapter. =C2=A0 Such a value cannot be generated by compliant PCAN-USB FD firmware and therefore indicates corrupted or malicious input. The driver already returns -EINVAL and aborts URB decoding for other protocol violations, for example when the reported channel index exceeds the bounds of the channel array. This change follows the same policy. =C2=A0 Since the input stream is already invalid at this point, I believe rejecting the entire URB is the correct behaviour rather than attempting to continue parsing subsequent records from untrusted data. Best regards, -- St=C3=A9phane ----- Mail original ----- > Thank you for your contribution! Sashiko AI review found 1 potential > issue(s) to consider: > - [High] can: peak_usb: returning -EINVAL for bogus error reports > drops valid URB packets > -- >=20 > commit 6ff05518907e7981e5fff27d40e8a2458d563751 > Author: St=C3=A9phane Grosjean >=20 > can: peak_usb: Add bus error reporting for the PCAN-USB FD family >=20 > CAN bus error reporting is currently missing for all PEAK-System > USB-to-CAN FD devices. Add support for reporting bus errors by > enabling > bus error notifications in the firmware for each CAN channel. >=20 > Link to v1: > https://patch.msgid.link/20260929-peak_usb-canfd-berr-v1-1-7abd585f877f@p= eak-system.fr >=20 > > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c > > b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c > > index 0d46f4ce5dca7..98f6943469b33 100644 > > --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c > > +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c > > @@ -661,17 +661,71 @@ static int pcan_usb_fd_decode_error(struct > > pcan_usb_fd_if *usb_if, > > =09struct pucan_error_msg *er =3D (struct pucan_error_msg *)rx_msg; > > =09struct pcan_usb_fd_device *pdev; > > =09struct peak_usb_device *dev; > > +=09struct can_frame *cf; > > +=09struct sk_buff *skb; > > +=09u8 can_err_tx =3D 0; > > =20 > > =09if (pucan_ermsg_get_channel(er) >=3D ARRAY_SIZE(usb_if->dev)) > > =09=09return -EINVAL; > > =20 > > +=09/* Guard against bogus channel 1 reports from single-channel > > adapters */ > > =09dev =3D usb_if->dev[pucan_ermsg_get_channel(er)]; > > +=09if (!dev) > > +=09=09return -EINVAL; >=20 > [Severity: High] > Does returning -EINVAL here cause valid URB packets to be dropped? >=20 > The caller pcan_usb_fd_decode_buf() handles a negative return value > by > aborting the URB decoding entirely and jumping to the fail label: >=20 > =09case PUCAN_MSG_ERROR: > =09=09err =3D pcan_usb_fd_decode_error(usb_if, rx_msg); > =09=09if (err < 0) > =09=09=09goto fail; > =09=09break; >=20 > Because USB bulk transfers pack multiple CAN records together, > aborting > the loop discards any subsequent valid CAN frames in the same URB. > This appears to lead to silent data loss under normal operation. >=20 > Should this return 0 instead of -EINVAL to safely ignore the hardware > quirk > without dropping the rest of the buffer? >=20 > -- > Sashiko AI review =C2=B7 > https://sashiko.dev/#/patchset/20260929-peak_usb-canfd-berr-v2-1-1a700579= 7c38@peak-system.fr?part=3D1 >=20