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

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

On Tue, Jun 11, 2019 at 08:09:12AM -0400, Stephen Suryaputra wrote:
[...]
> diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
> index a940c9fd9045..4155a32fade7 100644
> --- a/net/netfilter/nft_exthdr.c
> +++ b/net/netfilter/nft_exthdr.c
> @@ -62,6 +62,125 @@ 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.
> + *
> + * 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)

flags is never used, please remove it.

> +{
> +	unsigned char optbuf[sizeof(struct ip_options) + 41];

In other parts of the kernel this is + 40:

net/ipv4/cipso_ipv4.c:  unsigned char optbuf[sizeof(struct ip_options) + 40];

here it is + 41.

> +	struct ip_options *opt = (struct ip_options *)optbuf;
> +	struct iphdr *iph, _iph;
> +	unsigned int start;
> +	bool found = false;
> +	__be32 info;
> +	int optlen;
> +
> +	if (fragoff)
> +		*fragoff = 0;

fragoff is set and never used. Please, remove this parameter.

> +	iph = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> +	if (!iph || iph->version != 4)
> +		return -EBADMSG;
> +	start = 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

How does this "one byte beyond the option" trick works?

> +	 */
> +	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)
> +			break;
> +		found = target == IPOPT_SSRR ? opt->is_strictroute :
> +					       !opt->is_strictroute;
> +		if (found)
> +			*offset = opt->srr + start;
> +		break;
> +	case IPOPT_RR:
> +		if (opt->rr)
> +			break;
> +		*offset = opt->rr + start;
> +		found = true;
> +		break;
> +	case IPOPT_RA:
> +		if (opt->router_alert)
> +			break;
> +		*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) {
> +			target = -EOPNOTSUPP;
> +			break;
> +		}
> +		if (opt->end) {
> +			*offset = opt->end + start;
> +			target = IPOPT_END;

May I ask, what's the purpose of IPOPT_END? :-)

> +		} else {
> +			/* Point to beyond the options. */
> +			*offset = optlen + start;
> +			target = opt->__data[optlen];
> +		}
> +	}
> +	if (!found)
> +		target = -ENOENT;
> +	return target;

nitpick: Probably replace code above.

        return found ? target : -ENOENT;

Apart from the above, this looks good to me.

Thanks!