* [bug report] can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN
@ 2020-09-23 9:54 Dan Carpenter
2020-09-23 10:26 ` Marc Kleine-Budde
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2020-09-23 9:54 UTC (permalink / raw)
To: mkl; +Cc: linux-can
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN
2020-09-23 9:54 [bug report] can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN Dan Carpenter
@ 2020-09-23 10:26 ` Marc Kleine-Budde
2020-09-23 10:38 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Marc Kleine-Budde @ 2020-09-23 10:26 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-can
[-- Attachment #1.1: Type: text/plain, Size: 6840 bytes --]
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN
2020-09-23 10:26 ` Marc Kleine-Budde
@ 2020-09-23 10:38 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2020-09-23 10:38 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can
On Wed, Sep 23, 2020 at 12:26:42PM +0200, Marc Kleine-Budde wrote:
> 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:
>
Yeah. You're right. Sorry about that.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* [bug report] can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN
@ 2020-09-23 11:29 Dan Carpenter
2020-09-23 11:43 ` Marc Kleine-Budde
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2020-09-23 11:29 UTC (permalink / raw)
To: mkl; +Cc: linux-can
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 warning:
drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c:2155 mcp25xxfd_irq()
error: uninitialized symbol 'set_normal_mode'.
drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c
2074 do {
2075 u32 intf_pending, intf_pending_clearable;
2076 bool set_normal_mode;
^^^^^^^^^^^^^^^^^^^^
2077
2078 err = regmap_bulk_read(priv->map_reg, MCP25XXFD_REG_INT,
2079 &priv->regs_status,
2080 sizeof(priv->regs_status) /
2081 sizeof(u32));
2082 if (err)
2083 goto out_fail;
2084
2085 intf_pending = FIELD_GET(MCP25XXFD_REG_INT_IF_MASK,
2086 priv->regs_status.intf) &
2087 FIELD_GET(MCP25XXFD_REG_INT_IE_MASK,
2088 priv->regs_status.intf);
2089
2090 if (!(intf_pending))
2091 return handled;
2092
2093 /* Some interrupts must be ACKed in the
2094 * MCP25XXFD_REG_INT register.
2095 * - First ACK then handle, to avoid lost-IRQ race
2096 * condition on fast re-occurring interrupts.
2097 * - Write "0" to clear active IRQs, "1" to all other,
2098 * to avoid r/m/w race condition on the
2099 * MCP25XXFD_REG_INT register.
2100 */
2101 intf_pending_clearable = intf_pending &
2102 MCP25XXFD_REG_INT_IF_CLEARABLE_MASK;
2103 if (intf_pending_clearable) {
2104 err = regmap_update_bits(priv->map_reg,
2105 MCP25XXFD_REG_INT,
2106 MCP25XXFD_REG_INT_IF_MASK,
2107 ~intf_pending_clearable);
2108 if (err)
2109 goto out_fail;
2110 }
2111
2112 if (intf_pending & MCP25XXFD_REG_INT_MODIF) {
^^^^^^^^^^^^^^^^^^^^^^^
This is BIT(3)
2113 err = mcp25xxfd_handle(priv, modif, &set_normal_mode);
^^^^^^^^^^^^^^^^
2114 if (err)
2115 goto out_fail;
2116 }
2117
2118 if (intf_pending & MCP25XXFD_REG_INT_RXIF) {
2119 err = mcp25xxfd_handle(priv, rxif);
2120 if (err)
2121 goto out_fail;
2122 }
2123
2124 if (intf_pending & MCP25XXFD_REG_INT_TEFIF) {
2125 err = mcp25xxfd_handle(priv, tefif);
2126 if (err)
2127 goto out_fail;
2128 }
2129
2130 if (intf_pending & MCP25XXFD_REG_INT_RXOVIF) {
2131 err = mcp25xxfd_handle(priv, rxovif);
2132 if (err)
2133 goto out_fail;
2134 }
2135
2136 if (intf_pending & MCP25XXFD_REG_INT_TXATIF) {
2137 err = mcp25xxfd_handle(priv, txatif);
2138 if (err)
2139 goto out_fail;
2140 }
2141
2142 if (intf_pending & MCP25XXFD_REG_INT_IVMIF) {
2143 err = mcp25xxfd_handle(priv, ivmif);
2144 if (err)
2145 goto out_fail;
2146 }
2147
2148 if (intf_pending & MCP25XXFD_REG_INT_SERRIF) {
2149 err = mcp25xxfd_handle(priv, serrif);
2150 if (err)
2151 goto out_fail;
2152 }
2153
2154 if (intf_pending & MCP25XXFD_REG_INT_ECCIF) {
^^^^^^^^^^^^^^^^^^^^^^^
This is BIT(8)
2155 err = mcp25xxfd_handle(priv, eccif, set_normal_mode);
^^^^^^^^^^^^^^^^
This might not be initialized if BIT(8) is set but BIT(3) is not.
2156 if (err)
2157 goto out_fail;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN
2020-09-23 11:29 Dan Carpenter
@ 2020-09-23 11:43 ` Marc Kleine-Budde
0 siblings, 0 replies; 5+ messages in thread
From: Marc Kleine-Budde @ 2020-09-23 11:43 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-can
[-- Attachment #1.1: Type: text/plain, Size: 659 bytes --]
On 9/23/20 1:29 PM, 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 warning:
>
> drivers/net/can/spi/mcp25xxfd/mcp25xxfd-core.c:2155 mcp25xxfd_irq()
> error: uninitialized symbol 'set_normal_mode'.
ACK
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-23 11:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-23 9:54 [bug report] can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN Dan Carpenter
2020-09-23 10:26 ` Marc Kleine-Budde
2020-09-23 10:38 ` Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2020-09-23 11:29 Dan Carpenter
2020-09-23 11:43 ` Marc Kleine-Budde
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).