[PATCH] net: bridge: check for a null p->dev before dereferencing it

[PATCH] net: bridge: check for a null p->dev before dereferencing it

[Colin King]

From: Colin Ian King <colin.king@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@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

Re: [PATCH] net: bridge: check for a null p->dev before dereferencing it

[Nikolay Aleksandrov]

On 24/11/2018 14:15, Colin King wrote:
> From: Colin Ian King <colin.king@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@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

Re: [PATCH] net: bridge: check for a null p->dev before dereferencing it

[David Miller]

From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Sat, 24 Nov 2018 14:21:07 +0200

> 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.

I did a once over your analysis and I agree, the checks should be safe to remove.