From: Colin Ian King <[email protected]>
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 <[email protected]>
---
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 <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
From: Nikolay Aleksandrov <[email protected]>
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.