All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.