Re: [PATCH] net: fix search limit handling in skb_find_text()

[Roman Khimov]

Ð ÐÐÑÑÐÐ ÐÑ 15 ÐÑÐÑ 2015 19:06:39 ÐÐÐÑÐÐÐÐÑÐÐÑ Pablo Neira Ayuso ÐÐÐÐÑÐÐ:
> On Mon, Jun 15, 2015 at 12:11:58PM +0300, Roman I Khimov wrote:
> > Suppose that we're trying to use an xt_string netfilter module to match a
> > string in a specially crafted packet that has "a nice string" starting at
> > offset 28.
> > 
> > It could be done in iptables like this:
> > 
> > -A some_chain -m string --string "a nice string" --algo bm --from 28 --to
> > 38 -j DROP
> > 
> > And it would work as expected. Now changing that to
> > 
> > -A some_chain -m string --string "a nice string" --algo bm --from 29 --to
> > 38 -j DROP
> > 
> > breaks the match, as expected. But, if we try to make
> > 
> > -A some_chain -m string --string "a nice string" --algo bm --from 20 --to
> > 28 -j DROP
> > 
> > then it suddenly works again! So the 'to' parameter seems to be inclusive,
> > not working as an offset after which no search should be done. OK, now if
> > we try:
> > 
> > -A some_chain -m string --string "a nice string" --algo bm --from 28 --to
> > 28 -j DROP
> Can you reproduce the same behaviour with the km algo?

Will try tomorrow MSK time.

> > The first behaviour (matching at 'to' offset) comes from skb_find_text()
> > comparison. The second one (not matching if 'from' and 'to' are equal)
> > comes from skb_seq_read() check for (abs_offset >= st->upper_offset).
> > 
> > I think that the way skb_find_text() handles 'to' is wrong and should be
> > fixed so that we always have predictable behaviour -- only match before
> > 'to' offset.
> > 
> > There are currently only five usages of skb_find_text() in the kernel and
> > it looks to me that none of them expect to match something at the 'to'
> > offset, so probably this change is safe.
> 
> So both 'from' and 'to' are inclusive which seems consistent to me. If
> you make 'to' non-inclusive, then that will change the third example
> above, right? 

Yep.

> That will break existing setups for people that are
> relying on this behaviour. This has been exposed in this way for long
> time, so we should avoid that breakage.

Yes, that could be an issue, but there are other skb_find_text() usages and to 
me they suggest that the intended behaviour is to stop search at 'to' offset. 
In nf_conntrack_amanda.c, for example:

        start = skb_find_text(skb, dataoff, skb->len,
                              search[SEARCH_CONNECT].ts);
...
        stop = skb_find_text(skb, start, skb->len,
                             search[SEARCH_NEWLINE].ts);
...
        stop += start;
...
                off = skb_find_text(skb, start, stop, search[i].ts);

First of all, nothing can ever match at skb->len, and second, it looks like 
the third usage is also expecting to search from offset 'start' to offset 
'stop', not to 'stop + 1'.

em_text_match() in net/sched/em_text.c is also suspicious.

> I would suggest you fix the --from X --to Y where X == Y which is not
> doing what people would expect.

That would certainly make things consistent, but I'm not sure we want it to be 
consistent this way.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/