Message-ID: <20150616104841.GA4163@salvia>
Date:	Tue, 16 Jun 2015 12:48:41 +0200
From:	Pablo Neira Ayuso <pablo@...filter.org>
To:	Roman Khimov <khimov@...ell.ru>
Cc:	davem@...emloft.net, kaber@...sh.net, kadlec@...ckhole.kfki.hu,
	netfilter-devel@...r.kernel.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, Phil Oester <kernel@...uxace.com>,
	Thomas Graf <tgraf@...g.ch>
Subject: Re: [PATCH] net: fix search limit handling in skb_find_text()

On Mon, Jun 15, 2015 at 10:37:31PM +0300, Roman Khimov wrote:
> В письме от 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.

Thanks, wait for your feedback on this.

> > > 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'.

Then, please fix the Amanda helper.

Look, Amanda is an in-tree client of this textsearch infrastructure,
so it's not exposed to userspace, we can fix it.

But if we change the existing behaviour, users may be relying on it
and we'll get things broken for them. Someone else will come later one
with another patch to say: "hey, --to used to be inclusive but this is
not the case anymore and it's breaking my setup".

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

Please, elaborate.

> > 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.

We have a contract with users that we can't break, so far the corner
case that doesn't work if --from X --to Y where X == Y. If that
didn't work since the beginning, then we're 100% sure nobody has been
using it, so let's fix it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists