[PATCH] net: fix checking boundary of valid vlan id

[PATCH] net: fix checking boundary of valid vlan id

[akong]

From: Amos Kong <akong@redhat.com>

4096 is not a valid vlan id.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 net/bridge/netfilter/ebt_vlan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bridge/netfilter/ebt_vlan.c b/net/bridge/netfilter/ebt_vlan.c
index eae67bf..b279ec0 100644
--- a/net/bridge/netfilter/ebt_vlan.c
+++ b/net/bridge/netfilter/ebt_vlan.c
@@ -121,8 +121,8 @@ static int ebt_vlan_mt_check(const struct xt_mtchk_param *par)
 	 * if_vlan.h: VLAN_N_VID 4096. */
 	if (GET_BITMASK(EBT_VLAN_ID)) {
 		if (!!info->id) { /* if id!=0 => check vid range */
-			if (info->id > VLAN_N_VID) {
-				pr_debug("id %d is out of range (1-4096)\n",
+			if (info->id >= VLAN_N_VID) {
+				pr_debug("id %d is out of range (1-4095)\n",
 					 info->id);
 				return -EINVAL;
 			}
-- 
1.7.11.7

Re: [PATCH] net: fix checking boundary of valid vlan id

[Amos Kong]

On Sun, Dec 30, 2012 at 02:28:51PM +0800, akong@redhat.com wrote:
> From: Amos Kong <akong@redhat.com>
> 
> 4096 is not a valid vlan id.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  net/bridge/netfilter/ebt_vlan.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bridge/netfilter/ebt_vlan.c b/net/bridge/netfilter/ebt_vlan.c
> index eae67bf..b279ec0 100644
> --- a/net/bridge/netfilter/ebt_vlan.c
> +++ b/net/bridge/netfilter/ebt_vlan.c
> @@ -121,8 +121,8 @@ static int ebt_vlan_mt_check(const struct xt_mtchk_param *par)
>  	 * if_vlan.h: VLAN_N_VID 4096. */
>  	if (GET_BITMASK(EBT_VLAN_ID)) {
>  		if (!!info->id) { /* if id!=0 => check vid range */
> -			if (info->id > VLAN_N_VID) {
> -				pr_debug("id %d is out of range (1-4096)\n",
> +			if (info->id >= VLAN_N_VID) {
> +				pr_debug("id %d is out of range (1-4095)\n",

Hi David,

4095 is reserved, treat it as invalid here?

   			if (info->id >= VLAN_N_VID - 1) {
   				pr_debug("id %d is out of range (1-4094)\n",


>  					 info->id);
>  				return -EINVAL;
>  			}
> -- 
> 1.7.11.7

Re: [PATCH] net: fix checking boundary of valid vlan id

[David Miller]

Please send netfilter patches to the netfilter developers
at netfilter-devel@vger.kernel.org

Thanks.

Re: [PATCH] net: fix checking boundary of valid vlan id

[Florian Westphal]

akong@redhat.com <akong@redhat.com> wrote:
> From: Amos Kong <akong@redhat.com>
> 
> 4096 is not a valid vlan id.

True.

But why shouldn't users be allowed to check for frames
with reserved value?

Re: [PATCH] net: fix checking boundary of valid vlan id

[Glen Turner]

On 30/12/2012, at 10:55 PM, Florian Westphal wrote:

> akong@redhat.com <akong@redhat.com> wrote:
>> From: Amos Kong <akong@redhat.com>
>> 
>> 4096 is not a valid vlan id.
> 
> True.
> 
> But why shouldn't users be allowed to check for frames
> with reserved value


It may be a valid VLAN ID, or it may not. The meaning of FFF is reserved for vendor use, which doesn't preclude a vendor using it as a (non-interoperable) VLAN identifier. Many vendor's products treat 4096 as they do any other VID.

It's up to Linux if it cares to treat 4096 as a VLAN or as something else.

In any case, Florian's point is good: an ethernet-layer firewall should be able to trigger on any value of VID, and particularly one which should not be seen from some vendor's gear.

-glen

Re: [PATCH] net: fix checking boundary of valid vlan id

[Benny Amorsen]

Glen Turner <gdt@gdt.id.au> writes:

> It may be a valid VLAN ID, or it may not. The meaning of FFF is
> reserved for vendor use, which doesn't preclude a vendor using it as a
> (non-interoperable) VLAN identifier. Many vendor's products treat 4096
> as they do any other VID.

I may be missing something vital, but 4096 is 0x1000 not 0xFFF? 4095 is
reserved and 0 means "treat as if the packet was untagged". 4096 is
impossible, there are only 12 bit and the encoding is AFAIK bog standard
binary.


/Benny

Re: [PATCH] net: fix checking boundary of valid vlan id

[Glen Turner]

On 01/01/2013, at 3:42 AM, Benny Amorsen wrote:

> Glen Turner <gdt@gdt.id.au> writes:
> 
>> It may be a valid VLAN ID, or it may not. The meaning of FFF is
>> reserved for vendor use, which doesn't preclude a vendor using it as a
>> (non-interoperable) VLAN identifier. Many vendor's products treat 4096
>> as they do any other VID.
> 
> I may be missing something vital, but 4096 is 0x1000 not 0xFFF? 4095 is
> reserved and 0 means "treat as if the packet was untagged". 4096 is
> impossible, there are only 12 bit and the encoding is AFAIK bog standard
> binary.

Yep, I've failed at hex math. 0xfff = 4095 is the maximal VID value, and is the one reserved for vendor use.
Which means that the patch checking values 1 to 4095 is correct.

My apologies,
glen

Re: [PATCH] net: fix checking boundary of valid vlan id

[Pablo Neira Ayuso]

On Sun, Dec 30, 2012 at 02:28:51PM +0800, akong@redhat.com wrote:
> From: Amos Kong <akong@redhat.com>
> 
> 4096 is not a valid vlan id.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  net/bridge/netfilter/ebt_vlan.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bridge/netfilter/ebt_vlan.c b/net/bridge/netfilter/ebt_vlan.c
> index eae67bf..b279ec0 100644
> --- a/net/bridge/netfilter/ebt_vlan.c
> +++ b/net/bridge/netfilter/ebt_vlan.c
> @@ -121,8 +121,8 @@ static int ebt_vlan_mt_check(const struct xt_mtchk_param *par)
>  	 * if_vlan.h: VLAN_N_VID 4096. */
>  	if (GET_BITMASK(EBT_VLAN_ID)) {
>  		if (!!info->id) { /* if id!=0 => check vid range */
> -			if (info->id > VLAN_N_VID) {
> -				pr_debug("id %d is out of range (1-4096)\n",
> +			if (info->id >= VLAN_N_VID) {
> +				pr_debug("id %d is out of range (1-4095)\n",

Someone may forge frames including reserved VLAN ids.

People can use ebtables to drop invalid frames, this is a firewalling
utility after all ;-). I'm not taking these, sorry.

Re: [PATCH] net: fix checking boundary of valid vlan id

[Ben Hutchings]

On Thu, 2013-01-03 at 01:39 +0100, Pablo Neira Ayuso wrote:
> On Sun, Dec 30, 2012 at 02:28:51PM +0800, akong@redhat.com wrote:
> > From: Amos Kong <akong@redhat.com>
> > 
> > 4096 is not a valid vlan id.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  net/bridge/netfilter/ebt_vlan.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/bridge/netfilter/ebt_vlan.c b/net/bridge/netfilter/ebt_vlan.c
> > index eae67bf..b279ec0 100644
> > --- a/net/bridge/netfilter/ebt_vlan.c
> > +++ b/net/bridge/netfilter/ebt_vlan.c
> > @@ -121,8 +121,8 @@ static int ebt_vlan_mt_check(const struct xt_mtchk_param *par)
> >  	 * if_vlan.h: VLAN_N_VID 4096. */
> >  	if (GET_BITMASK(EBT_VLAN_ID)) {
> >  		if (!!info->id) { /* if id!=0 => check vid range */
> > -			if (info->id > VLAN_N_VID) {
> > -				pr_debug("id %d is out of range (1-4096)\n",
> > +			if (info->id >= VLAN_N_VID) {
> > +				pr_debug("id %d is out of range (1-4095)\n",
> 
> Someone may forge frames including reserved VLAN ids.

They may find it difficult to fit the value 4096 into a 12-bit field,
though.

Ben.

> People can use ebtables to drop invalid frames, this is a firewalling
> utility after all ;-). I'm not taking these, sorry.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.