re: net: wireless: add brcm80211 drivers

re: net: wireless: add brcm80211 drivers

[Dan Carpenter]

Hi Arend,

This code is not really new, but I thought I would email you anyway
because I know you are responsive.  :)

The patch 5b435de0d786: "net: wireless: add brcm80211 drivers" from 
Oct 5, 2011, leads to the following Smatch complaint:

drivers/net/wireless/brcm80211/brcmsmac/ampdu.c:741 brcms_c_sendampdu()
	 warn: variable dereferenced before check 'p' (see line 739)

drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
   733                  /*
   734                   * check to see if the next pkt is
   735                   * a candidate for aggregation
   736                   */
   737                  p = pktq_ppeek(&qi->q, prec);
   738			/* tx_info must be checked with current p */
   739			tx_info = IEEE80211_SKB_CB(p);
                                  ^^^^^^^^^^^^^^^^
"p" is dereferenced inside the call to IEEE80211_SKB_CB().

   740	
   741			if (p) {
                           ^^^
Checked too late.

   742				if ((tx_info->flags & IEEE80211_TX_CTL_AMPDU) &&
   743				    ((u8) (p->priority) == tid)) {

regards,
dan carpenter

Re: net: wireless: add brcm80211 drivers

[Arend van Spriel]

On 05/13/2012 07:43 PM, Dan Carpenter wrote:
> Hi Arend,
> 
> This code is not really new, but I thought I would email you anyway
> because I know you are responsive.  :)

Dito ;-)

