[PATCH net+iproute2 0/2] nbyte, cmp and text filter fixups

[PATCH net+iproute2 0/2] nbyte, cmp and text filter fixups

[Wolfgang Bumiller]

The iproute2 part allows the the actual use of the already existing
quoted string parsing.

The kernel side fixes an oob read in em_nbyte and allows 'layer 0' in
cmp and nbyte (and em_text whose existence surprised me given that I did
not see it exposed via iproute2) to actually match layer 0 rather than
being the same as specifying layer 1.

I seem to have stumbled upon a layer of dust (says git-blame).
Trying to match mac addresses I felt that the examples found online
using the 'u32' filter were rather inconvenient, particularly given
that there's the 'nbyte' filter around that could just memcmp the
entire a byte sequence at once.

Wolfgang Bumiller (1; 2):
  tc/lexer: let quotes actually start strings

 tc/emp_ematch.l | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

  net: sched: em_nbyte: don't add the data offset twice
  net_sched: fix TCF_LAYER_LINK case in tcf_get_base_ptr

 include/net/pkt_cls.h | 2 +-
 net/sched/em_nbyte.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.11.0

[PATCH iproute2] tc/lexer: let quotes actually start strings

[Wolfgang Bumiller]

The lexer will go with the longest match, so previously
the starting double quotes of a string would be swallowed by
the [^ \t\r\n()]+ pattern leaving the user no way to
actually use strings with escape sequences.
Fix this by not allowing this case to start with double
quotes.

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 tc/emp_ematch.l | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tc/emp_ematch.l b/tc/emp_ematch.l
index dc106759..d7a99304 100644
--- a/tc/emp_ematch.l
+++ b/tc/emp_ematch.l
@@ -137,7 +137,7 @@
 ")"					{
 						return yylval.i = *yytext;
 					}
