* [bug report] can: mcba_usb: Add support for Microchip CAN BUS Analyzer
@ 2017-05-04 11:42 Dan Carpenter
2017-05-04 14:46 ` Kołłątaj, Remigiusz
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2017-05-04 11:42 UTC (permalink / raw)
To: remigiusz.kollataj; +Cc: linux-can
Hello Remigiusz Kołłątaj,
The patch 51f3baad7de9: "can: mcba_usb: Add support for Microchip CAN
BUS Analyzer" from Apr 14, 2017, leads to the following static
checker warning:
drivers/net/can/usb/mcba_usb.c:366 mcba_usb_start_xmit()
error: 'usb_msg.dlc' from user is not capped properly
drivers/net/can/usb/mcba_usb.c
320 static netdev_tx_t mcba_usb_start_xmit(struct sk_buff *skb,
321 struct net_device *netdev)
322 {
323 struct mcba_priv *priv = netdev_priv(netdev);
324 struct can_frame *cf = (struct can_frame *)skb->data;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Smatch distrusts skb->data.
325 struct mcba_usb_ctx *ctx = NULL;
326 struct net_device_stats *stats = &priv->netdev->stats;
327 u16 sid;
328 int err;
329 struct mcba_usb_msg_can usb_msg = {
330 .cmd_id = MBCA_CMD_TRANSMIT_MESSAGE_EV
331 };
332
333 if (can_dropped_invalid_skb(netdev, skb))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We do actually cap cf->can_dlc here but it's limited to
CANFD_MAX_DLEN (64) on one path. Also instead of calling it ->can_dlc
we call it "->len". My devel versions of Smatch could have parsed this
if we had kept the types consistent.
334 return NETDEV_TX_OK;
335
336 ctx = mcba_usb_get_free_ctx(priv, cf);
337 if (!ctx)
338 return NETDEV_TX_BUSY;
339
340 can_put_echo_skb(skb, priv->netdev, ctx->ndx);
341
342 if (cf->can_id & CAN_EFF_FLAG) {
343 /* SIDH | SIDL | EIDH | EIDL
344 * 28 - 21 | 20 19 18 x x x 17 16 | 15 - 8 | 7 - 0
345 */
346 sid = MCBA_SIDL_EXID_MASK;
347 /* store 28-18 bits */
348 sid |= (cf->can_id & 0x1ffc0000) >> 13;
349 /* store 17-16 bits */
350 sid |= (cf->can_id & 0x30000) >> 16;
351 put_unaligned_be16(sid, &usb_msg.sid);
352
353 /* store 15-0 bits */
354 put_unaligned_be16(cf->can_id & 0xffff, &usb_msg.eid);
355 } else {
356 /* SIDH | SIDL
357 * 10 - 3 | 2 1 0 x x x x x
358 */
359 put_unaligned_be16((cf->can_id & CAN_SFF_MASK) << 5,
360 &usb_msg.sid);
361 usb_msg.eid = 0;
362 }
363
364 usb_msg.dlc = cf->can_dlc;
365
366 memcpy(usb_msg.data, cf->data, usb_msg.dlc);
^^^^^^^^^^^^
This only has room for 8 bytes, so 64 might be a buffer overflow?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] can: mcba_usb: Add support for Microchip CAN BUS Analyzer
2017-05-04 11:42 [bug report] can: mcba_usb: Add support for Microchip CAN BUS Analyzer Dan Carpenter
@ 2017-05-04 14:46 ` Kołłątaj, Remigiusz
2017-05-05 4:36 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Kołłątaj, Remigiusz @ 2017-05-04 14:46 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-can
Hi Dan,
On 4 May 2017 at 13:42, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Remigiusz Kołłątaj,
>
> The patch 51f3baad7de9: "can: mcba_usb: Add support for Microchip CAN
> BUS Analyzer" from Apr 14, 2017, leads to the following static
> checker warning:
>
> drivers/net/can/usb/mcba_usb.c:366 mcba_usb_start_xmit()
> error: 'usb_msg.dlc' from user is not capped properly
>
> drivers/net/can/usb/mcba_usb.c
> 320 static netdev_tx_t mcba_usb_start_xmit(struct sk_buff *skb,
> 321 struct net_device *netdev)
> 322 {
> 323 struct mcba_priv *priv = netdev_priv(netdev);
> 324 struct can_frame *cf = (struct can_frame *)skb->data;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Smatch distrusts skb->data.
>
>
> 325 struct mcba_usb_ctx *ctx = NULL;
> 326 struct net_device_stats *stats = &priv->netdev->stats;
> 327 u16 sid;
> 328 int err;
> 329 struct mcba_usb_msg_can usb_msg = {
> 330 .cmd_id = MBCA_CMD_TRANSMIT_MESSAGE_EV
> 331 };
> 332
> 333 if (can_dropped_invalid_skb(netdev, skb))
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> We do actually cap cf->can_dlc here but it's limited to
> CANFD_MAX_DLEN (64) on one path. Also instead of calling it ->can_dlc
> we call it "->len". My devel versions of Smatch could have parsed this
> if we had kept the types consistent.
The function looks as follows:
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))
goto inval_skb;
} else if (skb->protocol == htons(ETH_P_CANFD)) {
if (unlikely(skb->len != CANFD_MTU ||
cfd->len > CANFD_MAX_DLEN))
goto inval_skb;
} else
goto inval_skb;
MCBA supports normal frames only so it falls into ETH_P_CAN. Both DLC
and SKB sizes are verified here. I understand that it may be difficult
for static analyser... Maybe you could make all values coming from skb
trustworthy after they are verified with can_dropped_invalid_skb()?
>
> 334 return NETDEV_TX_OK;
> 335
> 336 ctx = mcba_usb_get_free_ctx(priv, cf);
> 337 if (!ctx)
> 338 return NETDEV_TX_BUSY;
> 339
> 340 can_put_echo_skb(skb, priv->netdev, ctx->ndx);
> 341
> 342 if (cf->can_id & CAN_EFF_FLAG) {
> 343 /* SIDH | SIDL | EIDH | EIDL
> 344 * 28 - 21 | 20 19 18 x x x 17 16 | 15 - 8 | 7 - 0
> 345 */
> 346 sid = MCBA_SIDL_EXID_MASK;
> 347 /* store 28-18 bits */
> 348 sid |= (cf->can_id & 0x1ffc0000) >> 13;
> 349 /* store 17-16 bits */
> 350 sid |= (cf->can_id & 0x30000) >> 16;
> 351 put_unaligned_be16(sid, &usb_msg.sid);
> 352
> 353 /* store 15-0 bits */
> 354 put_unaligned_be16(cf->can_id & 0xffff, &usb_msg.eid);
> 355 } else {
> 356 /* SIDH | SIDL
> 357 * 10 - 3 | 2 1 0 x x x x x
> 358 */
> 359 put_unaligned_be16((cf->can_id & CAN_SFF_MASK) << 5,
> 360 &usb_msg.sid);
> 361 usb_msg.eid = 0;
> 362 }
> 363
> 364 usb_msg.dlc = cf->can_dlc;
> 365
> 366 memcpy(usb_msg.data, cf->data, usb_msg.dlc);
> ^^^^^^^^^^^^
> This only has room for 8 bytes, so 64 might be a buffer overflow?
>
thanks can_dropped_invalid_skb() cf->can_dlc is guaranteed to be <= 8.
> regards,
> dan carpenter
Regards,
Remik
--
____________________
Remigiusz Kołłątaj
Mobica Ltd
Technology Consultant
Skype: remigiusz.kollataj
www.mobica.com
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] can: mcba_usb: Add support for Microchip CAN BUS Analyzer
2017-05-04 14:46 ` Kołłątaj, Remigiusz
@ 2017-05-05 4:36 ` Dan Carpenter
2017-05-05 6:23 ` Kołłątaj, Remigiusz
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2017-05-05 4:36 UTC (permalink / raw)
To: Kołłątaj, Remigiusz; +Cc: linux-can
On Thu, May 04, 2017 at 04:46:58PM +0200, Kołłątaj, Remigiusz wrote:
> Hi Dan,
>
> On 4 May 2017 at 13:42, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > Hello Remigiusz Kołłątaj,
> >
> > The patch 51f3baad7de9: "can: mcba_usb: Add support for Microchip CAN
> > BUS Analyzer" from Apr 14, 2017, leads to the following static
> > checker warning:
> >
> > drivers/net/can/usb/mcba_usb.c:366 mcba_usb_start_xmit()
> > error: 'usb_msg.dlc' from user is not capped properly
> >
> > drivers/net/can/usb/mcba_usb.c
> > 320 static netdev_tx_t mcba_usb_start_xmit(struct sk_buff *skb,
> > 321 struct net_device *netdev)
> > 322 {
> > 323 struct mcba_priv *priv = netdev_priv(netdev);
> > 324 struct can_frame *cf = (struct can_frame *)skb->data;
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > Smatch distrusts skb->data.
> >
> >
> > 325 struct mcba_usb_ctx *ctx = NULL;
> > 326 struct net_device_stats *stats = &priv->netdev->stats;
> > 327 u16 sid;
> > 328 int err;
> > 329 struct mcba_usb_msg_can usb_msg = {
> > 330 .cmd_id = MBCA_CMD_TRANSMIT_MESSAGE_EV
> > 331 };
> > 332
> > 333 if (can_dropped_invalid_skb(netdev, skb))
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > We do actually cap cf->can_dlc here but it's limited to
> > CANFD_MAX_DLEN (64) on one path. Also instead of calling it ->can_dlc
> > we call it "->len". My devel versions of Smatch could have parsed this
> > if we had kept the types consistent.
>
> The function looks as follows:
>
> 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))
> goto inval_skb;
> } else if (skb->protocol == htons(ETH_P_CANFD)) {
> if (unlikely(skb->len != CANFD_MTU ||
> cfd->len > CANFD_MAX_DLEN))
> goto inval_skb;
> } else
> goto inval_skb;
>
> MCBA supports normal frames only so it falls into ETH_P_CAN.
Where is the function my static checker can look at to verify this?
> Both DLC and SKB sizes are verified here. I understand that it may be
> difficult for static analyser...
The difficulty is because can_dropped_invalid_skb() treats skb->data as
a canfd_frame struct and mcba_usb_start_xmit() treats it as a can_frame.
The layouts happen to align, but the names are different.
> Maybe you could make all values coming from skb trustworthy after they
> are verified with can_dropped_invalid_skb()?
That's a trickier thing and it probably doesn't scale.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] can: mcba_usb: Add support for Microchip CAN BUS Analyzer
2017-05-05 4:36 ` Dan Carpenter
@ 2017-05-05 6:23 ` Kołłątaj, Remigiusz
0 siblings, 0 replies; 4+ messages in thread
From: Kołłątaj, Remigiusz @ 2017-05-05 6:23 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-can
On 5 May 2017 at 06:36, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Thu, May 04, 2017 at 04:46:58PM +0200, Kołłątaj, Remigiusz wrote:
>> Hi Dan,
>>
>> On 4 May 2017 at 13:42, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> >
>> > Hello Remigiusz Kołłątaj,
>> >
>> > The patch 51f3baad7de9: "can: mcba_usb: Add support for Microchip CAN
>> > BUS Analyzer" from Apr 14, 2017, leads to the following static
>> > checker warning:
>> >
>> > drivers/net/can/usb/mcba_usb.c:366 mcba_usb_start_xmit()
>> > error: 'usb_msg.dlc' from user is not capped properly
>> >
>> > drivers/net/can/usb/mcba_usb.c
>> > 320 static netdev_tx_t mcba_usb_start_xmit(struct sk_buff *skb,
>> > 321 struct net_device *netdev)
>> > 322 {
>> > 323 struct mcba_priv *priv = netdev_priv(netdev);
>> > 324 struct can_frame *cf = (struct can_frame *)skb->data;
>> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> > Smatch distrusts skb->data.
>> >
>> >
>> > 325 struct mcba_usb_ctx *ctx = NULL;
>> > 326 struct net_device_stats *stats = &priv->netdev->stats;
>> > 327 u16 sid;
>> > 328 int err;
>> > 329 struct mcba_usb_msg_can usb_msg = {
>> > 330 .cmd_id = MBCA_CMD_TRANSMIT_MESSAGE_EV
>> > 331 };
>> > 332
>> > 333 if (can_dropped_invalid_skb(netdev, skb))
>> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> > We do actually cap cf->can_dlc here but it's limited to
>> > CANFD_MAX_DLEN (64) on one path. Also instead of calling it ->can_dlc
>> > we call it "->len". My devel versions of Smatch could have parsed this
>> > if we had kept the types consistent.
>>
>> The function looks as follows:
>>
>> 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))
>> goto inval_skb;
>> } else if (skb->protocol == htons(ETH_P_CANFD)) {
>> if (unlikely(skb->len != CANFD_MTU ||
>> cfd->len > CANFD_MAX_DLEN))
>> goto inval_skb;
>> } else
>> goto inval_skb;
>>
>> MCBA supports normal frames only so it falls into ETH_P_CAN.
>
> Where is the function my static checker can look at to verify this?
For CANFD devices you need to have ctrlmode set to CAN_CTRLMODE_FD.
>
>> Both DLC and SKB sizes are verified here. I understand that it may be
>> difficult for static analyser...
>
> The difficulty is because can_dropped_invalid_skb() treats skb->data as
> a canfd_frame struct and mcba_usb_start_xmit() treats it as a can_frame.
> The layouts happen to align, but the names are different.
The names differs because there is a difference in DLC (data length
code) for CAN and CANFD. For normal CAN payload length matches dlc.
For CAN FD it does not. Take a look at explanation here:
https://www.can-cia.org/can-knowledge/can/can-fd/
>
>> Maybe you could make all values coming from skb trustworthy after they
>> are verified with can_dropped_invalid_skb()?
>
> That's a trickier thing and it probably doesn't scale.
>
> regards,
> dan carpenter
>
Regards,
Remik
--
____________________
Remigiusz Kołłątaj
Mobica Ltd
Technology Consultant
Skype: remigiusz.kollataj
www.mobica.com
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-05-05 6:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-04 11:42 [bug report] can: mcba_usb: Add support for Microchip CAN BUS Analyzer Dan Carpenter
2017-05-04 14:46 ` Kołłątaj, Remigiusz
2017-05-05 4:36 ` Dan Carpenter
2017-05-05 6:23 ` Kołłątaj, Remigiusz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.