> The patch 5b435de0d786: "net: wireless: add brcm80211 drivers" from 
> Oct 5, 2011, leads to the following Smatch complaint:
> 
> drivers/net/wireless/brcm80211/brcmsmac/ampdu.c:741 brcms_c_sendampdu()
> 	 warn: variable dereferenced before check 'p' (see line 739)
> 
> drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
>    733                  /*
>    734                   * check to see if the next pkt is
>    735                   * a candidate for aggregation
>    736                   */
>    737                  p = pktq_ppeek(&qi->q, prec);
>    738			/* tx_info must be checked with current p */
>    739			tx_info = IEEE80211_SKB_CB(p);
>                                   ^^^^^^^^^^^^^^^^
> "p" is dereferenced inside the call to IEEE80211_SKB_CB().
> 
>    740	
>    741			if (p) {
>                            ^^^
> Checked too late.
> 
>    742				if ((tx_info->flags & IEEE80211_TX_CTL_AMPDU) &&
>    743				    ((u8) (p->priority) == tid)) {
> 
> regards,
> dan carpenter
> 
> 

Thanks for running smatch. I will look in the current code base and fix
this. Just out of curiosity: Another static checker used regularly is
Coccinelle. What are the pros and cons of smatch compared to Coccinelle?

Gr. AvS

Re: net: wireless: add brcm80211 drivers

[Dan Carpenter]

On Mon, May 14, 2012 at 09:07:55AM +0200, Arend van Spriel wrote:
> Thanks for running smatch. I will look in the current code base and fix
> this. Just out of curiosity: Another static checker used regularly is
> Coccinelle. What are the pros and cons of smatch compared to Coccinelle?
> 
> Gr. AvS

That's a tricky question because of my obvious bias as the author
of Smatch, and I'm not an expert on Coccinelle.  ;)

Smatch works on the preprocessed code and Coccinelle works on .c
code.  So some things are easier to check for in Coccinelle.  I've
generally found hacks to get the information I need in Smatch but
sometimes it's gnarly.  So a one liner in Coccinelle is twenty lines
of code in Smatch which require in depth knowledge of Smatch and
Sparse.

Smatch doesn't fix the bugs it finds.

The other advantage for Coccinelle is that you can run it on other
architecture's code without setting up a cross compile environment.

What Smatch does that Coccinelle doesn't is that it tries to track
the values of all the variables.  This means you can detect array
overflows, for example.  Smatch tries to track values across
function calls as well, with the recent database work.

That's really the long term goal of Smatch, to track the value of
every variable in the kernel.  But there is still a lot of work to
do.  :P

regards,
dan carpenter

Re: net: wireless: add brcm80211 drivers

[Arend van Spriel]

On 05/14/2012 10:34 AM, Dan Carpenter wrote:
> On Mon, May 14, 2012 at 09:07:55AM +0200, Arend van Spriel wrote:
>> Thanks for running smatch. I will look in the current code base and fix
>> this. Just out of curiosity: Another static checker used regularly is
>> Coccinelle. What are the pros and cons of smatch compared to Coccinelle?
>>
>> Gr. AvS
> 
> That's a tricky question because of my obvious bias as the author
> of Smatch, and I'm not an expert on Coccinelle.  ;)

 ;-)

> Smatch works on the preprocessed code and Coccinelle works on .c
> code.  So some things are easier to check for in Coccinelle.  I've
> generally found hacks to get the information I need in Smatch but
> sometimes it's gnarly.  So a one liner in Coccinelle is twenty lines
> of code in Smatch which require in depth knowledge of Smatch and
> Sparse.
> 
> Smatch doesn't fix the bugs it finds.

+1 for Coccinelle.

> The other advantage for Coccinelle is that you can run it on other
> architecture's code without setting up a cross compile environment.
> 
> What Smatch does that Coccinelle doesn't is that it tries to track
> the values of all the variables.  This means you can detect array
> overflows, for example.  Smatch tries to track values across
> function calls as well, with the recent database work.

I guess Oracle knows a thing or two about databases.

> That's really the long term goal of Smatch, to track the value of
> every variable in the kernel.  But there is still a lot of work to
> do.  :P

Enjoy and thanks for taking time to explain.

Gr. AvS

Re: net: wireless: add brcm80211 drivers

[Dan Carpenter]

On Mon, May 14, 2012 at 11:06:09AM +0200, Arend van Spriel wrote:
> On 05/14/2012 10:34 AM, Dan Carpenter wrote:
> > On Mon, May 14, 2012 at 09:07:55AM +0200, Arend van Spriel wrote:
> > What Smatch does that Coccinelle doesn't is that it tries to track
> > the values of all the variables.  This means you can detect array
> > overflows, for example.  Smatch tries to track values across
> > function calls as well, with the recent database work.
> 
> I guess Oracle knows a thing or two about databases.

Uh...  It just puts all the information of how the functions are
called into an sqlite3 database.  Arg 3 is a number 0-9.

regards,
dan carpenter

Re: net: wireless: add brcm80211 drivers

[Julia Lawall]

On Mon, 14 May 2012, Dan Carpenter wrote:

> On Mon, May 14, 2012 at 09:07:55AM +0200, Arend van Spriel wrote:
>> Thanks for running smatch. I will look in the current code base and fix
>> this. Just out of curiosity: Another static checker used regularly is
>> Coccinelle. What are the pros and cons of smatch compared to Coccinelle?
>>
>> Gr. AvS
>
> That's a tricky question because of my obvious bias as the author
> of Smatch, and I'm not an expert on Coccinelle.  ;)
>
> Smatch works on the preprocessed code and Coccinelle works on .c
> code.  So some things are easier to check for in Coccinelle.  I've
> generally found hacks to get the information I need in Smatch but
> sometimes it's gnarly.  So a one liner in Coccinelle is twenty lines
> of code in Smatch which require in depth knowledge of Smatch and
> Sparse.
>
> Smatch doesn't fix the bugs it finds.
>
> The other advantage for Coccinelle is that you can run it on other
> architecture's code without setting up a cross compile environment.
>
> What Smatch does that Coccinelle doesn't is that it tries to track
> the values of all the variables.  This means you can detect array
> overflows, for example.  Smatch tries to track values across
> function calls as well, with the recent database work.
>
> That's really the long term goal of Smatch, to track the value of
> every variable in the kernel.  But there is still a lot of work to
> do.  :P

That all seems accurate to me :)