-[^ \t\r\n()]+				{
+[^" \t\r\n()][^ \t\r\n()]*		{
 						yylval.b = bstr_alloc(yytext);
 						if (yylval.b == NULL)
 							return ERROR;
-- 
2.11.0

[PATCH net 1/2] net: sched: em_nbyte: don't add the data offset twice

[Wolfgang Bumiller]

'ptr' is shifted by the offset and then validated,
the memcmp should not add it a second time.

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 net/sched/em_nbyte.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/em_nbyte.c b/net/sched/em_nbyte.c
index df3110d69585..07c10bac06a0 100644
--- a/net/sched/em_nbyte.c
+++ b/net/sched/em_nbyte.c
@@ -47,15 +47,15 @@ static int em_nbyte_match(struct sk_buff *skb, struct tcf_ematch *em,
 	unsigned char *ptr = tcf_get_base_ptr(skb, nbyte->hdr.layer);
 
 	ptr += nbyte->hdr.off;
 
 	if (!tcf_valid_offset(skb, ptr, nbyte->hdr.len))
 		return 0;
 
-	return !memcmp(ptr + nbyte->hdr.off, nbyte->pattern, nbyte->hdr.len);
+	return !memcmp(ptr, nbyte->pattern, nbyte->hdr.len);
 }
 
 static struct tcf_ematch_ops em_nbyte_ops = {
 	.kind	  = TCF_EM_NBYTE,
 	.change	  = em_nbyte_change,
 	.match	  = em_nbyte_match,
 	.owner	  = THIS_MODULE,
-- 
2.11.0

[PATCH net 2/2] net_sched: fix TCF_LAYER_LINK case in tcf_get_base_ptr

[Wolfgang Bumiller]

TCF_LAYER_LINK and TCF_LAYER_NETWORK returned the same pointer as
skb->data points to the network header.
Use skb_mac_header instead.

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
Alternatively this could return skb->head directly, but
'sk_buff->mac_header' is documented as 'Link layer header' and this
seemed more clear. Since on the first read I thought "it looks fine"
while in fact skb->head comes before skb->data, so this seems less
confusing.

 include/net/pkt_cls.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 8e08b6da72f3..753ac9361154 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -522,7 +522,7 @@ static inline unsigned char * tcf_get_base_ptr(struct sk_buff *skb, int layer)
 {
 	switch (layer) {
 		case TCF_LAYER_LINK:
-			return skb->data;
+			return skb_mac_header(skb);
 		case TCF_LAYER_NETWORK:
 			return skb_network_header(skb);
 		case TCF_LAYER_TRANSPORT:
-- 
2.11.0

Re: [PATCH net+iproute2 0/2] nbyte, cmp and text filter fixups

[Jiri Pirko]

Thu, Jan 18, 2018 at 11:32:33AM CET, w.bumiller@proxmox.com wrote:
>The iproute2 part allows the the actual use of the already existing
>quoted string parsing.

Makes no sense to send iproute2 patch which is not related with the
kernel ones in the same set. Please send it separatelly.

>
>The kernel side fixes an oob read in em_nbyte and allows 'layer 0' in
>cmp and nbyte (and em_text whose existence surprised me given that I did
>not see it exposed via iproute2) to actually match layer 0 rather than
>being the same as specifying layer 1.
>
>I seem to have stumbled upon a layer of dust (says git-blame).
>Trying to match mac addresses I felt that the examples found online
>using the 'u32' filter were rather inconvenient, particularly given
>that there's the 'nbyte' filter around that could just memcmp the
>entire a byte sequence at once.
>
>Wolfgang Bumiller (1; 2):
>  tc/lexer: let quotes actually start strings
>
> tc/emp_ematch.l | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>  net: sched: em_nbyte: don't add the data offset twice
>  net_sched: fix TCF_LAYER_LINK case in tcf_get_base_ptr

"net: sched:" or "net_sched:"? - please, try to be consistent

Re: [PATCH net 1/2] net: sched: em_nbyte: don't add the data offset twice

[David Miller]

From: Wolfgang Bumiller <w.bumiller@proxmox.com>
Date: Thu, 18 Jan 2018 11:32:35 +0100

> 'ptr' is shifted by the offset and then validated,
> the memcmp should not add it a second time.
> 
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>

Applied.

Re: [PATCH net 2/2] net_sched: fix TCF_LAYER_LINK case in tcf_get_base_ptr

[David Miller]

From: Wolfgang Bumiller <w.bumiller@proxmox.com>
Date: Thu, 18 Jan 2018 11:32:36 +0100

> TCF_LAYER_LINK and TCF_LAYER_NETWORK returned the same pointer as
> skb->data points to the network header.
> Use skb_mac_header instead.
> 
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>

Applied with Subject changed to be more consistent "net: sched:"

Re: [PATCH iproute2] tc/lexer: let quotes actually start strings

[Stephen Hemminger]

On Mon, 22 Jan 2018 11:53:46 +0100
Wolfgang Bumiller <w.bumiller@proxmox.com> wrote:

> The lexer will go with the longest match, so previously
> the starting double quotes of a string would be swallowed by
> the [^ \t\r\n()]+ pattern leaving the user no way to
> actually use strings with escape sequences.
> Fix this by not allowing this case to start with double
> quotes.
> 
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>

Looks good, applied.
Don't think ematch is that widely used; or someone would have noticed.

Re: [PATCH iproute2] tc/lexer: let quotes actually start strings

[Stephen Hemminger]

On Mon, 22 Jan 2018 11:53:46 +0100
Wolfgang Bumiller <w.bumiller@proxmox.com> wrote:

> The lexer will go with the longest match, so previously
> the starting double quotes of a string would be swallowed by
> the [^ \t\r\n()]+ pattern leaving the user no way to
> actually use strings with escape sequences.
> Fix this by not allowing this case to start with double
> quotes.
> 
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---

This is the same patch you posted earlier. It looked fine, just that
it needs some time for people on mailing list to review.

[PATCH iproute2] tc/lexer: let quotes actually start strings

[Wolfgang Bumiller]

The lexer will go with the longest match, so previously
the starting double quotes of a string would be swallowed by
the [^ \t\r\n()]+ pattern leaving the user no way to
actually use strings with escape sequences.
Fix this by not allowing this case to start with double
quotes.

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
(Resent as separate thread)
 tc/emp_ematch.l | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tc/emp_ematch.l b/tc/emp_ematch.l
index dc106759..d7a99304 100644
--- a/tc/emp_ematch.l
+++ b/tc/emp_ematch.l
@@ -137,7 +137,7 @@
 ")"					{
 						return yylval.i = *yytext;
 					}
-[^ \t\r\n()]+				{
+[^" \t\r\n()][^ \t\r\n()]*		{
 						yylval.b = bstr_alloc(yytext);
 						if (yylval.b == NULL)
 							return ERROR;
-- 
2.11.0