Re: [PATCH nf v2 1/1] netfilter: validate L4 headers after userspace packet writes

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

On Thu, Jul 30, 2026 at 03:30:26PM +0800, Zhiling Zou wrote:
[...]
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index b8aaf39cb4d8e..562b7bf6c8777 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -28,10 +28,16 @@
>  #include <linux/netfilter/nfnetlink.h>
>  #include <linux/netfilter/nfnetlink_queue.h>
>  #include <linux/netfilter/nf_conntrack_common.h>
> +#include <linux/icmp.h>
> +#include <linux/icmpv6.h>
>  #include <linux/list.h>
> +#include <linux/sctp.h>
>  #include <linux/cgroup-defs.h>
>  #include <linux/rhashtable.h>
>  #include <linux/jhash.h>
> +#include <linux/tcp.h>
> +#include <linux/udp.h>
> +#include <net/gre.h>
>  #include <net/gso.h>
>  #include <net/sock.h>
>  #include <net/tcp_states.h>
> @@ -1192,6 +1198,49 @@ nfqnl_enqueue_packet(struct nf_queue_entry *entry, unsigned int queuenum)
>  	return err;
>  }
>  
> +static bool nfqnl_validate_l4(const u8 *data, unsigned int data_len,
> +			      const struct nf_queue_entry *e, u8 proto)
> +{
> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
> +	enum ip_conntrack_info ctinfo;
> +	const struct nf_conn *ct;
> +
> +	ct = nf_ct_get(e->skb, &ctinfo);
> +	if (ct && !nf_ct_is_template(ct) && nf_ct_protonum(ct) != proto)

I think it should be easier to disallow protocol number mangling in
the IP header (layer 3 restrictions), if not done already.

> +		return false;
> +#endif
> +
> +	switch (proto) {
> +	case IPPROTO_TCP: {
> +		const struct tcphdr *th = (const struct tcphdr *)data;

This needs to use skb_header_pointer() here, you cannot assume the tcp
header is in a linear area.

> +		unsigned int thlen;
> +
> +		if (data_len < sizeof(*th))
> +			return false;
> +
> +		thlen = __tcp_hdrlen(th);
> +		if (thlen < sizeof(*th) || data_len < thlen)
> +			return false;
> +
> +		return true;
> +	}
> +	case IPPROTO_UDP:
> +		return data_len >= sizeof(struct udphdr);
> +	case IPPROTO_ICMP:
> +		return data_len >= sizeof(struct icmphdr);
> +	case IPPROTO_ICMPV6:
> +		return data_len >= sizeof(struct icmp6hdr);
> +	case IPPROTO_SCTP:
> +		return data_len >= sizeof(struct sctphdr);
> +	case IPPROTO_GRE:
> +		return data_len >= sizeof(struct gre_base_hdr);
> +	case IPPROTO_NONE:

Remove this and make it part of default and return true if protocol is
unknown.

> +		return true;
> +	}
> +
> +	return false;

Default is false for an unknown protocol, should be true.

> +}
> +
>  static bool nfqnl_validate_ipopts(const struct iphdr *iph_new,
>  				  const struct nf_queue_entry *e)
>  {
> @@ -1229,7 +1278,9 @@ static bool nfqnl_validate_ip4(const struct iphdr *iph, unsigned int data_len,
>  	/* support for ipopts mangling would require
>  	 * recompile + skb transport header update.
>  	 */
> -	return nfqnl_validate_ipopts(iph, e);
> +	return nfqnl_validate_ipopts(iph, e) &&
> +	       nfqnl_validate_l4((const u8 *)iph + ihl, data_len - ihl, e,
> +				 iph->protocol);
>  }
>  
>  static bool nfqnl_validate_one_exthdr(const u8 *data,
> @@ -1286,7 +1337,8 @@ static bool nfqnl_validate_exthdr(const struct ipv6hdr *ip6_new,
>  		int hdrlen;
>  
>  		if (orig_nexthdr == NEXTHDR_NONE)
> -			return true;
> +			return nfqnl_validate_l4(data, data_len, e,
> +						 new_nexthdr);
>  
>  		if (unlikely(exthdr_cnt++ >= IP6_MAX_EXT_HDRS_CNT))
>  			return false;
> @@ -1323,7 +1375,7 @@ static bool nfqnl_validate_exthdr(const struct ipv6hdr *ip6_new,
>  		data += hdrlen;
>  	}
>  
> -	return true;
> +	return nfqnl_validate_l4(data, data_len, e, new_nexthdr);
>  }
>  
>  static bool nfqnl_validate_ip6(const struct ipv6hdr *ip6, unsigned int data_len,
> diff --git a/net/netfilter/nft_payload.c b/net/netfilter/nft_payload.c
> index 391539a1ceaa7..63cf3b4f7420d 100644
> --- a/net/netfilter/nft_payload.c
> +++ b/net/netfilter/nft_payload.c
> @@ -1077,6 +1077,17 @@ static bool nft_payload_csum_write_ok(const struct nft_pktinfo *pkt,
>  	return false;
>  }
>  
> +static bool nft_th_write_ok(const struct nft_pktinfo *pkt,
> +			    const struct nft_payload_set *priv)
> +{
> +	unsigned int doff = offsetof(struct tcphdr, ack_seq) + sizeof(__be32);
> +
> +	if (pkt->tprot != IPPROTO_TCP)
> +		return true;
> +
> +	return priv->offset > doff || priv->offset + priv->len <= doff;
                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

maybe simply check priv->offset >= doff here?

> +}
> +
>  static void nft_payload_set_eval(const struct nft_expr *expr,
>  				 struct nft_regs *regs,
>  				 const struct nft_pktinfo *pkt)
> @@ -1115,6 +1126,8 @@ static void nft_payload_set_eval(const struct nft_expr *expr,
>  	case NFT_PAYLOAD_TRANSPORT_HEADER:
>  		if (!(pkt->flags & NFT_PKTINFO_L4PROTO) || pkt->fragoff)
>  			goto err;
> +		if (!nft_th_write_ok(pkt, priv))
> +			goto err;
>  		offset = nft_thoff(pkt);
>  		break;
>  	case NFT_PAYLOAD_INNER_HEADER:
> -- 
> 2.43.0