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

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

[Stephen Suryaputra]

This is the kernel change for the overall changes with this description:
Add capability to have rules matching IPv4 options. This is developed
mainly to support dropping of IP packets with loose and/or strict source
route route options. Nevertheless, the implementation include others and
ability to get specific fields in the option.

Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
---
 include/net/inet_sock.h                  |   2 +-
 include/uapi/linux/netfilter/nf_tables.h |   2 +
 net/ipv4/ip_options.c                    |   2 +
 net/netfilter/nft_exthdr.c               | 136 +++++++++++++++++++++++
 4 files changed, 141 insertions(+), 1 deletion(-)

diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
index e8eef85006aa..8db4f8639a33 100644
--- a/include/net/inet_sock.h
+++ b/include/net/inet_sock.h
@@ -55,7 +55,7 @@ struct ip_options {
 			ts_needaddr:1;
 	unsigned char	router_alert;
 	unsigned char	cipso;
-	unsigned char	__pad2;
+	unsigned char	end;
 	unsigned char	__data[0];
 };
 
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 061bb3eb20c3..81d31f4e4aa3 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -730,10 +730,12 @@ enum nft_exthdr_flags {
  *
  * @NFT_EXTHDR_OP_IPV6: match against ipv6 extension headers
  * @NFT_EXTHDR_OP_TCP: match against tcp options
+ * @NFT_EXTHDR_OP_IPV4: match against ipv4 options
  */
 enum nft_exthdr_op {
 	NFT_EXTHDR_OP_IPV6,
 	NFT_EXTHDR_OP_TCPOPT,
+	NFT_EXTHDR_OP_IPV4,
 	__NFT_EXTHDR_OP_MAX
 };
 #define NFT_EXTHDR_OP_MAX	(__NFT_EXTHDR_OP_MAX - 1)
diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
index 3db31bb9df50..fc0e694aa97c 100644
--- a/net/ipv4/ip_options.c
+++ b/net/ipv4/ip_options.c
@@ -272,6 +272,7 @@ int __ip_options_compile(struct net *net,
 	for (l = opt->optlen; l > 0; ) {
 		switch (*optptr) {
 		case IPOPT_END:
+			opt->end = optptr - iph;
 			for (optptr++, l--; l > 0; optptr++, l--) {
 				if (*optptr != IPOPT_END) {
 					*optptr = IPOPT_END;
@@ -473,6 +474,7 @@ int __ip_options_compile(struct net *net,
 		*info = htonl((pp_ptr-iph)<<24);
 	return -EINVAL;
 }
+EXPORT_SYMBOL(__ip_options_compile);
 
 int ip_options_compile(struct net *net,
 		       struct ip_options *opt, struct sk_buff *skb)
diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
index a940c9fd9045..c4d47d274bbe 100644
--- a/net/netfilter/nft_exthdr.c
+++ b/net/netfilter/nft_exthdr.c
@@ -62,6 +62,128 @@ static void nft_exthdr_ipv6_eval(const struct nft_expr *expr,
 	regs->verdict.code = NFT_BREAK;
 }
 
+/* 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.
+ *
+ * If target header is found, its offset is set in *offset and return option
+ * number. Otherwise, return negative error.
+ *
+ * If the first fragment doesn't contain the End of Options it is considered
+ * invalid.
+ */
+static int ipv4_find_option(struct net *net, struct sk_buff *skb, unsigned int *offset,
+		     int target, unsigned short *fragoff, int *flags)
+{
+	unsigned char optbuf[sizeof(struct ip_options) + 41];
+	struct ip_options *opt = (struct ip_options *)optbuf;
+	struct iphdr *iph, _iph;
+	unsigned int start;
+	__be32 info;
+	int optlen;
+	bool found = false;
+
+	if (fragoff)
+		*fragoff = 0;
+
+	if (!offset)
+		return -EINVAL;
+	if (!*offset)
+		*offset = skb_network_offset(skb);
+
+	iph = skb_header_pointer(skb, *offset, sizeof(_iph), &_iph);
+	if (!iph || iph->version != 4)
+		return -EBADMSG;
+	start = *offset + sizeof(struct iphdr);
+
+	optlen = iph->ihl * 4 - (int)sizeof(struct iphdr);
+	if (optlen <= 0)
+		return -ENOENT;
+
+	memset(opt, 0, sizeof(struct ip_options));
+	/* Copy the options since __ip_options_compile() modifies
+	 * the options. Get one byte beyond the option for target < 0
+	 */
+	if (skb_copy_bits(skb, start, opt->__data, optlen + 1))
+		return -EBADMSG;
+	opt->optlen = optlen;
+
+	if (__ip_options_compile(net, opt, NULL, &info))
+		return -EBADMSG;
+
+	switch (target) {
+	case IPOPT_SSRR:
+	case IPOPT_LSRR:
+		if (opt->srr) {
+			found = target == IPOPT_SSRR ? opt->is_strictroute :
+						       !opt->is_strictroute;
+			if (found)
+				*offset = opt->srr + start;
+		}
+		break;
+	case IPOPT_RR:
+		if (opt->rr) {
+			*offset = opt->rr + start;
+			found = true;
+		}
+		break;
+	case IPOPT_RA:
+		if (opt->router_alert) {
+			*offset = opt->router_alert + start;
+			found = true;
+		}
+		break;
+	default:
+		/* Either not supported or not a specific search, treated as found */
+		found = true;
+		if (target < 0) {
+			if (opt->end) {
+				*offset = opt->end + start;
+				target = IPOPT_END;
+			} else {
+				/* Point to beyond the options. */
+				*offset = optlen + start;
+				target = opt->__data[optlen];
+			}
+		} else {
+			target = -EOPNOTSUPP;
+		}
+	}
+	if (!found)
+		target = -ENOENT;
+	return target;
+}
+
+static void nft_exthdr_ipv4_eval(const struct nft_expr *expr,
+				 struct nft_regs *regs,
+				 const struct nft_pktinfo *pkt)
+{
+	struct nft_exthdr *priv = nft_expr_priv(expr);
+	u32 *dest = &regs->data[priv->dreg];
+	struct sk_buff *skb = pkt->skb;
+	unsigned int offset = 0;
+	int err;
+
+	err = ipv4_find_option(nft_net(pkt), skb, &offset, priv->type, NULL, NULL);
+	if (priv->flags & NFT_EXTHDR_F_PRESENT) {
+		*dest = (err >= 0);
+		return;
+	} else if (err < 0) {
+		goto err;
+	}
+	offset += priv->offset;
+
+	dest[priv->len / NFT_REG32_SIZE] = 0;
+	if (skb_copy_bits(pkt->skb, offset, dest, priv->len) < 0)
+		goto err;
+	return;
+err:
+	regs->verdict.code = NFT_BREAK;
+}
+
 static void *
 nft_tcp_header_pointer(const struct nft_pktinfo *pkt,
 		       unsigned int len, void *buffer, unsigned int *tcphdr_len)
@@ -360,6 +482,14 @@ static const struct nft_expr_ops nft_exthdr_ipv6_ops = {
 	.dump		= nft_exthdr_dump,
 };
 
+static const struct nft_expr_ops nft_exthdr_ipv4_ops = {
+	.type		= &nft_exthdr_type,
+	.size		= NFT_EXPR_SIZE(sizeof(struct nft_exthdr)),
+	.eval		= nft_exthdr_ipv4_eval,
+	.init		= nft_exthdr_init,
+	.dump		= nft_exthdr_dump,
+};
+
 static const struct nft_expr_ops nft_exthdr_tcp_ops = {
 	.type		= &nft_exthdr_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_exthdr)),
@@ -400,6 +530,12 @@ nft_exthdr_select_ops(const struct nft_ctx *ctx,
 		if (tb[NFTA_EXTHDR_DREG])
 			return &nft_exthdr_ipv6_ops;
 		break;
+	case NFT_EXTHDR_OP_IPV4:
+		if (ctx->family == NFPROTO_IPV4) {
+			if (tb[NFTA_EXTHDR_DREG])
+				return &nft_exthdr_ipv4_ops;
+		}
+		break;
 	}
 
 	return ERR_PTR(-EOPNOTSUPP);
-- 
2.17.1

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

[Pablo Neira Ayuso]

Hi Stephen,

On Thu, May 23, 2019 at 05:38:01AM -0400, Stephen Suryaputra wrote:
> This is the kernel change for the overall changes with this description:
> Add capability to have rules matching IPv4 options. This is developed
> mainly to support dropping of IP packets with loose and/or strict source
> route route options. Nevertheless, the implementation include others and
> ability to get specific fields in the option.

Thanks for submitting your patch.

> Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
> ---
>  include/net/inet_sock.h                  |   2 +-
[...]
>  net/ipv4/ip_options.c                    |   2 +

Please, place the update of these two files (which are not netfilter
specific) in a separated (initial) patch, we'll need an Acked-by: tag
from the general networking maintainer to get this in. It will make
things easier if this comes in a separated (initial) patch.

More comments below.

>  net/netfilter/nft_exthdr.c               | 136 +++++++++++++++++++++++
>  4 files changed, 141 insertions(+), 1 deletion(-)
> 
[...]
> diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
> index a940c9fd9045..c4d47d274bbe 100644
> --- a/net/netfilter/nft_exthdr.c
> +++ b/net/netfilter/nft_exthdr.c
> @@ -62,6 +62,128 @@ static void nft_exthdr_ipv6_eval(const struct nft_expr *expr,
>  	regs->verdict.code = NFT_BREAK;
>  }
>  
> +/* 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?

> + * If target header is found, its offset is set in *offset and return option
> + * number. Otherwise, return negative error.
> + *
> + * If the first fragment doesn't contain the End of Options it is considered
> + * invalid.
> + */
> +static int ipv4_find_option(struct net *net, struct sk_buff *skb, unsigned int *offset,
> +		     int target, unsigned short *fragoff, int *flags)

static int ipv4_find_option(struct net *net, struct sk_buff *skb,
                            unsigned int *offset, int target,
                            unsigned short *fragoff, int *flags)
                            ^
Align parameters to parens when breaking too long lines.

Please, also break lines at 80 chars.

> +{
> +	unsigned char optbuf[sizeof(struct ip_options) + 41];
> +	struct ip_options *opt = (struct ip_options *)optbuf;
> +	struct iphdr *iph, _iph;
> +	unsigned int start;
> +	__be32 info;
> +	int optlen;
> +	bool found = false;

Please, define variables using reverse xmas tree, ie.

        unsigned char optbuf[sizeof(struct ip_options) + 41];
        struct ip_options *opt = (struct ip_options *)optbuf;
        struct iphdr *iph, _iph;
        unsigned int start;
        bool found = false;
        __be32 info;
        int optlen;

From longest line to shortest one.

> +	if (fragoff)
> +		*fragoff = 0;
> +
> +	if (!offset)
> +		return -EINVAL;
> +	if (!*offset)
> +		*offset = skb_network_offset(skb);
> +
> +	iph = skb_header_pointer(skb, *offset, sizeof(_iph), &_iph);
> +	if (!iph || iph->version != 4)
> +		return -EBADMSG;
> +	start = *offset + sizeof(struct iphdr);
> +
> +	optlen = iph->ihl * 4 - (int)sizeof(struct iphdr);
> +	if (optlen <= 0)
> +		return -ENOENT;

You could just:

                return -1;

in all these errors in ipv4_find_option() since nft_exthdr_ipv4_eval()
does not use it.

> +	memset(opt, 0, sizeof(struct ip_options));
> +	/* Copy the options since __ip_options_compile() modifies
> +	 * the options. Get one byte beyond the option for target < 0
> +	 */
> +	if (skb_copy_bits(skb, start, opt->__data, optlen + 1))
> +		return -EBADMSG;
> +	opt->optlen = optlen;
> +
> +	if (__ip_options_compile(net, opt, NULL, &info))
> +		return -EBADMSG;
> +
> +	switch (target) {
> +	case IPOPT_SSRR:
> +	case IPOPT_LSRR:
> +		if (opt->srr) {

I'd suggest:

                if (!opt->srr)
                        break;

So you save one level of indentation below.

> +			found = target == IPOPT_SSRR ? opt->is_strictroute :
> +						       !opt->is_strictroute;
> +			if (found)
> +				*offset = opt->srr + start;
> +		}
> +		break;
> +	case IPOPT_RR:
> +		if (opt->rr) {

same here:

                if (!opt->rr)
                        break;

and same thing for other extensions.

> +			*offset = opt->rr + start;
> +			found = true;
> +		}
> +		break;
> +	case IPOPT_RA:
> +		if (opt->router_alert) {
> +			*offset = opt->router_alert + start;
> +			found = true;
> +		}
> +		break;
> +	default:
> +		/* Either not supported or not a specific search, treated as found */
> +		found = true;
> +		if (target < 0) {
> +			if (opt->end) {
> +				*offset = opt->end + start;
> +				target = IPOPT_END;
> +			} else {
> +				/* Point to beyond the options. */
> +				*offset = optlen + start;
> +				target = opt->__data[optlen];
> +			}
> +		} else {
> +			target = -EOPNOTSUPP;
> +		}
> +	}
> +	if (!found)
> +		target = -ENOENT;
> +	return target;

Hm, 'target' value is never used, right?

> +}
> +
> +static void nft_exthdr_ipv4_eval(const struct nft_expr *expr,
> +				 struct nft_regs *regs,
> +				 const struct nft_pktinfo *pkt)
> +{
> +	struct nft_exthdr *priv = nft_expr_priv(expr);
> +	u32 *dest = &regs->data[priv->dreg];
> +	struct sk_buff *skb = pkt->skb;
> +	unsigned int offset = 0;
> +	int err;
> +
> +	err = ipv4_find_option(nft_net(pkt), skb, &offset, priv->type, NULL, NULL);
> +	if (priv->flags & NFT_EXTHDR_F_PRESENT) {
> +		*dest = (err >= 0);
> +		return;
> +	} else if (err < 0) {
> +		goto err;
> +	}
> +	offset += priv->offset;
> +
> +	dest[priv->len / NFT_REG32_SIZE] = 0;
> +	if (skb_copy_bits(pkt->skb, offset, dest, priv->len) < 0)
> +		goto err;
> +	return;
> +err:
> +	regs->verdict.code = NFT_BREAK;
> +}
> +
>  static void *
>  nft_tcp_header_pointer(const struct nft_pktinfo *pkt,
>  		       unsigned int len, void *buffer, unsigned int *tcphdr_len)
> @@ -360,6 +482,14 @@ static const struct nft_expr_ops nft_exthdr_ipv6_ops = {
>  	.dump		= nft_exthdr_dump,
>  };
>  
> +static const struct nft_expr_ops nft_exthdr_ipv4_ops = {
> +	.type		= &nft_exthdr_type,
> +	.size		= NFT_EXPR_SIZE(sizeof(struct nft_exthdr)),
> +	.eval		= nft_exthdr_ipv4_eval,
> +	.init		= nft_exthdr_init,
> +	.dump		= nft_exthdr_dump,
> +};
> +
>  static const struct nft_expr_ops nft_exthdr_tcp_ops = {
>  	.type		= &nft_exthdr_type,
>  	.size		= NFT_EXPR_SIZE(sizeof(struct nft_exthdr)),
> @@ -400,6 +530,12 @@ nft_exthdr_select_ops(const struct nft_ctx *ctx,
>  		if (tb[NFTA_EXTHDR_DREG])
>  			return &nft_exthdr_ipv6_ops;
>  		break;
> +	case NFT_EXTHDR_OP_IPV4:
> +		if (ctx->family == NFPROTO_IPV4) {

This should also work for the NFPROTO_INET (inet tables), NFPROTO_BRIDGE
and the NFPROTO_NETDEV families.

I would turn this into:

		if (ctx->family != NFPROTO_IPV6) {

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

Thanks!

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

[Stephen Suryaputra]

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

> Align parameters to parens when breaking too long lines.
> Please, also break lines at 80 chars.
> Please, define variables using reverse xmas tree, ie.

OK on these.

> > +	optlen = iph->ihl * 4 - (int)sizeof(struct iphdr);
> > +	if (optlen <= 0)
> > +		return -ENOENT;
> 
> You could just:
> 
>                 return -1;
> 
> in all these errors in ipv4_find_option() since nft_exthdr_ipv4_eval()
> does not use it.

Yes, but I followed the pattern in ipv6_find_hdr().

> I'd suggest:
> 
>                 if (!opt->srr)
>                         break;
> 
> So you save one level of indentation below.
> 
> same here:
> 
>                 if (!opt->rr)
>                         break;
> 
> and same thing for other extensions.

OK on these.
> 
> > +	if (!found)
> > +		target = -ENOENT;
> > +	return target;
> 
> Hm, 'target' value is never used, right?

Again, this follows ipv6_find_hdr().

> This should also work for the NFPROTO_INET (inet tables), NFPROTO_BRIDGE
> and the NFPROTO_NETDEV families.
> 
> I would turn this into:
> 
> 		if (ctx->family != NFPROTO_IPV6) {
> 

OK.

> > +			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;

Thank you.

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

[Pablo Neira Ayuso]

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.

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > »       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;

Wouldn't it be preferable to just use nft_pf() != NFPROTO_IPV4?

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

[Pablo Neira Ayuso]

On Sat, Jun 01, 2019 at 10:27:32AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > »       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;
> 
> Wouldn't it be preferable to just use nft_pf() != NFPROTO_IPV4?

Then IPv4 options extension won't work from bridge and netdev families
too, right?

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >         if (skb->protocol != htons(ETH_P_IP))
> > >                 goto err;
> > 
> > Wouldn't it be preferable to just use nft_pf() != NFPROTO_IPV4?
> 
> Then IPv4 options extension won't work from bridge and netdev families
> too, right?

Ah, right.

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

[Stephen Suryaputra]

On Sat, Jun 01, 2019 at 02:22:30AM +0200, Pablo Neira Ayuso wrote:
> > 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?

I developed this patchset to suit my employer needs and there is no plan
for a follow up patchset, however I think non-zero offset might be useful
in the future for tunneled packets.

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

Got it.

Thanks.

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

[Stephen Suryaputra]

On Mon, Jun 03, 2019 at 02:30:06PM +0200, Pablo Neira Ayuso wrote:
> > I developed this patchset to suit my employer needs and there is no plan
> > for a follow up patchset, however I think non-zero offset might be useful
> > in the future for tunneled packets.
> 
> For tunneled traffic, we can store the network offset in the
> nft_pktinfo object. Then, add a new extension to update this network
> offset to point to the network offset inside the tunnel header, and
> use this pkt->network_offset everywhere.

OK. I'm changing so that offset isn't being used as input. But, it's
still being passed as reference for output. See further response
below...

> I think this new IPv4 options extension should use priv->offset to
> match fields inside the IPv4 option specifically, just like in the
> IPv6 extensions and TCP options do. If you look on how the
> priv->offset is used in the existing code, this offset points to
> values that the specific option field conveys.

I believe that's what I have coded:

	err = ipv4_find_option(nft_net(pkt), skb, &offset, priv->type, NULL, NULL);
	if (priv->flags & NFT_EXTHDR_F_PRESENT) {
		*dest = (err >= 0);
		return;
	} else if (err < 0) {
		goto err;
	}
	offset += priv->offset;

offset is returned as the offset where it matches the sought priv->type
then priv->offset is added to get to the right field between the offset.

If this is satisfactory, I can submit v2 of the kernel patch.

Thanks,

Stephen.

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

[Pablo Neira Ayuso]

On Sat, Jun 01, 2019 at 11:04:29AM -0400, Stephen Suryaputra wrote:
> On Sat, Jun 01, 2019 at 02:22:30AM +0200, Pablo Neira Ayuso wrote:
> > > 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?
> 
> I developed this patchset to suit my employer needs and there is no plan
> for a follow up patchset, however I think non-zero offset might be useful
> in the future for tunneled packets.

For tunneled traffic, we can store the network offset in the
nft_pktinfo object. Then, add a new extension to update this network
offset to point to the network offset inside the tunnel header, and
use this pkt->network_offset everywhere.

I think this new IPv4 options extension should use priv->offset to
match fields inside the IPv4 option specifically, just like in the
IPv6 extensions and TCP options do. If you look on how the
priv->offset is used in the existing code, this offset points to
values that the specific option field conveys.

Thanks.

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

[Pablo Neira Ayuso]

On Sat, Jun 01, 2019 at 10:27:06PM -0400, Stephen Suryaputra wrote:
> On Mon, Jun 03, 2019 at 02:30:06PM +0200, Pablo Neira Ayuso wrote:
> > > I developed this patchset to suit my employer needs and there is no plan
> > > for a follow up patchset, however I think non-zero offset might be useful
> > > in the future for tunneled packets.
> > 
> > For tunneled traffic, we can store the network offset in the
> > nft_pktinfo object. Then, add a new extension to update this network
> > offset to point to the network offset inside the tunnel header, and
> > use this pkt->network_offset everywhere.
> 
> OK. I'm changing so that offset isn't being used as input. But, it's
> still being passed as reference for output. See further response
> below...
> 
> > I think this new IPv4 options extension should use priv->offset to
> > match fields inside the IPv4 option specifically, just like in the
> > IPv6 extensions and TCP options do. If you look on how the
> > priv->offset is used in the existing code, this offset points to
> > values that the specific option field conveys.
> 
> I believe that's what I have coded:
> 
> 	err = ipv4_find_option(nft_net(pkt), skb, &offset, priv->type, NULL, NULL);
> 	if (priv->flags & NFT_EXTHDR_F_PRESENT) {
> 		*dest = (err >= 0);
> 		return;
> 	} else if (err < 0) {
> 		goto err;
> 	}
> 	offset += priv->offset;
> 
> offset is returned as the offset where it matches the sought priv->type
> then priv->offset is added to get to the right field between the offset.

I see, thanks for explaining.

I got me confused when I read this:

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

I thought this is how the new extension for nftables is working. Not
the function.

And then, this chunk:

+       if (!offset)
+               return -EINVAL;

This never happens, right? offset is always set.

+       if (!*offset)
+               *offset = skb_network_offset(skb);

So this is not needed either.

I would remove those, you can add more code to ipv4_find_option()
later on as you get more clients in the networking tree. I'd suggest,
better remove code that is not used yet, then introduce it once
needed.

> If this is satisfactory, I can submit v2 of the kernel patch.

Please do so, so you get more feedback (if needed) and we move on :-)

Thanks!