Re: [PATCH nf-next] netfilter: add support for matching IPv4 options

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

On Fri, May 31, 2019 at 03:35:58PM -0400, Stephen Suryaputra wrote:
> On Fri, May 31, 2019 at 07:11:01PM +0200, Pablo Neira Ayuso wrote:
> > > +/* find the offset to specified option or the header beyond the options
> > > + * if target < 0.
> > > + *
> > > + * Note that *offset is used as input/output parameter, and if it is not zero,
> > > + * then it must be a valid offset to an inner IPv4 header. This can be used
> > > + * to explore inner IPv4 header, eg. ICMP error messages.
> > 
> > In other extension headers (IPv6 and TCP options) this offset is used
> > to match for a field inside the extension / option.
> > 
> > So this semantics you describe here are slightly different, right?
> 
> It is the same as the IPv6 one. The offset returned is the offset to the
> specific option (target) or the byte beyond the options if the target
> isn't specified (< 0).

Thanks for explaining. So you are using ipv6_find_hdr() as reference,
but not sure this offset parameter is useful for this patchset since
this is always set to zero, do you have plans to use this in a follow
up patchset?

[...]
> > > +			if (tb[NFTA_EXTHDR_DREG])
> > > +				return &nft_exthdr_ipv4_ops;
> > > +		}
> > > +		break;
> > 
> > Then, from the _eval() path:
> > 
> > You have to replace iph->version == 4 check abive, you could use
> > skb->protocol instead, and check for htons(ETH_P_IP) packets.
> 
> A bit lost. Did you mean this?
> 
> static int ipv4_find_option(struct net *net, struct sk_buff *skb,
> »       »       »           unsigned int *offset, int target,
> »       »       »           unsigned short *fragoff, int *flags)
> {
> 	...
> »       iph = skb_header_pointer(skb, *offset, sizeof(_iph), &_iph);
> »       if (!iph || skb->protocol != htons(ETH_P_IP))
> »       »       return -EBADMSG;

I mean, you make this check upfront from the _eval() path, ie.

static void nft_exthdr_ipv4_eval(const struct nft_expr *expr,
                                 ...
{
        ...

        if (skb->protocol != htons(ETH_P_IP))
                goto err;

Thanks.