Re: [PATCH nf-next] netfilter: nft_exthdr: add boolean DCCP option matching

[Florian Westphal <fw@strlen.de>]

Jeremy Sowden <jeremy@azazel.net> wrote:
> The xt_dccp iptables module supports the matching of DCCP packets based
> on the presence or absence of DCCP options.  Extend nft_exthdr to add
> this functionality to nftables.
> 
> Link: https://bugzilla.netfilter.org/show_bug.cgi?id=930
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> ---
>  include/uapi/linux/netfilter/nf_tables.h |   2 +
>  net/netfilter/nft_exthdr.c               | 105 +++++++++++++++++++++++
> +struct nft_exthdr_dccp {
> +	struct nft_exthdr exthdr;
> +	/* A buffer into which to copy the DCCP packet options for parsing.  The
> +	 * options are located between the packet header and its data.  The
> +	 * offset of the data from the start of the header is stored in an 8-bit
> +	 * field as the number of 32-bit words, so the options will definitely
> +	 * be shorter than `4 * U8_MAX` bytes.
> +	 */
> +	u8 optbuf[4 * U8_MAX];
> +};
> +
>  static unsigned int optlen(const u8 *opt, unsigned int offset)
>  {
>  	/* Beware zero-length options: make finite progress */
> @@ -406,6 +418,70 @@ static void nft_exthdr_sctp_eval(const struct nft_expr *expr,
>  		regs->verdict.code = NFT_BREAK;
>  }
>  
> +static void nft_exthdr_dccp_eval(const struct nft_expr *expr,
> +				 struct nft_regs *regs,
> +				 const struct nft_pktinfo *pkt)
> +{
> +	struct nft_exthdr_dccp *priv_dccp = nft_expr_priv(expr);
> +	struct nft_exthdr *priv = &priv_dccp->exthdr;
> +	u32 *dest = &regs->data[priv->dreg];
> +	unsigned int optoff, optlen, i;
> +	const struct dccp_hdr *dh;
> +	struct dccp_hdr _dh;
> +	const u8 *options;
> +
> +	if (pkt->tprot != IPPROTO_DCCP || pkt->fragoff)
> +		goto err;
> +
> +	dh = skb_header_pointer(pkt->skb, nft_thoff(pkt), sizeof(_dh), &_dh);
> +	if (!dh)
> +		goto err;
> +
> +	if (dh->dccph_doff * 4 < __dccp_hdr_len(dh))
> +		goto err;
> +
> +	optoff = __dccp_hdr_len(dh);
> +	optlen = dh->dccph_doff * 4 - optoff;

Perhaps reorder this slightly:

     optoff = __dccp_hdr_len(dh);
     if (dh->dccph_doff * 4 <= optoff)
	     goto err;

     optlen = dh->dccph_doff * 4 - optoff;

     options = skb_header_pointer(pkt->skb, nft_thoff(pkt) + optoff, optlen,
				     priv_dccp->optbuf);

This isn't safe.  priv_dccp->optbuf is neither percpu nor is there
something that prevents a softinterrupt from firing.

I suggest you have a look at 'pipapo' set type which uses percpu scratch
maps.

Yet another alternative is to provide a small onstack scratch buffer,
say 256 byte, and fall back to kmalloc for larger spaces.

Or, always use a on-stack buffer that gets re-used for each of the
parsed options.

> +	for (i = 0; i < optlen; ) {
> +		/* Options 0 - 31 are 1B in the length.  Options 32 et seq. are
> +		 * at least 2B long.  In all cases, the first byte contains the
> +		 * option type.  In multi-byte options, the second byte contains
> +		 * the option length, which must be at least two; if it is
> +		 * greater than two, there are `len - 2` following bytes of
> +		 * option data.
> +		 */
> +		unsigned int len;
> +
> +		if (options[i] > 31 && (optlen - i < 2 || options[i + 1] < 2))
> +			goto err;
> +
> +		len = options[i] > 31 ? options[i + 1] : 1;
> +
> +		if (optlen - i < len)
> +			goto err;
> +
> +		if (options[i] != priv->type) {
> +			i += len;

I think this needs to guard against len == 0?