Linux Ethernet Bridging - Feb 2023 - [Bridge] [PATCH] net-bridge: fix unsafe dereference of potential null ptr in __vlan_del()

After having been compared to NULL value at br_vlan.c:399,
pointer 'p' is passed as 1st parameter in call to function
'nbp_vlan_set_vlan_dev_state' at br_vlan.c:420, 
where it is dereferenced at br_vlan.c:1722.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Alexander Sapozhnikov <alsp705 at gmail.com>
---
 net/bridge/br_vlan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index bc75fa1e4666..87091e270adf 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -417,7 +417,8 @@ static int __vlan_del(struct net_bridge_vlan *v)
 		rhashtable_remove_fast(&vg->vlan_hash, &v->vnode,
 				       br_vlan_rht_params);
 		__vlan_del_list(v);
-		nbp_vlan_set_vlan_dev_state(p, v->vid);
+		if (p)
+			nbp_vlan_set_vlan_dev_state(p, v->vid);
 		br_multicast_toggle_one_vlan(v, false);
 		br_multicast_port_ctx_deinit(&v->port_mcast_ctx);
 		call_rcu(&v->rcu, nbp_vlan_rcu_free);
-- 
2.34.1

From: Alexander Sapozhnikov <alsp705 at gmail.com>
Date: Fri, 17 Feb 2023 16:16:57 +0300
> [PATCH] net-bridge: fix unsafe dereference of potential null ptr in
> __vlan_del()
Must be:

[PATCH net] net: bridge: fix unsafe dereference of potential null ptr in
__vlan_del()
> After having been compared to NULL value at br_vlan.c:399,
> pointer 'p' is passed as 1st parameter in call to function
> 'nbp_vlan_set_vlan_dev_state' at br_vlan.c:420, 
> where it is dereferenced at br_vlan.c:1722.
Do you have a reproducer for this or it's purely hypothetical? I'm not
saying it's strongly required, at least I wouldn't require it, just
curious. And sometimes without a stable repro you should target
net-next, not net, but better ask the bridge maintainers re this.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
Here you should have a "Fixes:" tag with a reference to the commit
which
introduced this nullptr-deref.
> Signed-off-by: Alexander Sapozhnikov <alsp705 at gmail.com>
You have 2 copies of this patch in the list. The first one lacks proper
commit message, this one has it. But you didn't marked this resent as v2
(it's v2 in fact).
Please mark it accordingly next spin and don't forget always provide a
changelog between versions.
> ---
>  net/bridge/br_vlan.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index bc75fa1e4666..87091e270adf 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -417,7 +417,8 @@ static int __vlan_del(struct net_bridge_vlan *v)
>  		rhashtable_remove_fast(&vg->vlan_hash, &v->vnode,
>  				       br_vlan_rht_params);
>  		__vlan_del_list(v);
> -		nbp_vlan_set_vlan_dev_state(p, v->vid);
> +		if (p)
> +			nbp_vlan_set_vlan_dev_state(p, v->vid);
>  		br_multicast_toggle_one_vlan(v, false);
>  		br_multicast_port_ctx_deinit(&v->port_mcast_ctx);
>  		call_rcu(&v->rcu, nbp_vlan_rcu_free);
Thanks,
Olek

On 17/02/2023 15:16, Alexander Sapozhnikov wrote:
> After having been compared to NULL value at br_vlan.c:399,
> pointer 'p' is passed as 1st parameter in call to function
> 'nbp_vlan_set_vlan_dev_state' at br_vlan.c:420, 
> where it is dereferenced at br_vlan.c:1722.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Alexander Sapozhnikov <alsp705 at gmail.com>
> ---
>  net/bridge/br_vlan.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index bc75fa1e4666..87091e270adf 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -417,7 +417,8 @@ static int __vlan_del(struct net_bridge_vlan *v)
>  		rhashtable_remove_fast(&vg->vlan_hash, &v->vnode,
>  				       br_vlan_rht_params);
>  		__vlan_del_list(v);
> -		nbp_vlan_set_vlan_dev_state(p, v->vid);
> +		if (p)
> +			nbp_vlan_set_vlan_dev_state(p, v->vid);
>  		br_multicast_toggle_one_vlan(v, false);
>  		br_multicast_port_ctx_deinit(&v->port_mcast_ctx);
>  		call_rcu(&v->rcu, nbp_vlan_rcu_free);
This cannot happen, read the code more carefully.
If you have a trace or have hit a bug, please provide the log.
Thanks,

Nacked-by: Nikolay Aleksandrov <razor at blackwall.org>