Coverity issues in linux-wpan

Coverity issues in linux-wpan

[Stefan Schmidt]

Hello.

I gave the currently found coverity issues in the linux-wpan area a 
look. The analyzing builds are not tracking bluetooth-next but go with 
Linus tree and get updated from time to time.

As I'm not sure if all people here have access to this (you should 
request it, its useful) I will cite the reported issues here with some 
context.

 From what I can see 5 issues have been detected. Two in mac802154 and 
three in our drivers (cc2520 and mrf24j40).

mrf24j40 (CID: 1226982, 1226983)
o Return value with error codes gets overridden before returning.
o I have a patch for that which I will send right after this mail.

cc2520 (CID: 1230469)
o Unchecked return value.
o Should we check here as we do in the other cases or not?

622 if (filt->pan_coord)
CID 1230469 (#1 of 2): Unchecked return value (CHECKED_RETURN)
<https://scan5.coverity.com:8443/defectInstanceId=22519459&fileInstanceId=73901097&mergedDefectId=1230469>
623 cc2520_write_register(priv, CC2520_FRMFILT0, 0x02);
624 else
CID 1230469 (#2 of 2): Unchecked return value (CHECKED_RETURN)10. 
check_return: Calling cc2520_write_register without checking return 
value (as is done elsewhere 21 out of 24 times).
625 cc2520_write_register(priv, CC2520_FRMFILT0, 0x00);

mac802154/iface.c (CID: 1268751)
o WARN_ON clears the parameter so we can't check for it afterwards to 
hit the error path

147 if (!local->open_count) {
148 res = drv_start(local);
cond_const: Condition res, taking false branch. Now the value of res is 
equal to 0.
149 WARN_ON(res);
const: At condition res, the value of res must be equal to 
0.dead_error_condition: The condition res cannot be true.
150 if (res)
CID 1268751 (#1 of 1): Logically dead code (DEADCODE)dead_error_line: 
Execution cannot reach this statement: goto err;.
151 goto err;
152        }


mac802154/rx.c (CID: 1260107)
o Coverity thinks hdr.seq might be used unitiliazed in the pr_debug from 
ieee802154_parse_frame_start (line 149)
o Setting hdr.seq = 0; here would not hurt but I still wonder how 
coverity comes to think that as we pull the first three byte of the 
header before which include fc and seq.

199 struct ieee802154_sub_if_data *sdata;
1. var_decl: Declaring variable hdr without initializer.
200 struct ieee802154_hdr hdr;
201
CID 1260107 (#1 of 1): Uninitialized scalar variable (UNINIT)2. 
uninit_use_in_call: Using uninitialized value hdr.seq when calling 
ieee802154_parse_frame_start.
<https://scan5.coverity.com:8443/eventId=22526690-1&modelId=22526690-0&fileInstanceId=73892728&filePath=%2Fnet%2Fmac802154%2Frx.c&fileStart=136&fileEnd=192>
202 ret = ieee802154_parse_frame_start(skb, &hdr);


regards
Stefan Schmidt

Re: Coverity issues in linux-wpan

[Alexander Aring]

On Mon, Jun 08, 2015 at 03:05:54PM +0200, Stefan Schmidt wrote:
> Hello.
> 
> I gave the currently found coverity issues in the linux-wpan area a look.
> The analyzing builds are not tracking bluetooth-next but go with Linus tree
> and get updated from time to time.
> 
> As I'm not sure if all people here have access to this (you should request
> it, its useful) I will cite the reported issues here with some context.
> 
> From what I can see 5 issues have been detected. Two in mac802154 and three
> in our drivers (cc2520 and mrf24j40).
> 
> mrf24j40 (CID: 1226982, 1226983)
> o Return value with error codes gets overridden before returning.
> o I have a patch for that which I will send right after this mail.
> 
> cc2520 (CID: 1230469)
> o Unchecked return value.
> o Should we check here as we do in the other cases or not?
> 
> 622 if (filt->pan_coord)
> CID 1230469 (#1 of 2): Unchecked return value (CHECKED_RETURN)
> <https://scan5.coverity.com:8443/defectInstanceId=22519459&fileInstanceId=73901097&mergedDefectId=1230469>
> 623 cc2520_write_register(priv, CC2520_FRMFILT0, 0x02);
> 624 else
> CID 1230469 (#2 of 2): Unchecked return value (CHECKED_RETURN)10.
> check_return: Calling cc2520_write_register without checking return value
> (as is done elsewhere 21 out of 24 times).
> 625 cc2520_write_register(priv, CC2520_FRMFILT0, 0x00);
> 
> mac802154/iface.c (CID: 1268751)
> o WARN_ON clears the parameter so we can't check for it afterwards to hit
> the error path
> 
> 147 if (!local->open_count) {
> 148 res = drv_start(local);
> cond_const: Condition res, taking false branch. Now the value of res is
> equal to 0.
> 149 WARN_ON(res);
> const: At condition res, the value of res must be equal to
> 0.dead_error_condition: The condition res cannot be true.
> 150 if (res)
> CID 1268751 (#1 of 1): Logically dead code (DEADCODE)dead_error_line:
> Execution cannot reach this statement: goto err;.
> 151 goto err;
> 152        }
> 

Simple remove the WARN_ON or do WARN_ON(1) here, I would remove it.

I remember this section [0]. I leave the old beheavior which was not
like wireless code. Nevertheless I had some bad feeling when I saw this
and doing some code around. This behaviour was always there.

Comparing with wireless code, see [2].

> 
> mac802154/rx.c (CID: 1260107)
> o Coverity thinks hdr.seq might be used unitiliazed in the pr_debug from
> ieee802154_parse_frame_start (line 149)
> o Setting hdr.seq = 0; here would not hurt but I still wonder how coverity
> comes to think that as we pull the first three byte of the header before
> which include fc and seq.
> 
> 199 struct ieee802154_sub_if_data *sdata;
> 1. var_decl: Declaring variable hdr without initializer.
> 200 struct ieee802154_hdr hdr;
> 201
> CID 1260107 (#1 of 1): Uninitialized scalar variable (UNINIT)2.
> uninit_use_in_call: Using uninitialized value hdr.seq when calling
> ieee802154_parse_frame_start.
> <https://scan5.coverity.com:8443/eventId=22526690-1&modelId=22526690-0&fileInstanceId=73892728&filePath=%2Fnet%2Fmac802154%2Frx.c&fileStart=136&fileEnd=192>
> 202 ret = ieee802154_parse_frame_start(skb, &hdr);
> 

I think the hdr.seq should be filled by doing [1].


- Alex

[0] http://lxr.free-electrons.com/source/net/mac802154/ieee802154_dev.c?v=3.18#L63
[1] http://lxr.free-electrons.com/source/net/ieee802154/header_ops.c#L249
[2] http://lxr.free-electrons.com/source/net/mac80211/iface.c#L547

Re: Coverity issues in linux-wpan

[Stefan Schmidt]

Hello.

On 08/06/15 19:04, Alexander Aring wrote:
> On Mon, Jun 08, 2015 at 03:05:54PM +0200, Stefan Schmidt wrote:
>> Hello.
>>
>> I gave the currently found coverity issues in the linux-wpan area a look.
>> The analyzing builds are not tracking bluetooth-next but go with Linus tree
>> and get updated from time to time.
>>
>> As I'm not sure if all people here have access to this (you should request
>> it, its useful) I will cite the reported issues here with some context.
>>
>>  From what I can see 5 issues have been detected. Two in mac802154 and three
>> in our drivers (cc2520 and mrf24j40).
>>
>> mrf24j40 (CID: 1226982, 1226983)
>> o Return value with error codes gets overridden before returning.
>> o I have a patch for that which I will send right after this mail.
>>
>> cc2520 (CID: 1230469)
>> o Unchecked return value.
>> o Should we check here as we do in the other cases or not?
>>
>> 622 if (filt->pan_coord)
>> CID 1230469 (#1 of 2): Unchecked return value (CHECKED_RETURN)
>> <https://scan5.coverity.com:8443/defectInstanceId=22519459&fileInstanceId=73901097&mergedDefectId=1230469>
>> 623 cc2520_write_register(priv, CC2520_FRMFILT0, 0x02);
>> 624 else
>> CID 1230469 (#2 of 2): Unchecked return value (CHECKED_RETURN)10.
>> check_return: Calling cc2520_write_register without checking return value
>> (as is done elsewhere 21 out of 24 times).
>> 625 cc2520_write_register(priv, CC2520_FRMFILT0, 0x00);
>>
>> mac802154/iface.c (CID: 1268751)
>> o WARN_ON clears the parameter so we can't check for it afterwards to hit
>> the error path
>>
>> 147 if (!local->open_count) {
>> 148 res = drv_start(local);
>> cond_const: Condition res, taking false branch. Now the value of res is
>> equal to 0.
>> 149 WARN_ON(res);
>> const: At condition res, the value of res must be equal to
>> 0.dead_error_condition: The condition res cannot be true.
>> 150 if (res)
>> CID 1268751 (#1 of 1): Logically dead code (DEADCODE)dead_error_line:
>> Execution cannot reach this statement: goto err;.
>> 151 goto err;
>> 152        }
>>
> Simple remove the WARN_ON or do WARN_ON(1) here, I would remove it.
>
> I remember this section [0]. I leave the old beheavior which was not
> like wireless code. Nevertheless I had some bad feeling when I saw this
> and doing some code around. This behaviour was always there.
>
> Comparing with wireless code, see [2].
OK, this makes more sense now. I send a patch removing it. That way it 
works like we intend it.

>> mac802154/rx.c (CID: 1260107)
>> o Coverity thinks hdr.seq might be used unitiliazed in the pr_debug from
>> ieee802154_parse_frame_start (line 149)
>> o Setting hdr.seq = 0; here would not hurt but I still wonder how coverity
>> comes to think that as we pull the first three byte of the header before
>> which include fc and seq.
>>
>> 199 struct ieee802154_sub_if_data *sdata;
>> 1. var_decl: Declaring variable hdr without initializer.
>> 200 struct ieee802154_hdr hdr;
>> 201
>> CID 1260107 (#1 of 1): Uninitialized scalar variable (UNINIT)2.
>> uninit_use_in_call: Using uninitialized value hdr.seq when calling
>> ieee802154_parse_frame_start.
>> <https://scan5.coverity.com:8443/eventId=22526690-1&modelId=22526690-0&fileInstanceId=73892728&filePath=%2Fnet%2Fmac802154%2Frx.c&fileStart=136&fileEnd=192>
>> 202 ret = ieee802154_parse_frame_start(skb, &hdr);
>>
> I think the hdr.seq should be filled by doing [1].
>
Agreed. That is also what I think where the seq should be filled from. 
No idea why coverity gets this wrong. For now I consider this a false 
positive and no real issue for us.

That leaves only the cc2520 report open. Hopefully Varka has some 
comment on this. We can either assume that ignoring the return in these 
cases is fine or we add the error handling for them.

regards
Stefan Schmidt

Re: Coverity issues in linux-wpan

[Varka Bhadram]

Hi,

On 06/09/2015 01:40 AM, Stefan Schmidt wrote:

> Hello.
>
> On 08/06/15 19:04, Alexander Aring wrote:
>> On Mon, Jun 08, 2015 at 03:05:54PM +0200, Stefan Schmidt wrote:
>>> Hello.
>>>
>>> I gave the currently found coverity issues in the linux-wpan area a 
>>> look.
>>> The analyzing builds are not tracking bluetooth-next but go with 
>>> Linus tree
>>> and get updated from time to time.
>>>
>>> As I'm not sure if all people here have access to this (you should 
>>> request
>>> it, its useful) I will cite the reported issues here with some context.
>>>
>>>  From what I can see 5 issues have been detected. Two in mac802154 
>>> and three
>>> in our drivers (cc2520 and mrf24j40).
>>>
>>> mrf24j40 (CID: 1226982, 1226983)
>>> o Return value with error codes gets overridden before returning.
>>> o I have a patch for that which I will send right after this mail.
>>>
>>> cc2520 (CID: 1230469)
>>> o Unchecked return value.
>>> o Should we check here as we do in the other cases or not?
>>>
>>> 622 if (filt->pan_coord)
>>> CID 1230469 (#1 of 2): Unchecked return value (CHECKED_RETURN)
>>> <https://scan5.coverity.com:8443/defectInstanceId=22519459&fileInstanceId=73901097&mergedDefectId=1230469> 
>>>
>>> 623 cc2520_write_register(priv, CC2520_FRMFILT0, 0x02);
>>> 624 else
>>> CID 1230469 (#2 of 2): Unchecked return value (CHECKED_RETURN)10.
>>> check_return: Calling cc2520_write_register without checking return 
>>> value
>>> (as is done elsewhere 21 out of 24 times).
>>> 625 cc2520_write_register(priv, CC2520_FRMFILT0, 0x00);
>>>
>>> mac802154/iface.c (CID: 1268751)
>>> o WARN_ON clears the parameter so we can't check for it afterwards 
>>> to hit
>>> the error path
>>>
>>> 147 if (!local->open_count) {
>>> 148 res = drv_start(local);
>>> cond_const: Condition res, taking false branch. Now the value of res is
>>> equal to 0.
>>> 149 WARN_ON(res);
>>> const: At condition res, the value of res must be equal to
>>> 0.dead_error_condition: The condition res cannot be true.
>>> 150 if (res)
>>> CID 1268751 (#1 of 1): Logically dead code (DEADCODE)dead_error_line:
>>> Execution cannot reach this statement: goto err;.
>>> 151 goto err;
>>> 152        }
>>>
>> Simple remove the WARN_ON or do WARN_ON(1) here, I would remove it.
>>
>> I remember this section [0]. I leave the old beheavior which was not
>> like wireless code. Nevertheless I had some bad feeling when I saw this
>> and doing some code around. This behaviour was always there.
>>
>> Comparing with wireless code, see [2].
> OK, this makes more sense now. I send a patch removing it. That way it 
> works like we intend it.
>
>>> mac802154/rx.c (CID: 1260107)
>>> o Coverity thinks hdr.seq might be used unitiliazed in the pr_debug 
>>> from
>>> ieee802154_parse_frame_start (line 149)
>>> o Setting hdr.seq = 0; here would not hurt but I still wonder how 
>>> coverity
>>> comes to think that as we pull the first three byte of the header 
>>> before
>>> which include fc and seq.
>>>
>>> 199 struct ieee802154_sub_if_data *sdata;
>>> 1. var_decl: Declaring variable hdr without initializer.
>>> 200 struct ieee802154_hdr hdr;
>>> 201
>>> CID 1260107 (#1 of 1): Uninitialized scalar variable (UNINIT)2.
>>> uninit_use_in_call: Using uninitialized value hdr.seq when calling
>>> ieee802154_parse_frame_start.
>>> <https://scan5.coverity.com:8443/eventId=22526690-1&modelId=22526690-0&fileInstanceId=73892728&filePath=%2Fnet%2Fmac802154%2Frx.c&fileStart=136&fileEnd=192> 
>>>
>>> 202 ret = ieee802154_parse_frame_start(skb, &hdr);
>>>
>> I think the hdr.seq should be filled by doing [1].
>>
> Agreed. That is also what I think where the seq should be filled from. 
> No idea why coverity gets this wrong. For now I consider this a false 
> positive and no real issue for us.
>
> That leaves only the cc2520 report open. Hopefully Varka has some 
> comment on this. We can either assume that ignoring the return in 
> these cases is fine or we add the error handling for them.

mhhh.. Its better to have the error handling at this place.

Please send the patch.

-- 
Varka Bhadram