[PATCH] bridge: netlink: check vlan_default_pvid range

[PATCH] bridge: netlink: check vlan_default_pvid range

[Tobias Jungel]

Currently it is allowed to set the default pvid of a bridge to a value
above VLAN_VID_MASK (0xfff). This patch checks the passed pvid and
disables the pvid in case it is out of bounds.

Reproduce by calling:

[root@test ~]# ip l a type bridge
[root@test ~]# ip l a type dummy
[root@test ~]# ip l s bridge0 type bridge vlan_filtering 1
[root@test ~]# ip l s bridge0 type bridge vlan_default_pvid 9999
[root@test ~]# ip l s dummy0 master bridge0
[root@test ~]# bridge vlan
port	vlan ids
bridge0	 9999 PVID Egress Untagged

dummy0	 9999 PVID Egress Untagged
---
 net/bridge/br_vlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index b838213..e363d75 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -820,7 +820,7 @@ int __br_vlan_set_default_pvid(struct net_bridge *br, u16 pvid)
 	int err = 0;
 	unsigned long *changed;
 
-	if (!pvid) {
+	if (!pvid || pvid >= VLAN_VID_MASK) {
 		br_vlan_disable_default_pvid(br);
 		return 0;
 	}
-- 
2.9.4

Re: [PATCH] bridge: netlink: check vlan_default_pvid range

[Sabrina Dubroca]

Hi Tobias,

2017-05-15, 13:08:19 +0200, Tobias Jungel wrote:
> Currently it is allowed to set the default pvid of a bridge to a value
> above VLAN_VID_MASK (0xfff). This patch checks the passed pvid and
> disables the pvid in case it is out of bounds.

Could we return an error (-EINVAL) to userspace instead?  Silently
disabling the feature seems confusing to me.  This would probably be
better in br_validate() (like the IFLA_BR_VLAN_PROTOCOL check), since
there's already such a check when setting default_pvid from sysfs (in
br_vlan_set_default_pvid()).

> 
> Reproduce by calling:
> 
> [root@test ~]# ip l a type bridge
> [root@test ~]# ip l a type dummy
> [root@test ~]# ip l s bridge0 type bridge vlan_filtering 1
> [root@test ~]# ip l s bridge0 type bridge vlan_default_pvid 9999
> [root@test ~]# ip l s dummy0 master bridge0
> [root@test ~]# bridge vlan
> port	vlan ids
> bridge0	 9999 PVID Egress Untagged
> 
> dummy0	 9999 PVID Egress Untagged

You'll also need to add a Signed-off-by, and a Fixes tag would be nice.


Thanks,

-- 
Sabrina

Re: [PATCH] bridge: netlink: check vlan_default_pvid range

[Tobias Jungel]

Thanks Sabrina and Nik.

On Mon, 2017-05-15 at 14:01 +0200, Sabrina Dubroca wrote:
> Hi Tobias,
> 
> 2017-05-15, 13:08:19 +0200, Tobias Jungel wrote:
> > Currently it is allowed to set the default pvid of a bridge to a
> > value
> > above VLAN_VID_MASK (0xfff). This patch checks the passed pvid and
> > disables the pvid in case it is out of bounds.
> 
> Could we return an error (-EINVAL) to userspace instead?  Silently
> disabling the feature seems confusing to me.  This would probably be
> better in br_validate() (like the IFLA_BR_VLAN_PROTOCOL check), since
> there's already such a check when setting default_pvid from sysfs (in
> br_vlan_set_default_pvid()).

I will send a v2 that returns -EINVAL. br_validate seems to be the
wrong place to me since it deals with the bridge ports.

> 
> > 
> > Reproduce by calling:
> > 
> > [root@test ~]# ip l a type bridge
> > [root@test ~]# ip l a type dummy
> > [root@test ~]# ip l s bridge0 type bridge vlan_filtering 1
> > [root@test ~]# ip l s bridge0 type bridge vlan_default_pvid 9999
> > [root@test ~]# ip l s dummy0 master bridge0
> > [root@test ~]# bridge vlan
> > port	vlan ids
> > bridge0	 9999 PVID Egress Untagged
> > 
> > dummy0	 9999 PVID Egress Untagged
> 
> You'll also need to add a Signed-off-by, and a Fixes tag would be
> nice.
> 

