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
