Re: [PATCH net] netfilter: ip6t_NPT: ensure skb is writable before header modification

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

Hi,

On Wed, Sep 30, 2026 at 11:09:39AM -0700, Joas Antonio wrote:
> ip6t_snpt_tg() and ip6t_dnpt_tg() modify the IPv6 source/destination
> address in-place via ip6t_npt_map_pfx() without first calling
> skb_ensure_writable().  When the skb is cloned (nflog, tee, multicast,
> tc mirred), the write hits shared data visible to other users of the
> same buffer:
> 
>   if (!ip6t_npt_map_pfx(npt, &ipv6_hdr(skb)->saddr))
>           ...
> 
> ip6t_npt_map_pfx() does the actual in-place modification:
> 
>   addr->s6_addr32[idx] &= mask;
>   addr->s6_addr32[idx] |= ~mask & npt->dst_pfx.in6.s6_addr32[idx];
> 
> Neither target checks writability first.  Every comparable target in
> the tree does: ipt_ECN (line 32), ebt_dnat (line 24), ebt_snat
> (line 25), xt_TCPOPTSTRIP (line 52).
> 
> Additionally, the ICMPv6 bounced header path uses
> skb_header_pointer(), which returns a pointer to a stack-local copy
> for nonlinear skbs.  ip6t_npt_map_pfx() then modifies the copy;
> changes are silently lost and the inner IPv6 header in ICMPv6 error
> messages is never translated.
> 
> Fix both by adding skb_ensure_writable() before each target function
> operates, and replacing the skb_header_pointer() + helper pattern
> with skb_ensure_writable() up to the inner header offset followed by
> a direct pointer.  Remove the now-unused icmpv6_bounced_ipv6hdr().
> 
> Fixes: 8a91bb0c304b ("netfilter: ip6tables: add stateless IPv6-to-IPv6 Network Prefix Translation target")
> Signed-off-by: Joas Antonio <joasantonio108@gmail.com>
> ---
> Testing: verified with NFLOG + SNPT/DNPT rules on a namespace pair
> that cloned skb data is no longer corrupted after translation.
> Nonlinear-skb path confirmed by forcing GRO aggregation on the
> ingress side; inner ICMPv6 header now carries the translated prefix.

This is nf-next material.