Right, will add this as well.

> 
> Thanks,
> 

Re: [PATCH] bridge: netlink: check vlan_default_pvid range

[Nikolay Aleksandrov]

On 5/15/17 4:21 PM, Tobias Jungel wrote:
> Thanks Sabrina and Nik.
> 
> On Mon, 2017-05-15 at 14:01 +0200, Sabrina Dubroca wrote:
>> Hi Tobias,
>>
>> 2017-05-15, 13:08:19 +0200, Tobias Jungel wrote:
>>> Currently it is allowed to set the default pvid of a bridge to a
>>> value
>>> above VLAN_VID_MASK (0xfff). This patch checks the passed pvid and
>>> disables the pvid in case it is out of bounds.
>>
>> Could we return an error (-EINVAL) to userspace instead?  Silently
>> disabling the feature seems confusing to me.  This would probably be
>> better in br_validate() (like the IFLA_BR_VLAN_PROTOCOL check), since
>> there's already such a check when setting default_pvid from sysfs (in
>> br_vlan_set_default_pvid()).
> 
> I will send a v2 that returns -EINVAL. br_validate seems to be the
> wrong place to me since it deals with the bridge ports.
> 

Could you elaborate ? br_validate should be called for all and is a very good
suggestion.

>>
>>>
>>> Reproduce by calling:
>>>
>>> [root@test ~]# ip l a type bridge
>>> [root@test ~]# ip l a type dummy
>>> [root@test ~]# ip l s bridge0 type bridge vlan_filtering 1
>>> [root@test ~]# ip l s bridge0 type bridge vlan_default_pvid 9999
>>> [root@test ~]# ip l s dummy0 master bridge0
>>> [root@test ~]# bridge vlan
>>> port	vlan ids
>>> bridge0	 9999 PVID Egress Untagged
>>>
>>> dummy0	 9999 PVID Egress Untagged
>>
>> You'll also need to add a Signed-off-by, and a Fixes tag would be
>> nice.
>>
> 
> Right, will add this as well.
> 
>>
>> Thanks,
>>

Re: [PATCH] bridge: netlink: check vlan_default_pvid range

[Nikolay Aleksandrov]

On 5/15/17 4:29 PM, Nikolay Aleksandrov wrote:
> On 5/15/17 4:21 PM, Tobias Jungel wrote:
>> Thanks Sabrina and Nik.
>>
>> On Mon, 2017-05-15 at 14:01 +0200, Sabrina Dubroca wrote:
>>> Hi Tobias,
>>>
>>> 2017-05-15, 13:08:19 +0200, Tobias Jungel wrote:
>>>> Currently it is allowed to set the default pvid of a bridge to a
>>>> value
>>>> above VLAN_VID_MASK (0xfff). This patch checks the passed pvid and
>>>> disables the pvid in case it is out of bounds.
>>>
>>> Could we return an error (-EINVAL) to userspace instead?  Silently
>>> disabling the feature seems confusing to me.  This would probably be
>>> better in br_validate() (like the IFLA_BR_VLAN_PROTOCOL check), since
>>> there's already such a check when setting default_pvid from sysfs (in
>>> br_vlan_set_default_pvid()).
>>
>> I will send a v2 that returns -EINVAL. br_validate seems to be the
>> wrong place to me since it deals with the bridge ports.
>>
> 
> Could you elaborate ? br_validate should be called for all and is a very good
> suggestion.

I meant for the bridge newlink/changelink of course. :-)

> 
>>>
>>>>
>>>> Reproduce by calling:
>>>>
>>>> [root@test ~]# ip l a type bridge
>>>> [root@test ~]# ip l a type dummy
>>>> [root@test ~]# ip l s bridge0 type bridge vlan_filtering 1
>>>> [root@test ~]# ip l s bridge0 type bridge vlan_default_pvid 9999
>>>> [root@test ~]# ip l s dummy0 master bridge0
>>>> [root@test ~]# bridge vlan
>>>> port    vlan ids
>>>> bridge0     9999 PVID Egress Untagged
>>>>
>>>> dummy0     9999 PVID Egress Untagged
>>>
>>> You'll also need to add a Signed-off-by, and a Fixes tag would be
>>> nice.
>>>
>>
>> Right, will add this as well.
>>
>>>
>>> Thanks,
>>>
> 

