Re: Coverity issues in linux-wpan

[Alexander Aring <alex.aring@gmail.com>]

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