>  net/ipv6/netfilter/ip6t_NPT.c | 66 ++++++++++++++++++--------------
>  1 file changed, 37 insertions(+), 29 deletions(-)
> 
> diff --git a/net/ipv6/netfilter/ip6t_NPT.c b/net/ipv6/netfilter/ip6t_NPT.c
> index XXXXXXX..XXXXXXX 100644
> --- a/net/ipv6/netfilter/ip6t_NPT.c
> +++ b/net/ipv6/netfilter/ip6t_NPT.c
> @@ -78,57 +78,65 @@ static bool ip6t_npt_map_pfx(const struct ip6t_npt_tginfo *npt,
>  	return true;
>  }
> 
> -static struct ipv6hdr *icmpv6_bounced_ipv6hdr(struct sk_buff *skb,
> -					      struct ipv6hdr *_bounced_hdr)
> -{
> -	if (ipv6_hdr(skb)->nexthdr != IPPROTO_ICMPV6)
> -		return NULL;
> -
> -	if (!icmpv6_is_err(icmp6_hdr(skb)->icmp6_type))
> -		return NULL;
> -
> -	return skb_header_pointer(skb,
> -				  skb_transport_offset(skb) + sizeof(struct icmp6hdr),
> -				  sizeof(struct ipv6hdr),
> -				  _bounced_hdr);
> -}
> -
>  static unsigned int
>  ip6t_snpt_tg(struct sk_buff *skb, const struct xt_action_param *par)
>  {
>  	const struct ip6t_npt_tginfo *npt = par->targinfo;
> -	struct ipv6hdr _bounced_hdr;
> -	struct ipv6hdr *bounced_hdr;
>  	struct in6_addr bounced_pfx;
> +	struct ipv6hdr *bounced_hdr;
> +	unsigned int bounced_off;
> +
> +	if (skb_ensure_writable(skb, sizeof(struct ipv6hdr)))
> +		return NF_DROP;
> 
>  	if (!ip6t_npt_map_pfx(npt, &ipv6_hdr(skb)->saddr)) {
>  		icmpv6_send(skb, ICMPV6_PARAMPROB, ICMPV6_HDR_FIELD,
>  			    offsetof(struct ipv6hdr, saddr));
>  		return NF_DROP;
>  	}
> 
>  	/* rewrite dst addr of bounced packet which was sent to dst range */
> -	bounced_hdr = icmpv6_bounced_ipv6hdr(skb, &_bounced_hdr);
> -	if (bounced_hdr) {
> -		ipv6_addr_prefix(&bounced_pfx, &bounced_hdr->daddr, npt->src_pfx_len);
> -		if (ipv6_addr_cmp(&bounced_pfx, &npt->src_pfx.in6) == 0)
> -			ip6t_npt_map_pfx(npt, &bounced_hdr->daddr);
> -	}
> +	if (ipv6_hdr(skb)->nexthdr != IPPROTO_ICMPV6 ||
> +	    !icmpv6_is_err(icmp6_hdr(skb)->icmp6_type))
> +		return XT_CONTINUE;
> +
> +	bounced_off = skb_transport_offset(skb) + sizeof(struct icmp6hdr);
> +	if (skb_ensure_writable(skb, bounced_off + sizeof(struct ipv6hdr)))
> +		return XT_CONTINUE;
> +
> +	bounced_hdr = (struct ipv6hdr *)(skb_transport_header(skb) +
> +					  sizeof(struct icmp6hdr));
> +	ipv6_addr_prefix(&bounced_pfx, &bounced_hdr->daddr, npt->src_pfx_len);
> +	if (ipv6_addr_cmp(&bounced_pfx, &npt->src_pfx.in6) == 0)
> +		ip6t_npt_map_pfx(npt, &bounced_hdr->daddr);
> 
>  	return XT_CONTINUE;
>  }
> 
>  static unsigned int
>  ip6t_dnpt_tg(struct sk_buff *skb, const struct xt_action_param *par)
>  {
>  	const struct ip6t_npt_tginfo *npt = par->targinfo;
> -	struct ipv6hdr _bounced_hdr;
> -	struct ipv6hdr *bounced_hdr;
>  	struct in6_addr bounced_pfx;
> +	struct ipv6hdr *bounced_hdr;
> +	unsigned int bounced_off;
> +
> +	if (skb_ensure_writable(skb, sizeof(struct ipv6hdr)))
> +		return NF_DROP;
> 
>  	if (!ip6t_npt_map_pfx(npt, &ipv6_hdr(skb)->daddr)) {
>  		icmpv6_send(skb, ICMPV6_PARAMPROB, ICMPV6_HDR_FIELD,
>  			    offsetof(struct ipv6hdr, daddr));
>  		return NF_DROP;
>  	}
> 
>  	/* rewrite src addr of bounced packet which was sent from dst range */
> -	bounced_hdr = icmpv6_bounced_ipv6hdr(skb, &_bounced_hdr);
> -	if (bounced_hdr) {
> -		ipv6_addr_prefix(&bounced_pfx, &bounced_hdr->saddr, npt->src_pfx_len);
> -		if (ipv6_addr_cmp(&bounced_pfx, &npt->src_pfx.in6) == 0)
> -			ip6t_npt_map_pfx(npt, &bounced_hdr->saddr);
> -	}
> +	if (ipv6_hdr(skb)->nexthdr != IPPROTO_ICMPV6 ||
> +	    !icmpv6_is_err(icmp6_hdr(skb)->icmp6_type))
> +		return XT_CONTINUE;
> +
> +	bounced_off = skb_transport_offset(skb) + sizeof(struct icmp6hdr);
> +	if (skb_ensure_writable(skb, bounced_off + sizeof(struct ipv6hdr)))
> +		return XT_CONTINUE;
> +
> +	bounced_hdr = (struct ipv6hdr *)(skb_transport_header(skb) +
> +					  sizeof(struct icmp6hdr));
> +	ipv6_addr_prefix(&bounced_pfx, &bounced_hdr->saddr, npt->src_pfx_len);
> +	if (ipv6_addr_cmp(&bounced_pfx, &npt->src_pfx.in6) == 0)
> +		ip6t_npt_map_pfx(npt, &bounced_hdr->saddr);
> 
>  	return XT_CONTINUE;
>  }
> --
> 2.43.0