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

[Roman Khimov]

Ð ÐÐÑÑÐÐ ÐÑ 16 ÐÑÐÑ 2015 12:48:41 ÐÐÐÑÐÐÐÐÑÐÐÑ Pablo Neira Ayuso ÐÐÐÐÑÐÐ:
> 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.

Same behaviour with kmp.

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

I do understand your concerns, but fixing it this way would require changing 
skb_seq_read() and basicaly would propagate "'to' offset included" semantics 
(which seems a bit strange for programmers, IMO) further. And initially I 
thought that changing skb_seq_read() would be more intrusive, although looking 
at all this now it looks like the only real user of upper_offset field in 
ts_config struct is skb_find_text(), because other invocations of 
skb_seq_read() from drivers/scsi/libiscsi_tcp.c and net/batman-adv/main.c use 
skb->len as an upper limit.

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

The way it constructs 'to' offset, I think it doesn't expect something to 
match at 'to'. Although I might be wrong here.

Attachment: smime.p7s
Description: S/MIME cryptographic signature