Linux Ethernet Bridging - Nov 2018 - [Bridge] [PATCH] net: bridge: check for a null p->dev before dereferencing it

From: Colin Ian King <colin.king at canonical.com>

A recent change added a null check on p->dev after p->dev was being
dereferenced by the ns_capable check on p->dev.  Fix this by performing
the p->dev sanity check before it is dereferenced.

Detected by CoverityScan, CID#751490 ("Dereference before null check")

Fixes: a5f3ea54f3cc ("net: bridge: add support for raw sysfs port
options")
Signed-off-by: Colin Ian King <colin.king at canonical.com>
---
 net/bridge/br_sysfs_if.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index 7c87a2fe5248..aab8aa17cccf 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -314,15 +314,15 @@ static ssize_t brport_store(struct kobject *kobj,
 	unsigned long val;
 	char *endp;
 
+	if (!p->dev || !p->br)
+		return -EINVAL;
+
 	if (!ns_capable(dev_net(p->dev)->user_ns, CAP_NET_ADMIN))
 		return -EPERM;
 
 	if (!rtnl_trylock())
 		return restart_syscall();
 
-	if (!p->dev || !p->br)
-		goto out_unlock;
-
 	if (brport_attr->store_raw) {
 		char *buf_copy;
 
-- 
2.19.1

On 24/11/2018 14:15, Colin King wrote:
> From: Colin Ian King <colin.king at canonical.com>
> 
> A recent change added a null check on p->dev after p->dev was being
> dereferenced by the ns_capable check on p->dev.  Fix this by performing
> the p->dev sanity check before it is dereferenced.
> 
> Detected by CoverityScan, CID#751490 ("Dereference before null
check")
> 
> Fixes: a5f3ea54f3cc ("net: bridge: add support for raw sysfs port
options")
> Signed-off-by: Colin Ian King <colin.king at canonical.com>
> ---
>  net/bridge/br_sysfs_if.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
> index 7c87a2fe5248..aab8aa17cccf 100644
> --- a/net/bridge/br_sysfs_if.c
> +++ b/net/bridge/br_sysfs_if.c
> @@ -314,15 +314,15 @@ static ssize_t brport_store(struct kobject *kobj,
>  	unsigned long val;
>  	char *endp;
>  
> +	if (!p->dev || !p->br)
> +		return -EINVAL;
> +
>  	if (!ns_capable(dev_net(p->dev)->user_ns, CAP_NET_ADMIN))
>  		return -EPERM;
>  
>  	if (!rtnl_trylock())
>  		return restart_syscall();
>  
> -	if (!p->dev || !p->br)
> -		goto out_unlock;
> -
>  	if (brport_attr->store_raw) {
>  		char *buf_copy;
>  
> 
Hi,
I was contacted recently about this privately and this was my reply:
"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.
"

So I think it's safe to remove those checks altogether. It'd be nice to
get a second
look over my reasoning as I might be missing something in sysfs/kernfs call
path.

Thanks,
 Nik