Linux Ethernet Bridging - Oct 2018 - [Bridge] [bug report] net: bridge: add support for raw sysfs port options

Hello Nikolay Aleksandrov,

This is a semi-automatic email about new static checker warnings.

The patch a5f3ea54f3cc: "net: bridge: add support for raw sysfs port 
options" from Jul 23, 2018, leads to the following Smatch complaint:

    net/bridge/br_sysfs_if.c:323 brport_store()
     warn: variable dereferenced before check 'p->dev' (see line 317)

net/bridge/br_sysfs_if.c
   316	
   317		if (!ns_capable(dev_net(p->dev)->user_ns, CAP_NET_ADMIN))
                                        ^^^^^^
Dereferenced here.

   318			return -EPERM;
   319	
   320		if (!rtnl_trylock())
   321			return restart_syscall();
   322	
   323		if (!p->dev || !p->br)
                    ^^^^^^^
Hopefully this can't happen?

   324			goto out_unlock;
   325	

regards,
dan carpenter

On 30/10/2018 11:18, Dan Carpenter wrote:
> Hello Nikolay Aleksandrov,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch a5f3ea54f3cc: "net: bridge: add support for raw sysfs port 
> options" from Jul 23, 2018, leads to the following Smatch complaint:
> 
>     net/bridge/br_sysfs_if.c:323 brport_store()
>      warn: variable dereferenced before check 'p->dev' (see line
317)
> 
> net/bridge/br_sysfs_if.c
>    316	
>    317		if (!ns_capable(dev_net(p->dev)->user_ns, CAP_NET_ADMIN))
>                                         ^^^^^^
> Dereferenced here.
> 
>    318			return -EPERM;
>    319	
>    320		if (!rtnl_trylock())
>    321			return restart_syscall();
>    322	
>    323		if (!p->dev || !p->br)
>                     ^^^^^^^
> Hopefully this can't happen?
> 
>    324			goto out_unlock;
>    325	
> 
> regards,
> dan carpenter
> 
Hi Dan,
Thank you for the report, but I think these checks are there for historic
reasons.
Checking new_nbp() and del_nbp() it should not be possible to reach that code
with p->dev or p->br as NULL. The cap check code has always been there, I
just
shuffled the rest of the function to obtain rtnl lock and kept it as close to
the original as possible, thus the checks remained.
In order to avoid future reports like this I'll send a cleanup once net-next
opens up.

My reasoning of why it shouldn't be possible:
- On port add new_nbp() sets both p->dev and p->br before creating
kobj/sysfs

- On port del (trickier) del_nbp() calls kobject_del() before call_rcu() to
destroy
  the port which in turn calls sysfs_remove_dir() which uses kernfs_remove()
which
  deactivates (shouldn't be able to open new files) and calls kernfs_drain()
to drain
  current open/mmaped files in the respective dir before continuing, thus making
it
  impossible to open a bridge port sysfs file with p->dev and p->br equal
to NULL.


I'll move the null checks above the cap check.

Thanks again,
 Nik