julia

re: net: wireless: add brcm80211 drivers

[Dan Carpenter]

Hi Arend,

The patch 5b435de0d786: "net: wireless: add brcm80211 drivers" from
Oct 5, 2011, leads to the following warning:
drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c:1699 brcmf_sdbrcm_readframes()
	 warn: is it ok to set 'rxseq' to -1?

  1697                          cnt = brcmf_sdbrcm_rxglom(bus, rxseq);
  1698                          brcmf_dbg(GLOM, "rxglom returned %d\n", cnt);
  1699                          rxseq += cnt - 1;
  1700                          rxleft = (rxleft > cnt) ? (rxleft - cnt) : 1;
  1701                          continue;

This isn't a warning, it's just one of the things I audit.
brcmf_sdbrcm_rxglom() can return 0 so it's weird that we do
"rxseq += 0 - 1;"  Was that intended?

regards,
dan carpenter

Re: net: wireless: add brcm80211 drivers

[Arend van Spriel]

On 08/05/2012 09:57 PM, Dan Carpenter wrote:
> Hi Arend,
> 
> The patch 5b435de0d786: "net: wireless: add brcm80211 drivers" from
> Oct 5, 2011, leads to the following warning:
> drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c:1699 brcmf_sdbrcm_readframes()
> 	 warn: is it ok to set 'rxseq' to -1?
> 
>   1697                          cnt = brcmf_sdbrcm_rxglom(bus, rxseq);
>   1698                          brcmf_dbg(GLOM, "rxglom returned %d\n", cnt);
>   1699                          rxseq += cnt - 1;
>   1700                          rxleft = (rxleft > cnt) ? (rxleft - cnt) : 1;
>   1701                          continue;
> 
> This isn't a warning, it's just one of the things I audit.
> brcmf_sdbrcm_rxglom() can return 0 so it's weird that we do
> "rxseq += 0 - 1;"  Was that intended?
> 
> regards,
> dan carpenter
> 
> 

Thanks, Dan

Probably it was not intended, but we need a closer look. At rxseq should
never be -1.

Gr. AvS

re: net: wireless: add brcm80211 drivers

[Dan Carpenter]

Hello Arend van Spriel,

The patch 5b435de0d786: "net: wireless: add brcm80211 drivers" from
Oct 5, 2011, leads to the following static checker warning:

	drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c:26460 wlc_phy_rxcal_radio_setup_nphy()
	warn: mask and shift to zero

drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
 26452                                  } else {
 26453                                          pi->tx_rx_cal_radio_saveregs[4] =
 26454                                                  read_radio_reg(pi,
 26455                                                          RADIO_2056_RX_LNAA_TUNE
 26456                                                          | RADIO_2056_RX0);
 26457  
 26458                                          offtune_val =
 26459                                                  (pi->tx_rx_cal_radio_saveregs
 26460                                                   [2] & 0xF0) >> 8;

This is obviously nonsense code, but I have no idea what was intended.

 26461                                          offtune_val =
 26462                                                  (offtune_val <= 0x7) ? 0xF : 0;

This is perhaps a bug fix/work around for the earlier line.

 26463  
 26464                                          mod_radio_reg(pi,
 26465                                                        RADIO_2056_RX_LNAA_TUNE |
 26466                                                        RADIO_2056_RX0, 0xF0,
 26467                                                        (offtune_val << 8));
 26468                                  }

regards,
dan carpenter

Re: net: wireless: add brcm80211 drivers

[Dan Carpenter]

There were a some others as well:

drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c:26511 wlc_phy_rxcal_radio_setup_nphy() warn: mask and shift to zero
drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c:26585 wlc_phy_rxcal_radio_setup_nphy() warn: mask and shift to zero
drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c:26635 wlc_phy_rxcal_radio_setup_nphy() warn: mask and shift to zero

regards,
dan carpenter