On 9/23/20 11:54 AM, Dan Carpenter wrote:
> 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).
From my point of view they look like false positive, let's see:
> 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():
> 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))
^^^^^^^^^^^^^^^^^^^^^^^
> 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);
[...]
> }
can_dropped_invalid_skb() checks if the skb is valid and consistent:
> 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...
> 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...
> goto inval_skb;
> } else
> goto inval_skb;
>
> if (!can_skb_headroom_valid(dev, skb))
> goto inval_skb;
>
> return false;
>
> inval_skb:
> kfree_skb(skb);
> dev->stats.tx_dropped++;
> return true;
> }
...and the limits are defined as:
> #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);
> ^^^^^^^^
> 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
--
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 |
Attachment:
signature.asc
Description: OpenPGP digital signature
