| Message ID | 20180321024216.546-1-bernie.harris@alliedtelesis.co.nz |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [v2,1/3] net: Allow to and from offsets to be equal in skb_find_text | expand |
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
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
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);
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(-)