Re: [PATCH] bridge: netlink: check vlan_default_pvid range

[Tobias Jungel]

On Mon, 2017-05-15 at 16:31 +0300, Nikolay Aleksandrov wrote:
> On 5/15/17 4:29 PM, Nikolay Aleksandrov wrote:
> > On 5/15/17 4:21 PM, Tobias Jungel wrote:
> > > Thanks Sabrina and Nik.
> > > 
> > > On Mon, 2017-05-15 at 14:01 +0200, Sabrina Dubroca wrote:
> > > > Hi Tobias,
> > > > 
> > > > 2017-05-15, 13:08:19 +0200, Tobias Jungel wrote:
> > > > > Currently it is allowed to set the default pvid of a bridge
> > > > > to a
> > > > > value
> > > > > above VLAN_VID_MASK (0xfff). This patch checks the passed
> > > > > pvid and
> > > > > disables the pvid in case it is out of bounds.
> > > > 
> > > > Could we return an error (-EINVAL) to userspace
> > > > instead?  Silently
> > > > disabling the feature seems confusing to me.  This would
> > > > probably be
> > > > better in br_validate() (like the IFLA_BR_VLAN_PROTOCOL check),
> > > > since
> > > > there's already such a check when setting default_pvid from
> > > > sysfs (in
> > > > br_vlan_set_default_pvid()).
> > > 
> > > I will send a v2 that returns -EINVAL. br_validate seems to be
> > > the
> > > wrong place to me since it deals with the bridge ports.
> > > 
> > 
> > Could you elaborate ? br_validate should be called for all and is a
> > very good
> > suggestion.
> 
> I meant for the bridge newlink/changelink of course. :-)

Sorry had a wrong understanding of that function. Will come up with a
v3 later.

> 
> > 
> > > > 
> > > > > 
> > > > > Reproduce by calling:
> > > > > 
> > > > > [root@test ~]# ip l a type bridge
> > > > > [root@test ~]# ip l a type dummy
> > > > > [root@test ~]# ip l s bridge0 type bridge vlan_filtering 1
> > > > > [root@test ~]# ip l s bridge0 type bridge vlan_default_pvid
> > > > > 9999
> > > > > [root@test ~]# ip l s dummy0 master bridge0
> > > > > [root@test ~]# bridge vlan
> > > > > port    vlan ids
> > > > > bridge0     9999 PVID Egress Untagged
> > > > > 
> > > > > dummy0     9999 PVID Egress Untagged
> > > > 
> > > > You'll also need to add a Signed-off-by, and a Fixes tag would
> > > > be
> > > > nice.
> > > > 
> > > 
> > > Right, will add this as well.
> > > 
> > > > 
> > > > Thanks,
> > > > 
> 
> 

Re: [PATCH] bridge: netlink: check vlan_default_pvid range

[Nikolay Aleksandrov]

On 5/15/17 2:08 PM, Tobias Jungel wrote:
> Currently it is allowed to set the default pvid of a bridge to a value
> above VLAN_VID_MASK (0xfff). This patch checks the passed pvid and
> disables the pvid in case it is out of bounds.
> 
> Reproduce by calling:
> 
> [root@test ~]# ip l a type bridge
> [root@test ~]# ip l a type dummy
> [root@test ~]# ip l s bridge0 type bridge vlan_filtering 1
> [root@test ~]# ip l s bridge0 type bridge vlan_default_pvid 9999
> [root@test ~]# ip l s dummy0 master bridge0
> [root@test ~]# bridge vlan
> port	vlan ids
> bridge0	 9999 PVID Egress Untagged
> 
> dummy0	 9999 PVID Egress Untagged
> ---
>   net/bridge/br_vlan.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Good catch, but this is not the right fix. Default pvid of 0 disables the
default pvid, but a wrong pvid should result in -EINVAL return instead of
0. Take a look how the parent default pvid function handles this case
(br_vlan_set_default_pvid).

Also please add a sign off and a proper Fixes tag.
For more information about submitting patches you can check
Documentation/SubmittingPatches

Thanks,
  Nik