Re: [PATCH nf-next] netfilter: x_tables: add context to know if extension runs from nft_compat

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Mon, Mar 02, 2015 at 01:27:05PM +0000, Patrick McHardy wrote:
> On 02.03, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > 2) TCPMSS. Relax the checking when used from nft_compat and make sure
> > >    that we skip !syn packets in case userspace provides a wrong
> > >    configuration.
> > > diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
> > > index e762de5..3b9761f 100644
> > > --- a/net/netfilter/xt_TCPMSS.c
> > > +++ b/net/netfilter/xt_TCPMSS.c
> > > @@ -97,6 +97,9 @@ tcpmss_mangle_packet(struct sk_buff *skb,
> > >  	if (!skb_make_writable(skb, skb->len))
> > >  		return -1;
> > >  
> > > +	if (unlikely(!tcph->syn))
> > > +		return 0;
> > > +
> > >  	len = skb->len - tcphoff;
> > 
> > Applying this to my copy of nf-next would insert this test before
> > tcph is set up.
> 
> I actually don't think the test is necessary at all. Since we
> don't check the protocol with nft, any packet with that bit set
> will pass. It will most likely fail or corrupt the packet, but
> why should we care? It won't crash and with nft its the
> responsibility of userspace to take care of using the extension
> correctly.

According to what I can read from the code, it will corrupt the
packet since we will get a MSS option in non-syn TCP packet.

I thought we could crash, but looking at it again you seem to be
right, I'm going to undo this change and send a v2.