Patchwork [tcpdump-workers] vlan tagged packets and libpcap breakage

login
register
mail settings
Submitter Ani Sinha
Date Dec. 11, 2012, 10:36 p.m.
Message ID <alpine.OSX.2.00.1212111432430.78903@animac.local>
Download mbox | patch
Permalink /patch/205326/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Ani Sinha - Dec. 11, 2012, 10:36 p.m.
>
> It is possible to test for the presence of support of the new vlan bpf
> extensions by attempting to load a filter that uses them.  As only valid
> filters can be loaded, old kernels that do not support filtering of vlan
> tags will fail to load the a test filter with uses them.

Unfortunately I do not see this. The sk_chk_filter() does not have a
default in the case statement and the check will not detect an unknown
instruction. It will fail when the filter is run and as far as I can see,
the packet will be dropped. Something like this might help?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Dec. 11, 2012, 11:04 p.m.
On Tue, 2012-12-11 at 14:36 -0800, Ani Sinha wrote:
> >
> > It is possible to test for the presence of support of the new vlan bpf
> > extensions by attempting to load a filter that uses them.  As only valid
> > filters can be loaded, old kernels that do not support filtering of vlan
> > tags will fail to load the a test filter with uses them.
> 
> Unfortunately I do not see this. The sk_chk_filter() does not have a
> default in the case statement and the check will not detect an unknown
> instruction. It will fail when the filter is run and as far as I can see,
> the packet will be dropped. Something like this might help?
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index c23543c..96338aa 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -548,6 +548,8 @@ int sk_chk_filter(struct sock_filter *filter, unsigned int flen)
>  			return -EINVAL;
>  		/* Some instructions need special checks */
>  		switch (code) {
> +		/* for unknown instruction, return EINVAL */
> +		default : return -EINVAL;
>  		case BPF_S_ALU_DIV_K:
>  			/* check for division by zero */
>  			if (ftest->k == 0)

This patch is wrong.

Check lines 546, 547, 548 where we do the check for unknown instructions

code = codes[code];
if (!code)
	return -EINVAL;

If you want to test ANCILLARY possible values, its already too late, as
old kernels wont use any patch anyway.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger - Dec. 11, 2012, 11:12 p.m.
On Tue, 11 Dec 2012 14:36:33 -0800 (PST)
Ani Sinha <ani@aristanetworks.com> wrote:

> >
> > It is possible to test for the presence of support of the new vlan bpf
> > extensions by attempting to load a filter that uses them.  As only valid
> > filters can be loaded, old kernels that do not support filtering of vlan
> > tags will fail to load the a test filter with uses them.
> 
> Unfortunately I do not see this. The sk_chk_filter() does not have a
> default in the case statement and the check will not detect an unknown
> instruction. It will fail when the filter is run and as far as I can see,
> the packet will be dropped. Something like this might help?
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index c23543c..96338aa 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -548,6 +548,8 @@ int sk_chk_filter(struct sock_filter *filter, unsigned int flen)
>  			return -EINVAL;
>  		/* Some instructions need special checks */
>  		switch (code) {
> +		/* for unknown instruction, return EINVAL */
> +		default : return -EINVAL;
>  		case BPF_S_ALU_DIV_K:
>  			/* check for division by zero */
>  			if (ftest->k == 0)

Did you test this? I think it will blow up for some existing instructions
like BPF_S_ALU_XOR_X or any of the other non-special instructions.

Also it is not formatted correctly for the kernel programming style.

ERROR: space prohibited before that ':' (ctx:WxW)
#86: FILE: net/core/filter.c:552:
+		default : return -EINVAL;
 		        ^


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ani Sinha - Dec. 12, 2012, 12:46 a.m.
On Tue, Dec 11, 2012 at 3:04 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2012-12-11 at 14:36 -0800, Ani Sinha wrote:
>> >
>> > It is possible to test for the presence of support of the new vlan bpf
>> > extensions by attempting to load a filter that uses them.  As only valid
>> > filters can be loaded, old kernels that do not support filtering of vlan
>> > tags will fail to load the a test filter with uses them.
>>
>> Unfortunately I do not see this. The sk_chk_filter() does not have a
>> default in the case statement and the check will not detect an unknown
>> instruction. It will fail when the filter is run and as far as I can see,
>> the packet will be dropped. Something like this might help?
>>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index c23543c..96338aa 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -548,6 +548,8 @@ int sk_chk_filter(struct sock_filter *filter, unsigned int flen)
>>                       return -EINVAL;
>>               /* Some instructions need special checks */
>>               switch (code) {
>> +             /* for unknown instruction, return EINVAL */
>> +             default : return -EINVAL;
>>               case BPF_S_ALU_DIV_K:
>>                       /* check for division by zero */
>>                       if (ftest->k == 0)
>
> This patch is wrong.


yes I generated this patch wrong.

>
> Check lines 546, 547, 548 where we do the check for unknown instructions
>
> code = codes[code];
> if (!code)
>         return -EINVAL;

yepph it's OK here.

>
> If you want to test ANCILLARY possible values, its already too late, as
> old kernels wont use any patch anyway.

yepph, I was looking at possible ancilliary values. Basically this
case statement :

#define ANCILLARY(CODE) case SKF_AD_OFF + SKF_AD_##CODE:        \
                                code = BPF_S_ANC_##CODE;        \
                                break
                        switch (ftest->k) {
                        ANCILLARY(PROTOCOL);
                        ANCILLARY(PKTTYPE);
                        ANCILLARY(IFINDEX);
                        ANCILLARY(NLATTR);
                        ANCILLARY(NLATTR_NEST);
                        ANCILLARY(MARK);
                        ANCILLARY(QUEUE);
                        ANCILLARY(HATYPE);
                        ANCILLARY(RXHASH);
                        ANCILLARY(CPU);
                        ANCILLARY(ALU_XOR_X);
                        ANCILLARY(VLAN_TAG);
                        ANCILLARY(VLAN_TAG_PRESENT);
                        }
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ani Sinha - Dec. 12, 2012, 12:50 a.m.
On Tue, Dec 11, 2012 at 3:04 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2012-12-11 at 14:36 -0800, Ani Sinha wrote:
>> >
>> > It is possible to test for the presence of support of the new vlan bpf

> If you want to test ANCILLARY possible values, its already too late, as
> old kernels wont use any patch anyway.
>

So basically this means that if we generate a filter with these
special negative offset values and expect that the kernel will
complain if it does not recognize the newer values then we would be
wrong. And you are right. Old kernels never knew about them and the
code wasn't written in a way to return EINVAL if it didn't recognize a
special negative anciliary offset value.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/core/filter.c b/net/core/filter.c
index c23543c..96338aa 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -548,6 +548,8 @@  int sk_chk_filter(struct sock_filter *filter, unsigned int flen)
 			return -EINVAL;
 		/* Some instructions need special checks */
 		switch (code) {
+		/* for unknown instruction, return EINVAL */
+		default : return -EINVAL;
 		case BPF_S_ALU_DIV_K:
 			/* check for division by zero */
 			if (ftest->k == 0)