can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN

bug report Dan Carpenter 2 participants 3 messages Sep 23, 2020 CAN network drivers

Share & tools

Dan Carpenter

author

2020-09-23 09:55 UTC #0 lore raw

Hello Marc Kleine-Budde,

The patch 55e5b97f003e: "can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN" from Sep 18, 2020, leads to the following static checker Smatch warning:

drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c:2271 mcp25xxfd_tx_obj_from_skb() warn: user controlled 'len' cast to postive rl = '(-249)-(-1),1-67' drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c:2272 mcp25xxfd_tx_obj_from_skb() error: 'memcpy()' 'hw_tx_obj->data' too small (64 vs 255) drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c:2272 mcp25xxfd_tx_obj_from_skb() error: 'memcpy()' 'cfd->data' too small (64 vs 255) drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c:2272 mcp25xxfd_tx_obj_from_skb() error: 'cfd->len' from user is not capped properly

(Only one of these checks is published and it's disabled unless you run with the --spammy flag).

drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c
2208 static void
2209 mcp25xxfd_tx_obj_from_skb(const struct mcp25xxfd_priv *priv,

  2210                            struct mcp25xxfd_tx_obj *tx_obj,
  2211                            const struct sk_buff *skb,
  2212                            unsigned int seq)
  2213  {
  2214          const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Smatch distrusts all skb->data information.

  2215          struct mcp25xxfd_hw_tx_obj_raw *hw_tx_obj;
  2216          union mcp25xxfd_tx_obj_load_buf *load_buf;
  2217          u8 dlc;
  2218          u32 id, flags;
  2219          int offset, len;
  2220  
  2221          if (cfd->can_id & CAN_EFF_FLAG) {
  2222                  u32 sid, eid;
  2223  
  2224                  sid = FIELD_GET(MCP25XXFD_REG_FRAME_EFF_SID_MASK, cfd->can_id);
  2225                  eid = FIELD_GET(MCP25XXFD_REG_FRAME_EFF_EID_MASK, cfd->can_id);
  2226  
  2227                  id = FIELD_PREP(MCP25XXFD_OBJ_ID_EID_MASK, eid) |
  2228                          FIELD_PREP(MCP25XXFD_OBJ_ID_SID_MASK, sid);
  2229  
  2230                  flags = MCP25XXFD_OBJ_FLAGS_IDE;
  2231          } else {
  2232                  id = FIELD_PREP(MCP25XXFD_OBJ_ID_SID_MASK, cfd->can_id);
  2233                  flags = 0;
  2234          }
  2235  
  2236          /* Use the MCP2518FD mask even on the MCP2517FD. It doesn't
  2237           * harm, only the lower 7 bits will be transferred into the
  2238           * TEF object.
  2239           */
  2240          dlc = can_len2dlc(cfd->len);
  2241          flags |= FIELD_PREP(MCP25XXFD_OBJ_FLAGS_SEQ_MCP2518FD_MASK, seq) |
  2242                  FIELD_PREP(MCP25XXFD_OBJ_FLAGS_DLC, dlc);
  2243  
  2244          if (cfd->can_id & CAN_RTR_FLAG)
  2245                  flags |= MCP25XXFD_OBJ_FLAGS_RTR;
  2246  
  2247          /* CANFD */
  2248          if (can_is_canfd_skb(skb)) {
  2249                  if (cfd->flags & CANFD_ESI)
  2250                          flags |= MCP25XXFD_OBJ_FLAGS_ESI;
  2251  
  2252                  flags |= MCP25XXFD_OBJ_FLAGS_FDF;
  2253  
  2254                  if (cfd->flags & CANFD_BRS)
  2255                          flags |= MCP25XXFD_OBJ_FLAGS_BRS;
  2256          }
  2257  
  2258          load_buf = &tx_obj->buf;
  2259          if (priv->devtype_data.quirks & MCP25XXFD_QUIRK_CRC_TX)
  2260                  hw_tx_obj = &load_buf->crc.hw_tx_obj;
  2261          else
  2262                  hw_tx_obj = &load_buf->nocrc.hw_tx_obj;
  2263  
  2264          put_unaligned_le32(id, &hw_tx_obj->id);
  2265          put_unaligned_le32(flags, &hw_tx_obj->flags);
  2266  
  2267          /* Clear data at end of CAN frame */
  2268          offset = round_down(cfd->len, sizeof(u32));
  2269          len = round_up(can_dlc2len(dlc), sizeof(u32)) - offset;

Smatch thinks that "cfd->len" can be more than 64 which leads to setting "len" negative.

2270 if (MCP25XXFD_SANITIZE_CAN && len)

  2271                  memset(hw_tx_obj->data + offset, 0x0, len);
                                                              ^^^

Which would corrupt memory.

  2272          memcpy(hw_tx_obj->data, cfd->data, cfd->len);
                                                   ^^^^^^^^

Smatch thinks this can be more than 64.

2273
2274 /* Number of bytes to be written into the RAM of the controller */
2275 len = sizeof(hw_tx_obj->id) + sizeof(hw_tx_obj->flags);

regards,
dan carpenter

Marc Kleine-Budde

maintainer

2020-09-23 10:26 UTC #1 lore raw

Quoted 6 lines · Dan Carpenter

Hello Marc Kleine-Budde,
The patch 55e5b97f003e: "can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN" from Sep 18, 2020, leads to the following static checker Smatch warning:

Quoted 7 lines

drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c:2271 mcp25xxfd_tx_obj_from_skb() warn: user controlled 'len' cast to postive rl = '(-249)-(-1),1-67' drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c:2272 mcp25xxfd_tx_obj_from_skb() error: 'memcpy()' 'hw_tx_obj->data' too small (64 vs 255) drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c:2272 mcp25xxfd_tx_obj_from_skb() error: 'memcpy()' 'cfd->data' too small (64 vs 255) drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c:2272 mcp25xxfd_tx_obj_from_skb() error: 'cfd->len' from user is not capped properly
(Only one of these checks is published and it's disabled unless you run with the --spammy flag).

From my point of view they look like false positive, let's see:

Quoted 10 lines

drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c
2208 static void
2209 mcp25xxfd_tx_obj_from_skb(const struct mcp25xxfd_priv *priv,
  2210                            struct mcp25xxfd_tx_obj *tx_obj,
  2211                            const struct sk_buff *skb,
  2212                            unsigned int seq)
  2213  {
  2214          const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Smatch distrusts all skb->data information.

We do neither.

The function in question is only called from mcp25xxfd_tx_obj_from_skb():

Quoted 10 lines

static netdev_tx_t mcp25xxfd_start_xmit(struct sk_buff *skb,
struct net_device *ndev)

{
	struct mcp25xxfd_priv *priv = netdev_priv(ndev);
	struct mcp25xxfd_tx_ring *tx_ring = priv->tx;
	struct mcp25xxfd_tx_obj *tx_obj;
	u8 tx_head;
	int err;

	if (can_dropped_invalid_skb(ndev, skb))

            ^^^^^^^^^^^^^^^^^^^^^^^

Quoted 7 lines

		return NETDEV_TX_OK;

	if (mcp25xxfd_tx_busy(priv, tx_ring))
		return NETDEV_TX_BUSY;

	tx_obj = mcp25xxfd_get_tx_obj_next(tx_ring);
	mcp25xxfd_tx_obj_from_skb(priv, tx_obj, skb, tx_ring->head);

[...]

Quoted 1 line

}

can_dropped_invalid_skb() checks if the skb is valid and consistent:

Quoted 8 lines

static inline bool can_dropped_invalid_skb(struct net_device *dev,
struct sk_buff *skb)
{
	const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;

	if (skb->protocol == htons(ETH_P_CAN)) {
		if (unlikely(skb->len != CAN_MTU ||
			     cfd->len > CAN_MAX_DLEN))

                             ^^^^^^^^^^^^^^^^^^^^^^^

...here we check for cfd->len...

Quoted 4 lines

			goto inval_skb;
	} else if (skb->protocol == htons(ETH_P_CANFD)) {
		if (unlikely(skb->len != CANFD_MTU ||
			     cfd->len > CANFD_MAX_DLEN))

                             ^^^^^^^^^^^^^^^^^^^^^^^^^

...here we check for cfd->len...

Quoted 9 lines

			goto inval_skb;
	} else
		goto inval_skb;

	if (!can_skb_headroom_valid(dev, skb))
		goto inval_skb;

	return false;

Quoted 5 lines

inval_skb:

	kfree_skb(skb);
	dev->stats.tx_dropped++;
	return true;
}

...and the limits are defined as:

Quoted 69 lines

#define CAN_MAX_DLEN 8
#define CANFD_MAX_DLEN 64

  2215          struct mcp25xxfd_hw_tx_obj_raw *hw_tx_obj;
  2216          union mcp25xxfd_tx_obj_load_buf *load_buf;
  2217          u8 dlc;
  2218          u32 id, flags;
  2219          int offset, len;
  2220  
  2221          if (cfd->can_id & CAN_EFF_FLAG) {
  2222                  u32 sid, eid;
  2223  
  2224                  sid = FIELD_GET(MCP25XXFD_REG_FRAME_EFF_SID_MASK, cfd->can_id);
  2225                  eid = FIELD_GET(MCP25XXFD_REG_FRAME_EFF_EID_MASK, cfd->can_id);
  2226  
  2227                  id = FIELD_PREP(MCP25XXFD_OBJ_ID_EID_MASK, eid) |
  2228                          FIELD_PREP(MCP25XXFD_OBJ_ID_SID_MASK, sid);
  2229  
  2230                  flags = MCP25XXFD_OBJ_FLAGS_IDE;
  2231          } else {
  2232                  id = FIELD_PREP(MCP25XXFD_OBJ_ID_SID_MASK, cfd->can_id);
  2233                  flags = 0;
  2234          }
  2235  
  2236          /* Use the MCP2518FD mask even on the MCP2517FD. It doesn't
  2237           * harm, only the lower 7 bits will be transferred into the
  2238           * TEF object.
  2239           */
  2240          dlc = can_len2dlc(cfd->len);
  2241          flags |= FIELD_PREP(MCP25XXFD_OBJ_FLAGS_SEQ_MCP2518FD_MASK, seq) |
  2242                  FIELD_PREP(MCP25XXFD_OBJ_FLAGS_DLC, dlc);
  2243  
  2244          if (cfd->can_id & CAN_RTR_FLAG)
  2245                  flags |= MCP25XXFD_OBJ_FLAGS_RTR;
  2246  
  2247          /* CANFD */
  2248          if (can_is_canfd_skb(skb)) {
  2249                  if (cfd->flags & CANFD_ESI)
  2250                          flags |= MCP25XXFD_OBJ_FLAGS_ESI;
  2251  
  2252                  flags |= MCP25XXFD_OBJ_FLAGS_FDF;
  2253  
  2254                  if (cfd->flags & CANFD_BRS)
  2255                          flags |= MCP25XXFD_OBJ_FLAGS_BRS;
  2256          }
  2257  
  2258          load_buf = &tx_obj->buf;
  2259          if (priv->devtype_data.quirks & MCP25XXFD_QUIRK_CRC_TX)
  2260                  hw_tx_obj = &load_buf->crc.hw_tx_obj;
  2261          else
  2262                  hw_tx_obj = &load_buf->nocrc.hw_tx_obj;
  2263  
  2264          put_unaligned_le32(id, &hw_tx_obj->id);
  2265          put_unaligned_le32(flags, &hw_tx_obj->flags);
  2266  
  2267          /* Clear data at end of CAN frame */
  2268          offset = round_down(cfd->len, sizeof(u32));
  2269          len = round_up(can_dlc2len(dlc), sizeof(u32)) - offset;

Smatch thinks that "cfd->len" can be more than 64 which leads to setting "len" negative.

2270 if (MCP25XXFD_SANITIZE_CAN && len)

  2271                  memset(hw_tx_obj->data + offset, 0x0, len);
                                                              ^^^

Which would corrupt memory.

  2272          memcpy(hw_tx_obj->data, cfd->data, cfd->len);
                                                   ^^^^^^^^

Quoted 9 lines

Smatch thinks this can be more than 64.
2273
2274 /* Number of bytes to be written into the RAM of the controller */
2275 len = sizeof(hw_tx_obj->id) + sizeof(hw_tx_obj->flags);
regards,
dan carpenter

regards,
Marc

Signature · 4 lines

Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Dan Carpenter

author

2020-09-23 10:38 UTC #2 lore raw

Quoted 12 lines · Marc Kleine-Budde

Quoted 11 lines · Dan Carpenter
Hello Marc Kleine-Budde,
The patch 55e5b97f003e: "can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN" from Sep 18, 2020, leads to the following static checker Smatch warning:
drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c:2271 mcp25xxfd_tx_obj_from_skb() warn: user controlled 'len' cast to postive rl = '(-249)-(-1),1-67' drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c:2272 mcp25xxfd_tx_obj_from_skb() error: 'memcpy()' 'hw_tx_obj->data' too small (64 vs 255) drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c:2272 mcp25xxfd_tx_obj_from_skb() error: 'memcpy()' 'cfd->data' too small (64 vs 255) drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c:2272 mcp25xxfd_tx_obj_from_skb() error: 'cfd->len' from user is not capped properly

Quoted 5 lines

Quoted 4 lines
(Only one of these checks is published and it's disabled unless you run with the --spammy flag).
From my point of view they look like false positive, let's see:

Yeah. You're right. Sorry about that.

regards,
dan carpenter