From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753428AbbFOUQO (ORCPT ); Mon, 15 Jun 2015 16:16:14 -0400 Received: from roman.khimov.ru ([82.146.54.17]:59668 "EHLO roman.khimov.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751909AbbFOUQH convert rfc822-to-8bit (ORCPT ); Mon, 15 Jun 2015 16:16:07 -0400 X-Greylist: delayed 1781 seconds by postgrey-1.27 at vger.kernel.org; Mon, 15 Jun 2015 16:16:06 EDT From: Roman Khimov To: Pablo Neira Ayuso Reply-To: khimov@altell.ru Cc: davem@davemloft.net, kaber@trash.net, kadlec@blackhole.kfki.hu, netfilter-devel@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Phil Oester , Thomas Graf Subject: Re: [PATCH] net: fix search limit handling in skb_find_text() Date: Mon, 15 Jun 2015 22:37:31 +0300 Message-ID: <1847765.0Hbie9lSro@mate.hex> Organization: Altell Ltd. User-Agent: KMail/4.11.5 (Linux/3.11.10-21-desktop; KDE/4.11.5; x86_64; ; ) In-Reply-To: <20150615170639.GA21117@salvia> References: <1434359518-16897-1-git-send-email-khimov@altell.ru> <20150615170639.GA21117@salvia> MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org В письме от 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.