* [linux-next:master 2736/10638] drivers/net/can/usb/peak_usb/pcan_usb.c:523 pcan_usb_decode_error() error: we previously assumed 'cf' could be null (see line 503)
@ 2021-08-27 8:05 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2021-08-27 4:39 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 7504 bytes --]
CC: kbuild-all(a)lists.01.org
CC: Linux Memory Management List <linux-mm@kvack.org>
TO: Stephane Grosjean <s.grosjean@peak-system.com>
CC: "Marc Kleine-Budde" <mkl@pengutronix.de>
tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
head: 88fac11862d38306dd0d2398015744877140390d
commit: c11dcee758302702a83c6e85e4c4c3d9af42d2b3 [2736/10638] can: peak_usb: pcan_usb_decode_error(): upgrade handling of bus state changes
:::::: branch date:
:::::: commit date: 5 weeks ago
config: i386-randconfig-m021-20210827 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/net/can/usb/peak_usb/pcan_usb.c:523 pcan_usb_decode_error() error: we previously assumed 'cf' could be null (see line 503)
vim +/cf +523 drivers/net/can/usb/peak_usb/pcan_usb.c
46be265d338833 Stephane Grosjean 2012-03-02 449
46be265d338833 Stephane Grosjean 2012-03-02 450 static int pcan_usb_decode_error(struct pcan_usb_msg_context *mc, u8 n,
46be265d338833 Stephane Grosjean 2012-03-02 451 u8 status_len)
46be265d338833 Stephane Grosjean 2012-03-02 452 {
46be265d338833 Stephane Grosjean 2012-03-02 453 struct sk_buff *skb;
46be265d338833 Stephane Grosjean 2012-03-02 454 struct can_frame *cf;
c11dcee7583027 Stephane Grosjean 2021-07-15 455 enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
46be265d338833 Stephane Grosjean 2012-03-02 456
46be265d338833 Stephane Grosjean 2012-03-02 457 /* ignore this error until 1st ts received */
46be265d338833 Stephane Grosjean 2012-03-02 458 if (n == PCAN_USB_ERROR_QOVR)
46be265d338833 Stephane Grosjean 2012-03-02 459 if (!mc->pdev->time_ref.tick_count)
46be265d338833 Stephane Grosjean 2012-03-02 460 return 0;
46be265d338833 Stephane Grosjean 2012-03-02 461
c11dcee7583027 Stephane Grosjean 2021-07-15 462 /* allocate an skb to store the error frame */
c11dcee7583027 Stephane Grosjean 2021-07-15 463 skb = alloc_can_err_skb(mc->netdev, &cf);
46be265d338833 Stephane Grosjean 2012-03-02 464
c11dcee7583027 Stephane Grosjean 2021-07-15 465 if (n & PCAN_USB_ERROR_RXQOVR) {
c11dcee7583027 Stephane Grosjean 2021-07-15 466 /* data overrun interrupt */
c11dcee7583027 Stephane Grosjean 2021-07-15 467 netdev_dbg(mc->netdev, "data overrun interrupt\n");
c11dcee7583027 Stephane Grosjean 2021-07-15 468 mc->netdev->stats.rx_over_errors++;
c11dcee7583027 Stephane Grosjean 2021-07-15 469 mc->netdev->stats.rx_errors++;
c11dcee7583027 Stephane Grosjean 2021-07-15 470 if (cf) {
c11dcee7583027 Stephane Grosjean 2021-07-15 471 cf->can_id |= CAN_ERR_CRTL;
c11dcee7583027 Stephane Grosjean 2021-07-15 472 cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
46be265d338833 Stephane Grosjean 2012-03-02 473 }
46be265d338833 Stephane Grosjean 2012-03-02 474 }
46be265d338833 Stephane Grosjean 2012-03-02 475
c11dcee7583027 Stephane Grosjean 2021-07-15 476 if (n & PCAN_USB_ERROR_TXQFULL)
c11dcee7583027 Stephane Grosjean 2021-07-15 477 netdev_dbg(mc->netdev, "device Tx queue full)\n");
c11dcee7583027 Stephane Grosjean 2021-07-15 478
46be265d338833 Stephane Grosjean 2012-03-02 479 if (n & PCAN_USB_ERROR_BUS_OFF) {
46be265d338833 Stephane Grosjean 2012-03-02 480 new_state = CAN_STATE_BUS_OFF;
c11dcee7583027 Stephane Grosjean 2021-07-15 481 } else if (n & PCAN_USB_ERROR_BUS_HEAVY) {
c11dcee7583027 Stephane Grosjean 2021-07-15 482 new_state = ((mc->pdev->bec.txerr >= 128) ||
c11dcee7583027 Stephane Grosjean 2021-07-15 483 (mc->pdev->bec.rxerr >= 128)) ?
c11dcee7583027 Stephane Grosjean 2021-07-15 484 CAN_STATE_ERROR_PASSIVE :
c11dcee7583027 Stephane Grosjean 2021-07-15 485 CAN_STATE_ERROR_WARNING;
c11dcee7583027 Stephane Grosjean 2021-07-15 486 } else {
c11dcee7583027 Stephane Grosjean 2021-07-15 487 new_state = CAN_STATE_ERROR_ACTIVE;
46be265d338833 Stephane Grosjean 2012-03-02 488 }
46be265d338833 Stephane Grosjean 2012-03-02 489
c11dcee7583027 Stephane Grosjean 2021-07-15 490 /* handle change of state */
c11dcee7583027 Stephane Grosjean 2021-07-15 491 if (new_state != mc->pdev->dev.can.state) {
c11dcee7583027 Stephane Grosjean 2021-07-15 492 enum can_state tx_state =
c11dcee7583027 Stephane Grosjean 2021-07-15 493 (mc->pdev->bec.txerr >= mc->pdev->bec.rxerr) ?
c11dcee7583027 Stephane Grosjean 2021-07-15 494 new_state : 0;
c11dcee7583027 Stephane Grosjean 2021-07-15 495 enum can_state rx_state =
c11dcee7583027 Stephane Grosjean 2021-07-15 496 (mc->pdev->bec.txerr <= mc->pdev->bec.rxerr) ?
c11dcee7583027 Stephane Grosjean 2021-07-15 497 new_state : 0;
46be265d338833 Stephane Grosjean 2012-03-02 498
c11dcee7583027 Stephane Grosjean 2021-07-15 499 can_change_state(mc->netdev, cf, tx_state, rx_state);
46be265d338833 Stephane Grosjean 2012-03-02 500
c11dcee7583027 Stephane Grosjean 2021-07-15 501 if (new_state == CAN_STATE_BUS_OFF) {
46be265d338833 Stephane Grosjean 2012-03-02 502 can_bus_off(mc->netdev);
c11dcee7583027 Stephane Grosjean 2021-07-15 @503 } else if (cf && (cf->can_id & CAN_ERR_CRTL)) {
c11dcee7583027 Stephane Grosjean 2021-07-15 504 /* Supply TX/RX error counters in case of
c11dcee7583027 Stephane Grosjean 2021-07-15 505 * controller error.
c11dcee7583027 Stephane Grosjean 2021-07-15 506 */
ea8b33bde76c8f Stephane Grosjean 2019-12-06 507 cf->data[6] = mc->pdev->bec.txerr;
ea8b33bde76c8f Stephane Grosjean 2019-12-06 508 cf->data[7] = mc->pdev->bec.rxerr;
ea8b33bde76c8f Stephane Grosjean 2019-12-06 509 }
46be265d338833 Stephane Grosjean 2012-03-02 510 }
46be265d338833 Stephane Grosjean 2012-03-02 511
c11dcee7583027 Stephane Grosjean 2021-07-15 512 if (!skb)
c11dcee7583027 Stephane Grosjean 2021-07-15 513 return -ENOMEM;
46be265d338833 Stephane Grosjean 2012-03-02 514
46be265d338833 Stephane Grosjean 2012-03-02 515 if (status_len & PCAN_USB_STATUSLEN_TIMESTAMP) {
c9faaa09e2a133 Oliver Hartkopp 2012-11-21 516 struct skb_shared_hwtstamps *hwts = skb_hwtstamps(skb);
c9faaa09e2a133 Oliver Hartkopp 2012-11-21 517
d5888a1e75c799 Arnd Bergmann 2017-11-03 518 peak_usb_get_ts_time(&mc->pdev->time_ref, mc->ts16,
d5888a1e75c799 Arnd Bergmann 2017-11-03 519 &hwts->hwtstamp);
46be265d338833 Stephane Grosjean 2012-03-02 520 }
46be265d338833 Stephane Grosjean 2012-03-02 521
46be265d338833 Stephane Grosjean 2012-03-02 522 mc->netdev->stats.rx_packets++;
c7b74967799b1a Oliver Hartkopp 2020-11-20 @523 mc->netdev->stats.rx_bytes += cf->len;
1c0ee046957648 Marc Kleine-Budde 2015-07-11 524 netif_rx(skb);
46be265d338833 Stephane Grosjean 2012-03-02 525
46be265d338833 Stephane Grosjean 2012-03-02 526 return 0;
46be265d338833 Stephane Grosjean 2012-03-02 527 }
46be265d338833 Stephane Grosjean 2012-03-02 528
:::::: The code at line 523 was first introduced by commit
:::::: c7b74967799b1af52b3045d69d4c26836b2d41de can: replace can_dlc as variable/element for payload length
:::::: TO: Oliver Hartkopp <socketcan@hartkopp.net>
:::::: CC: Marc Kleine-Budde <mkl@pengutronix.de>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 42268 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* [linux-next:master 2736/10638] drivers/net/can/usb/peak_usb/pcan_usb.c:523 pcan_usb_decode_error() error: we previously assumed 'cf' could be null (see line 503)
@ 2021-08-27 8:05 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2021-08-27 8:05 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 7190 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
head: 88fac11862d38306dd0d2398015744877140390d
commit: c11dcee758302702a83c6e85e4c4c3d9af42d2b3 [2736/10638] can: peak_usb: pcan_usb_decode_error(): upgrade handling of bus state changes
config: i386-randconfig-m021-20210827 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/net/can/usb/peak_usb/pcan_usb.c:523 pcan_usb_decode_error() error: we previously assumed 'cf' could be null (see line 503)
vim +/cf +523 drivers/net/can/usb/peak_usb/pcan_usb.c
46be265d338833 Stephane Grosjean 2012-03-02 450 static int pcan_usb_decode_error(struct pcan_usb_msg_context *mc, u8 n,
46be265d338833 Stephane Grosjean 2012-03-02 451 u8 status_len)
46be265d338833 Stephane Grosjean 2012-03-02 452 {
46be265d338833 Stephane Grosjean 2012-03-02 453 struct sk_buff *skb;
46be265d338833 Stephane Grosjean 2012-03-02 454 struct can_frame *cf;
c11dcee7583027 Stephane Grosjean 2021-07-15 455 enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
46be265d338833 Stephane Grosjean 2012-03-02 456
46be265d338833 Stephane Grosjean 2012-03-02 457 /* ignore this error until 1st ts received */
46be265d338833 Stephane Grosjean 2012-03-02 458 if (n == PCAN_USB_ERROR_QOVR)
46be265d338833 Stephane Grosjean 2012-03-02 459 if (!mc->pdev->time_ref.tick_count)
46be265d338833 Stephane Grosjean 2012-03-02 460 return 0;
46be265d338833 Stephane Grosjean 2012-03-02 461
c11dcee7583027 Stephane Grosjean 2021-07-15 462 /* allocate an skb to store the error frame */
c11dcee7583027 Stephane Grosjean 2021-07-15 463 skb = alloc_can_err_skb(mc->netdev, &cf);
46be265d338833 Stephane Grosjean 2012-03-02 464
c11dcee7583027 Stephane Grosjean 2021-07-15 465 if (n & PCAN_USB_ERROR_RXQOVR) {
c11dcee7583027 Stephane Grosjean 2021-07-15 466 /* data overrun interrupt */
c11dcee7583027 Stephane Grosjean 2021-07-15 467 netdev_dbg(mc->netdev, "data overrun interrupt\n");
c11dcee7583027 Stephane Grosjean 2021-07-15 468 mc->netdev->stats.rx_over_errors++;
c11dcee7583027 Stephane Grosjean 2021-07-15 469 mc->netdev->stats.rx_errors++;
c11dcee7583027 Stephane Grosjean 2021-07-15 470 if (cf) {
^^
Check for NULL
c11dcee7583027 Stephane Grosjean 2021-07-15 471 cf->can_id |= CAN_ERR_CRTL;
c11dcee7583027 Stephane Grosjean 2021-07-15 472 cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
46be265d338833 Stephane Grosjean 2012-03-02 473 }
46be265d338833 Stephane Grosjean 2012-03-02 474 }
46be265d338833 Stephane Grosjean 2012-03-02 475
c11dcee7583027 Stephane Grosjean 2021-07-15 476 if (n & PCAN_USB_ERROR_TXQFULL)
c11dcee7583027 Stephane Grosjean 2021-07-15 477 netdev_dbg(mc->netdev, "device Tx queue full)\n");
c11dcee7583027 Stephane Grosjean 2021-07-15 478
46be265d338833 Stephane Grosjean 2012-03-02 479 if (n & PCAN_USB_ERROR_BUS_OFF) {
46be265d338833 Stephane Grosjean 2012-03-02 480 new_state = CAN_STATE_BUS_OFF;
c11dcee7583027 Stephane Grosjean 2021-07-15 481 } else if (n & PCAN_USB_ERROR_BUS_HEAVY) {
c11dcee7583027 Stephane Grosjean 2021-07-15 482 new_state = ((mc->pdev->bec.txerr >= 128) ||
c11dcee7583027 Stephane Grosjean 2021-07-15 483 (mc->pdev->bec.rxerr >= 128)) ?
c11dcee7583027 Stephane Grosjean 2021-07-15 484 CAN_STATE_ERROR_PASSIVE :
c11dcee7583027 Stephane Grosjean 2021-07-15 485 CAN_STATE_ERROR_WARNING;
c11dcee7583027 Stephane Grosjean 2021-07-15 486 } else {
c11dcee7583027 Stephane Grosjean 2021-07-15 487 new_state = CAN_STATE_ERROR_ACTIVE;
46be265d338833 Stephane Grosjean 2012-03-02 488 }
46be265d338833 Stephane Grosjean 2012-03-02 489
c11dcee7583027 Stephane Grosjean 2021-07-15 490 /* handle change of state */
c11dcee7583027 Stephane Grosjean 2021-07-15 491 if (new_state != mc->pdev->dev.can.state) {
c11dcee7583027 Stephane Grosjean 2021-07-15 492 enum can_state tx_state =
c11dcee7583027 Stephane Grosjean 2021-07-15 493 (mc->pdev->bec.txerr >= mc->pdev->bec.rxerr) ?
c11dcee7583027 Stephane Grosjean 2021-07-15 494 new_state : 0;
c11dcee7583027 Stephane Grosjean 2021-07-15 495 enum can_state rx_state =
c11dcee7583027 Stephane Grosjean 2021-07-15 496 (mc->pdev->bec.txerr <= mc->pdev->bec.rxerr) ?
c11dcee7583027 Stephane Grosjean 2021-07-15 497 new_state : 0;
46be265d338833 Stephane Grosjean 2012-03-02 498
c11dcee7583027 Stephane Grosjean 2021-07-15 499 can_change_state(mc->netdev, cf, tx_state, rx_state);
46be265d338833 Stephane Grosjean 2012-03-02 500
c11dcee7583027 Stephane Grosjean 2021-07-15 501 if (new_state == CAN_STATE_BUS_OFF) {
46be265d338833 Stephane Grosjean 2012-03-02 502 can_bus_off(mc->netdev);
c11dcee7583027 Stephane Grosjean 2021-07-15 @503 } else if (cf && (cf->can_id & CAN_ERR_CRTL)) {
^^
Check for NULL
c11dcee7583027 Stephane Grosjean 2021-07-15 504 /* Supply TX/RX error counters in case of
c11dcee7583027 Stephane Grosjean 2021-07-15 505 * controller error.
c11dcee7583027 Stephane Grosjean 2021-07-15 506 */
ea8b33bde76c8f Stephane Grosjean 2019-12-06 507 cf->data[6] = mc->pdev->bec.txerr;
ea8b33bde76c8f Stephane Grosjean 2019-12-06 508 cf->data[7] = mc->pdev->bec.rxerr;
ea8b33bde76c8f Stephane Grosjean 2019-12-06 509 }
46be265d338833 Stephane Grosjean 2012-03-02 510 }
46be265d338833 Stephane Grosjean 2012-03-02 511
c11dcee7583027 Stephane Grosjean 2021-07-15 512 if (!skb)
c11dcee7583027 Stephane Grosjean 2021-07-15 513 return -ENOMEM;
46be265d338833 Stephane Grosjean 2012-03-02 514
46be265d338833 Stephane Grosjean 2012-03-02 515 if (status_len & PCAN_USB_STATUSLEN_TIMESTAMP) {
c9faaa09e2a133 Oliver Hartkopp 2012-11-21 516 struct skb_shared_hwtstamps *hwts = skb_hwtstamps(skb);
c9faaa09e2a133 Oliver Hartkopp 2012-11-21 517
d5888a1e75c799 Arnd Bergmann 2017-11-03 518 peak_usb_get_ts_time(&mc->pdev->time_ref, mc->ts16,
d5888a1e75c799 Arnd Bergmann 2017-11-03 519 &hwts->hwtstamp);
46be265d338833 Stephane Grosjean 2012-03-02 520 }
46be265d338833 Stephane Grosjean 2012-03-02 521
46be265d338833 Stephane Grosjean 2012-03-02 522 mc->netdev->stats.rx_packets++;
c7b74967799b1a Oliver Hartkopp 2020-11-20 @523 mc->netdev->stats.rx_bytes += cf->len;
^^^^^^^
Unchecked dereference.
1c0ee046957648 Marc Kleine-Budde 2015-07-11 524 netif_rx(skb);
46be265d338833 Stephane Grosjean 2012-03-02 525
46be265d338833 Stephane Grosjean 2012-03-02 526 return 0;
46be265d338833 Stephane Grosjean 2012-03-02 527 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [linux-next:master 2736/10638] drivers/net/can/usb/peak_usb/pcan_usb.c:523 pcan_usb_decode_error() error: we previously assumed 'cf' could be null (see line 503)
2021-08-27 8:05 ` Dan Carpenter
(?)
@ 2021-08-27 9:20 ` Marc Kleine-Budde
2021-08-27 9:54 ` Dan Carpenter
-1 siblings, 1 reply; 5+ messages in thread
From: Marc Kleine-Budde @ 2021-08-27 9:20 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 8468 bytes --]
On 27.08.2021 11:05:05, Dan Carpenter wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> head: 88fac11862d38306dd0d2398015744877140390d
> commit: c11dcee758302702a83c6e85e4c4c3d9af42d2b3 [2736/10638] can: peak_usb: pcan_usb_decode_error(): upgrade handling of bus state changes
> config: i386-randconfig-m021-20210827 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> smatch warnings:
> drivers/net/can/usb/peak_usb/pcan_usb.c:523 pcan_usb_decode_error() error: we previously assumed 'cf' could be null (see line 503)
>
> vim +/cf +523 drivers/net/can/usb/peak_usb/pcan_usb.c
>
> 46be265d338833 Stephane Grosjean 2012-03-02 450 static int pcan_usb_decode_error(struct pcan_usb_msg_context *mc, u8 n,
> 46be265d338833 Stephane Grosjean 2012-03-02 451 u8 status_len)
> 46be265d338833 Stephane Grosjean 2012-03-02 452 {
> 46be265d338833 Stephane Grosjean 2012-03-02 453 struct sk_buff *skb;
> 46be265d338833 Stephane Grosjean 2012-03-02 454 struct can_frame *cf;
> c11dcee7583027 Stephane Grosjean 2021-07-15 455 enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
> 46be265d338833 Stephane Grosjean 2012-03-02 456
> 46be265d338833 Stephane Grosjean 2012-03-02 457 /* ignore this error until 1st ts received */
> 46be265d338833 Stephane Grosjean 2012-03-02 458 if (n == PCAN_USB_ERROR_QOVR)
> 46be265d338833 Stephane Grosjean 2012-03-02 459 if (!mc->pdev->time_ref.tick_count)
> 46be265d338833 Stephane Grosjean 2012-03-02 460 return 0;
> 46be265d338833 Stephane Grosjean 2012-03-02 461
> c11dcee7583027 Stephane Grosjean 2021-07-15 462 /* allocate an skb to store the error frame */
> c11dcee7583027 Stephane Grosjean 2021-07-15 463 skb = alloc_can_err_skb(mc->netdev, &cf);
alloc_can_err_skb() calls alloc_can_skb() this looks like:
| struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
| {
| struct sk_buff *skb;
|
| skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
| sizeof(struct can_frame));
| if (unlikely(!skb)) {
| *cf = NULL;
|
| return NULL;
| }
[...]
| }
This means if "skb" is NULL, so is "cf".
> 46be265d338833 Stephane Grosjean 2012-03-02 464
> c11dcee7583027 Stephane Grosjean 2021-07-15 465 if (n & PCAN_USB_ERROR_RXQOVR) {
> c11dcee7583027 Stephane Grosjean 2021-07-15 466 /* data overrun interrupt */
> c11dcee7583027 Stephane Grosjean 2021-07-15 467 netdev_dbg(mc->netdev, "data overrun interrupt\n");
> c11dcee7583027 Stephane Grosjean 2021-07-15 468 mc->netdev->stats.rx_over_errors++;
> c11dcee7583027 Stephane Grosjean 2021-07-15 469 mc->netdev->stats.rx_errors++;
> c11dcee7583027 Stephane Grosjean 2021-07-15 470 if (cf) {
> ^^
> Check for NULL
>
> c11dcee7583027 Stephane Grosjean 2021-07-15 471 cf->can_id |= CAN_ERR_CRTL;
> c11dcee7583027 Stephane Grosjean 2021-07-15 472 cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
> 46be265d338833 Stephane Grosjean 2012-03-02 473 }
> 46be265d338833 Stephane Grosjean 2012-03-02 474 }
> 46be265d338833 Stephane Grosjean 2012-03-02 475
> c11dcee7583027 Stephane Grosjean 2021-07-15 476 if (n & PCAN_USB_ERROR_TXQFULL)
> c11dcee7583027 Stephane Grosjean 2021-07-15 477 netdev_dbg(mc->netdev, "device Tx queue full)\n");
> c11dcee7583027 Stephane Grosjean 2021-07-15 478
> 46be265d338833 Stephane Grosjean 2012-03-02 479 if (n & PCAN_USB_ERROR_BUS_OFF) {
> 46be265d338833 Stephane Grosjean 2012-03-02 480 new_state = CAN_STATE_BUS_OFF;
> c11dcee7583027 Stephane Grosjean 2021-07-15 481 } else if (n & PCAN_USB_ERROR_BUS_HEAVY) {
> c11dcee7583027 Stephane Grosjean 2021-07-15 482 new_state = ((mc->pdev->bec.txerr >= 128) ||
> c11dcee7583027 Stephane Grosjean 2021-07-15 483 (mc->pdev->bec.rxerr >= 128)) ?
> c11dcee7583027 Stephane Grosjean 2021-07-15 484 CAN_STATE_ERROR_PASSIVE :
> c11dcee7583027 Stephane Grosjean 2021-07-15 485 CAN_STATE_ERROR_WARNING;
> c11dcee7583027 Stephane Grosjean 2021-07-15 486 } else {
> c11dcee7583027 Stephane Grosjean 2021-07-15 487 new_state = CAN_STATE_ERROR_ACTIVE;
> 46be265d338833 Stephane Grosjean 2012-03-02 488 }
> 46be265d338833 Stephane Grosjean 2012-03-02 489
> c11dcee7583027 Stephane Grosjean 2021-07-15 490 /* handle change of state */
> c11dcee7583027 Stephane Grosjean 2021-07-15 491 if (new_state != mc->pdev->dev.can.state) {
> c11dcee7583027 Stephane Grosjean 2021-07-15 492 enum can_state tx_state =
> c11dcee7583027 Stephane Grosjean 2021-07-15 493 (mc->pdev->bec.txerr >= mc->pdev->bec.rxerr) ?
> c11dcee7583027 Stephane Grosjean 2021-07-15 494 new_state : 0;
> c11dcee7583027 Stephane Grosjean 2021-07-15 495 enum can_state rx_state =
> c11dcee7583027 Stephane Grosjean 2021-07-15 496 (mc->pdev->bec.txerr <= mc->pdev->bec.rxerr) ?
> c11dcee7583027 Stephane Grosjean 2021-07-15 497 new_state : 0;
> 46be265d338833 Stephane Grosjean 2012-03-02 498
> c11dcee7583027 Stephane Grosjean 2021-07-15 499 can_change_state(mc->netdev, cf, tx_state, rx_state);
> 46be265d338833 Stephane Grosjean 2012-03-02 500
> c11dcee7583027 Stephane Grosjean 2021-07-15 501 if (new_state == CAN_STATE_BUS_OFF) {
> 46be265d338833 Stephane Grosjean 2012-03-02 502 can_bus_off(mc->netdev);
> c11dcee7583027 Stephane Grosjean 2021-07-15 @503 } else if (cf && (cf->can_id & CAN_ERR_CRTL)) {
> ^^
> Check for NULL
>
> c11dcee7583027 Stephane Grosjean 2021-07-15 504 /* Supply TX/RX error counters in case of
> c11dcee7583027 Stephane Grosjean 2021-07-15 505 * controller error.
> c11dcee7583027 Stephane Grosjean 2021-07-15 506 */
> ea8b33bde76c8f Stephane Grosjean 2019-12-06 507 cf->data[6] = mc->pdev->bec.txerr;
> ea8b33bde76c8f Stephane Grosjean 2019-12-06 508 cf->data[7] = mc->pdev->bec.rxerr;
> ea8b33bde76c8f Stephane Grosjean 2019-12-06 509 }
> 46be265d338833 Stephane Grosjean 2012-03-02 510 }
> 46be265d338833 Stephane Grosjean 2012-03-02 511
> c11dcee7583027 Stephane Grosjean 2021-07-15 512 if (!skb)
> c11dcee7583027 Stephane Grosjean 2021-07-15 513 return -ENOMEM;
If cf is NULL, skb is NULL, and this function returns.
> 46be265d338833 Stephane Grosjean 2012-03-02 514
> 46be265d338833 Stephane Grosjean 2012-03-02 515 if (status_len & PCAN_USB_STATUSLEN_TIMESTAMP) {
> c9faaa09e2a133 Oliver Hartkopp 2012-11-21 516 struct skb_shared_hwtstamps *hwts = skb_hwtstamps(skb);
> c9faaa09e2a133 Oliver Hartkopp 2012-11-21 517
> d5888a1e75c799 Arnd Bergmann 2017-11-03 518 peak_usb_get_ts_time(&mc->pdev->time_ref, mc->ts16,
> d5888a1e75c799 Arnd Bergmann 2017-11-03 519 &hwts->hwtstamp);
> 46be265d338833 Stephane Grosjean 2012-03-02 520 }
> 46be265d338833 Stephane Grosjean 2012-03-02 521
> 46be265d338833 Stephane Grosjean 2012-03-02 522 mc->netdev->stats.rx_packets++;
> c7b74967799b1a Oliver Hartkopp 2020-11-20 @523 mc->netdev->stats.rx_bytes += cf->len;
> ^^^^^^^
> Unchecked dereference.
skb is not NULL here, so cf is valid, too.
> 1c0ee046957648 Marc Kleine-Budde 2015-07-11 524 netif_rx(skb);
> 46be265d338833 Stephane Grosjean 2012-03-02 525
> 46be265d338833 Stephane Grosjean 2012-03-02 526 return 0;
> 46be265d338833 Stephane Grosjean 2012-03-02 527 }
This pattern exists in probably more than one driver.
Should we extend the NULL pointer check to "cf", to make smatch happy?
> c11dcee7583027 Stephane Grosjean 2021-07-15 512 if (!skb || !cf)
> c11dcee7583027 Stephane Grosjean 2021-07-15 513 return -ENOMEM;
Marc
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [linux-next:master 2736/10638] drivers/net/can/usb/peak_usb/pcan_usb.c:523 pcan_usb_decode_error() error: we previously assumed 'cf' could be null (see line 503)
2021-08-27 9:20 ` Marc Kleine-Budde
@ 2021-08-27 9:54 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2021-08-27 9:54 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]
Yeah...
Thanks for looking at this. I should have caught it myself, because I
should have wondered why I hadn't sent a warning for this earlier from
my own test system.
The kbuild-bot doesn't do cross function analysis. (It adds
hours and hours to the build). When I build this on my system with
the cross function DB then Smatch parses it correctly.
On Fri, Aug 27, 2021 at 11:20:40AM +0200, Marc Kleine-Budde wrote:
> > 1c0ee046957648 Marc Kleine-Budde 2015-07-11 524 netif_rx(skb);
> > 46be265d338833 Stephane Grosjean 2012-03-02 525
> > 46be265d338833 Stephane Grosjean 2012-03-02 526 return 0;
> > 46be265d338833 Stephane Grosjean 2012-03-02 527 }
>
> This pattern exists in probably more than one driver.
>
> Should we extend the NULL pointer check to "cf", to make smatch happy?
My philosophy is always that we shouldn't add code just to make the
checker happy.
This is a complicated one because it requires cross function analysis.
When you built the smatch cross function DB, it parses every file and
outputs SQL to describe what the function does. Then you pipe the SQL
to sqlite3 which takes a few hours just because the DB is so huge (20GB).
Smatch also parses some functions inline in an in-memory database but
that can't work here because alloc_can_err_skb() is in a different .c
file.
I imagine that in a couple years, the first step of a Smatch run will be
to slurp every function into a database raw so that they can be parsed
inline. That would be a lot faster.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [linux-next:master 2736/10638] drivers/net/can/usb/peak_usb/pcan_usb.c:523 pcan_usb_decode_error() error: we previously assumed 'cf' could be null (see line 503)
@ 2021-08-27 9:54 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2021-08-27 9:54 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]
Yeah...
Thanks for looking at this. I should have caught it myself, because I
should have wondered why I hadn't sent a warning for this earlier from
my own test system.
The kbuild-bot doesn't do cross function analysis. (It adds
hours and hours to the build). When I build this on my system with
the cross function DB then Smatch parses it correctly.
On Fri, Aug 27, 2021 at 11:20:40AM +0200, Marc Kleine-Budde wrote:
> > 1c0ee046957648 Marc Kleine-Budde 2015-07-11 524 netif_rx(skb);
> > 46be265d338833 Stephane Grosjean 2012-03-02 525
> > 46be265d338833 Stephane Grosjean 2012-03-02 526 return 0;
> > 46be265d338833 Stephane Grosjean 2012-03-02 527 }
>
> This pattern exists in probably more than one driver.
>
> Should we extend the NULL pointer check to "cf", to make smatch happy?
My philosophy is always that we shouldn't add code just to make the
checker happy.
This is a complicated one because it requires cross function analysis.
When you built the smatch cross function DB, it parses every file and
outputs SQL to describe what the function does. Then you pipe the SQL
to sqlite3 which takes a few hours just because the DB is so huge (20GB).
Smatch also parses some functions inline in an in-memory database but
that can't work here because alloc_can_err_skb() is in a different .c
file.
I imagine that in a couple years, the first step of a Smatch run will be
to slurp every function into a database raw so that they can be parsed
inline. That would be a lot faster.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-08-27 9:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-27 4:39 [linux-next:master 2736/10638] drivers/net/can/usb/peak_usb/pcan_usb.c:523 pcan_usb_decode_error() error: we previously assumed 'cf' could be null (see line 503) kernel test robot
2021-08-27 8:05 ` Dan Carpenter
2021-08-27 9:20 ` Marc Kleine-Budde
2021-08-27 9:54 ` Dan Carpenter
2021-08-27 9:54 ` Dan Carpenter
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.