[v2,1/3] net: Allow to and from offsets to be equal in skb_find_text

Message ID 20180321024216.546-1-bernie.harris@alliedtelesis.co.nz
State Under Review
Delegated to: Pablo Neira
Headers show
Series
  • [v2,1/3] net: Allow to and from offsets to be equal in skb_find_text
Related show

Commit Message

Bernie Harris March 21, 2018, 2:42 a.m.
The xt_string module uses skb_find_text to match a pattern
against packet data. The current behaviour is that the offsets
are used as the range in which a match can start, with the 'to'
offset being included in that range. This means that to do an
exact match for a string at a specific offset, the 'to' and
'from' offsets need to be equal. However, skb_seq_read does not
allow any data to be read if the offsets are equal.

This patch fixes this behaviour by adding the pattern length to
the 'to' offset when calling skb_prepare_seq_read. This should
not change the behaviour of any existing callers of skb_find_text
since the maximum number of bytes read does not change. This
makes it possible for the xt_string module to do an exact match
for a string at a specific offset.

Signed-off-by: Bernie Harris <bernie.harris@alliedtelesis.co.nz>
---
 net/core/skbuff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Pablo Neira Ayuso March 30, 2018, 9:50 a.m. | #1
On Wed, Mar 21, 2018 at 03:42:14PM +1300, Bernie Harris wrote:
> The xt_string module uses skb_find_text to match a pattern
> against packet data. The current behaviour is that the offsets
> are used as the range in which a match can start, with the 'to'
> offset being included in that range. This means that to do an
> exact match for a string at a specific offset, the 'to' and
> 'from' offsets need to be equal. However, skb_seq_read does not
> allow any data to be read if the offsets are equal.
> 
> This patch fixes this behaviour by adding the pattern length to
> the 'to' offset when calling skb_prepare_seq_read. This should
> not change the behaviour of any existing callers of skb_find_text
> since the maximum number of bytes read does not change. This
> makes it possible for the xt_string module to do an exact match
> for a string at a specific offset.
> 
> Signed-off-by: Bernie Harris <bernie.harris@alliedtelesis.co.nz>
> ---
>  net/core/skbuff.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 0bb0d8877954..3026158a9993 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3353,7 +3353,8 @@ unsigned int skb_find_text(struct sk_buff *skb, unsigned int from,
>  	config->get_next_block = skb_ts_get_next_block;
>  	config->finish = skb_ts_finish;
>  
> -	skb_prepare_seq_read(skb, from, to, TS_SKB_CB(&state));
> +	skb_prepare_seq_read(skb, from, to + textsearch_get_pattern_len(config),
> +			     TS_SKB_CB(&state));

I think this may change semantics a bit.

I mean, if you specify [ from , to ] range where from != to, then this
is now going to do [ from, to + pattern] which may be a large range.

I may be overlooking anything, but shouldn't we fix this from xt_string?
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bernie Harris April 3, 2018, 5:24 a.m. | #2
On Fri, Mar 30, 2018 at 11:50:33AM +0200, Pablo Neira Ayuso wrote:
> 
> On Wed, Mar 21, 2018 at 03:42:14PM +1300, Bernie Harris wrote:
> > The xt_string module uses skb_find_text to match a pattern
> > against packet data. The current behaviour is that the offsets
> > are used as the range in which a match can start, with the 'to'
> > offset being included in that range. This means that to do an
> > exact match for a string at a specific offset, the 'to' and
> > 'from' offsets need to be equal. However, skb_seq_read does not
> > allow any data to be read if the offsets are equal.
> > 
> > This patch fixes this behaviour by adding the pattern length to
> > the 'to' offset when calling skb_prepare_seq_read. This should
> > not change the behaviour of any existing callers of skb_find_text
> > since the maximum number of bytes read does not change. This
> > makes it possible for the xt_string module to do an exact match
> > for a string at a specific offset.
> > 
> > Signed-off-by: Bernie Harris <bernie.harris@alliedtelesis.co.nz>
> > ---
> >  net/core/skbuff.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 0bb0d8877954..3026158a9993 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -3353,7 +3353,8 @@ unsigned int skb_find_text(struct sk_buff *skb, unsigned int from,
> >        config->get_next_block = skb_ts_get_next_block;
> >        config->finish = skb_ts_finish;
> >  
> > -     skb_prepare_seq_read(skb, from, to, TS_SKB_CB(&state));
> > +     skb_prepare_seq_read(skb, from, to + textsearch_get_pattern_len(config),
> > +                          TS_SKB_CB(&state));
> 
> I think this may change semantics a bit.
> 
> I mean, if you specify [ from , to ] range where from != to, then this
> is now going to do [ from, to + pattern] which may be a large range.
> 
> I may be overlooking anything, but shouldn't we fix this from xt_string?

Hi Pablo, thanks for the reply.

As far as I can tell what happens is:

The "upper_offset" is only used in skb_seq_read() to stop the search if we try to read a block
that starts on or after that offset. It seems that this will only happen if the pattern exceeds the
length of an entire block so that when we try to get the next block, the starting offset of the new
block exceeds "upper_offset". The other case where this happens is if the "upper_offset"
is equal to "lower_offset", which will cause the match to stop immediately.

So since skb_find_text() prevents doing an exact match on a string at a specific offset, the
only way that I can think of to fix this within xt_string would be to add some number to the
"to" offset so that the match doesn't end immediately, and then reject the match if it did not
occur within [from, to]. My patch just does that in skb_find_text() instead since it already
rejects matches that occur outside [from, to].

With my patch, the number of bytes processed by the text search algorithm will not actually
change, since the text search algorithm only uses the number of bytes indicated by the
pattern length.

I think that instead of adding the pattern length to the "to" offset, I could alternatively just add 1
and still get the behaviour that I want (although I haven't tested that).--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" 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/skbuff.c b/net/core/skbuff.c
index 0bb0d8877954..3026158a9993 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3353,7 +3353,8 @@  unsigned int skb_find_text(struct sk_buff *skb, unsigned int from,
 	config->get_next_block = skb_ts_get_next_block;
 	config->finish = skb_ts_finish;
 
-	skb_prepare_seq_read(skb, from, to, TS_SKB_CB(&state));
+	skb_prepare_seq_read(skb, from, to + textsearch_get_pattern_len(config),
+			     TS_SKB_CB(&state));
 
 	ret = textsearch_find(config, &state);
 	return (ret <= to - from ? ret : UINT_MAX);