RE: [PATCH net] can: peak_usb: add range checking in decode operations

["Stéphane Grosjean" <s.grosjean@peak-system.com>]

Hello Dan,

Don't know if this patch is still relevant, but:

- there is absolutely no reason for the device firmware to provide a channel index greater than or equal to 2, because the IP core of these USB devices handles 2 channels only. Anyway, these changes are correct.
- considering the verification of the length "cfd->len" on the other hand, this one comes directly from can_send() via dev_queue_xmit() AFAIK and it seems to me that the underlying driver can assume that its value is smaller than 64.

Regards,
---
Stéphane Grosjean
PEAK-System France
132, rue André Bisiaux
F-54320 MAXEVILLE
Tél : +(33) 9.72.54.51.97



De : Dan Carpenter <dan.carpenter@oracle.com>
Envoyé : jeudi 13 août 2020 16:06
À : Wolfgang Grandegger <wg@grandegger.com>; Stéphane Grosjean <s.grosjean@peak-system.com>
Cc : Marc Kleine-Budde <mkl@pengutronix.de>; David S. Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Andri Yngvason <andri.yngvason@marel.com>; Oliver Hartkopp <socketcan@hartkopp.net>; linux-can@vger.kernel.org <linux-can@vger.kernel.org>; netdev@vger.kernel.org <netdev@vger.kernel.org>; kernel-janitors@vger.kernel.org <kernel-janitors@vger.kernel.org>
Objet : [PATCH net] can: peak_usb: add range checking in decode operations

These values come from skb->data so Smatch considers them untrusted.  I
believe Smatch is correct but I don't have a way to test this.

The usb_if->dev[] array has 2 elements but the index is in the 0-15
range without checks.  The cfd->len can be up to 255 but the maximum
valid size is CANFD_MAX_DLEN (64) so that could lead to memory
corruption.

Fixes: 0a25e1f4f185 ("can: peak_usb: add support for PEAK new CANFD USB adapters")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 48 +++++++++++++++++-----
 1 file changed, 37 insertions(+), 11 deletions(-)

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 47cc1ff5b88e..dee3e689b54d 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -468,12 +468,18 @@ static int pcan_usb_fd_decode_canmsg(struct pcan_usb_fd_if *usb_if,
                                      struct pucan_msg *rx_msg)
 {
         struct pucan_rx_msg *rm = (struct pucan_rx_msg *)rx_msg;
-       struct peak_usb_device *dev = usb_if->dev[pucan_msg_get_channel(rm)];
-       struct net_device *netdev = dev->netdev;
+       struct peak_usb_device *dev;
+       struct net_device *netdev;
         struct canfd_frame *cfd;
         struct sk_buff *skb;
         const u16 rx_msg_flags = le16_to_cpu(rm->flags);

+       if (pucan_msg_get_channel(rm) >= ARRAY_SIZE(usb_if->dev))
+               return -ENOMEM;
+
+       dev = usb_if->dev[pucan_msg_get_channel(rm)];
+       netdev = dev->netdev;
+
         if (rx_msg_flags & PUCAN_MSG_EXT_DATA_LEN) {
                 /* CANFD frame case */
                 skb = alloc_canfd_skb(netdev, &cfd);
@@ -519,15 +525,21 @@ static int pcan_usb_fd_decode_status(struct pcan_usb_fd_if *usb_if,
                                      struct pucan_msg *rx_msg)
 {
         struct pucan_status_msg *sm = (struct pucan_status_msg *)rx_msg;
-       struct peak_usb_device *dev = usb_if->dev[pucan_stmsg_get_channel(sm)];
-       struct pcan_usb_fd_device *pdev =
-                       container_of(dev, struct pcan_usb_fd_device, dev);
+       struct pcan_usb_fd_device *pdev;
         enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
         enum can_state rx_state, tx_state;
-       struct net_device *netdev = dev->netdev;
+       struct peak_usb_device *dev;
+       struct net_device *netdev;
         struct can_frame *cf;
         struct sk_buff *skb;

+       if (pucan_stmsg_get_channel(sm) >= ARRAY_SIZE(usb_if->dev))
+               return -ENOMEM;
+
+       dev = usb_if->dev[pucan_stmsg_get_channel(sm)];
+       pdev = container_of(dev, struct pcan_usb_fd_device, dev);
+       netdev = dev->netdev;
+
         /* nothing should be sent while in BUS_OFF state */
         if (dev->can.state == CAN_STATE_BUS_OFF)
                 return 0;
@@ -579,9 +591,14 @@ static int pcan_usb_fd_decode_error(struct pcan_usb_fd_if *usb_if,
                                     struct pucan_msg *rx_msg)
 {
         struct pucan_error_msg *er = (struct pucan_error_msg *)rx_msg;
-       struct peak_usb_device *dev = usb_if->dev[pucan_ermsg_get_channel(er)];
-       struct pcan_usb_fd_device *pdev =
-                       container_of(dev, struct pcan_usb_fd_device, dev);
+       struct pcan_usb_fd_device *pdev;
+       struct peak_usb_device *dev;
+
+       if (pucan_ermsg_get_channel(er) >= ARRAY_SIZE(usb_if->dev))
+               return -EINVAL;
+
+       dev = usb_if->dev[pucan_ermsg_get_channel(er)];
+       pdev = container_of(dev, struct pcan_usb_fd_device, dev);

         /* keep a trace of tx and rx error counters for later use */
         pdev->bec.txerr = er->tx_err_cnt;
@@ -595,11 +612,17 @@ static int pcan_usb_fd_decode_overrun(struct pcan_usb_fd_if *usb_if,
                                       struct pucan_msg *rx_msg)
 {
         struct pcan_ufd_ovr_msg *ov = (struct pcan_ufd_ovr_msg *)rx_msg;
-       struct peak_usb_device *dev = usb_if->dev[pufd_omsg_get_channel(ov)];
-       struct net_device *netdev = dev->netdev;
+       struct peak_usb_device *dev;
+       struct net_device *netdev;
         struct can_frame *cf;
         struct sk_buff *skb;

+       if (pufd_omsg_get_channel(ov) >= ARRAY_SIZE(usb_if->dev))
+               return -EINVAL;
+
+       dev = usb_if->dev[pufd_omsg_get_channel(ov)];
+       netdev = dev->netdev;
+
         /* allocate an skb to store the error frame */
         skb = alloc_can_err_skb(netdev, &cf);
         if (!skb)
@@ -716,6 +739,9 @@ static int pcan_usb_fd_encode_msg(struct peak_usb_device *dev,
         u16 tx_msg_size, tx_msg_flags;
         u8 can_dlc;

+       if (cfd->len > CANFD_MAX_DLEN)
+               return -EINVAL;
+
         tx_msg_size = ALIGN(sizeof(struct pucan_tx_msg) + cfd->len, 4);
         tx_msg->size = cpu_to_le16(tx_msg_size);
         tx_msg->type = cpu_to_le16(PUCAN_MSG_CAN_TX);
--
2.28.0

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: Alexander Gach / Uwe Wilhelm
Unsere Datenschutzerklaerung mit wichtigen Hinweisen
zur Behandlung personenbezogener Daten finden Sie unter
www.peak-system.com/Datenschutz.483.0.html