Re: [PATCH nf-next] netfilter: nf_reject_ipv6: do not reject ICMPv6 Redirect with an ICMPv6 error

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

On Mon, Jun 08, 2026 at 04:01:55PM +0530, Sayooj K Karun wrote:
> While fixing is_ineligible() for the L3 reject path, the bridge/netdev
> reject path was found to have the same defect. RFC 4443 section 2.4(e.2)
> mandates that an ICMPv6 error MUST NOT be originated in response to an
> ICMPv6 Redirect message (type 137).
> 
> There are two IPv6 reject paths, and they suppress this in different
> places. The L3 path (ip6t_REJECT / nft_reject -> nf_send_unreach6())
> goes through icmpv6_send(), where is_ineligible() decides whether the
> triggering packet may generate an error; a companion fix makes that gate
> drop errors in response to a Redirect. The bridge/netdev path
> (nft_reject_bridge / nft_reject_netdev -> nf_reject_skb_v6_unreach())
> builds the ICMPv6 packet itself and never reaches is_ineligible().
> Its local guard, nf_skb_is_icmp6_unreach(), only matched
> ICMPV6_DEST_UNREACH and let every other type through,
> including Redirect.
> 
> A triggerable scenario: a bridge or netdev firewall with a REJECT rule
> applied to incoming ICMPv6 traffic (e.g., dropping Redirects from an
> untrusted segment). When the Redirect hits the REJECT rule,
> nf_reject_skb_v6_unreach() builds and transmits a Destination Unreachable
> in response. Without this fix the guard lets the Redirect through and the
> error is erroneously sent, violating the RFC.
> 
> Extend the guard, renamed nf_skb_is_icmp6_unreach_or_redirect(), to also
> match NDISC_REDIRECT so that Redirect packets are skipped and both reject
> paths behave consistently.
> 
> Link: https://lore.kernel.org/ah_hYJa3byoUyose@chamomile
> Signed-off-by: Sayooj K Karun <sayooj@aerlync.com>
> ---
>  net/ipv6/netfilter/nf_reject_ipv6.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv6/netfilter/nf_reject_ipv6.c b/net/ipv6/netfilter/nf_reject_ipv6.c
> index ef5b7e85c..d4eec8d9a 100644
> --- a/net/ipv6/netfilter/nf_reject_ipv6.c
> +++ b/net/ipv6/netfilter/nf_reject_ipv6.c
> @@ -8,6 +8,7 @@
>  #include <net/ip6_route.h>
>  #include <net/ip6_fib.h>
>  #include <net/ip6_checksum.h>
> +#include <net/ndisc.h>
>  #include <net/netfilter/ipv6/nf_reject.h>
>  #include <linux/netfilter_ipv6.h>
>  #include <linux/netfilter_bridge.h>
> @@ -104,7 +105,7 @@ struct sk_buff *nf_reject_skb_v6_tcp_reset(struct net *net,
>  }
>  EXPORT_SYMBOL_GPL(nf_reject_skb_v6_tcp_reset);
>  
> -static bool nf_skb_is_icmp6_unreach(const struct sk_buff *skb)
> +static bool nf_skb_is_icmp6_unreach_or_redirect(const struct sk_buff *skb)
>  {
>  	const struct ipv6hdr *ip6h = ipv6_hdr(skb);
>  	u8 proto = ip6h->nexthdr;
> @@ -127,7 +128,7 @@ static bool nf_skb_is_icmp6_unreach(const struct sk_buff *skb)
>  	if (!tp)
>  		return false;
>  
> -	return *tp == ICMPV6_DEST_UNREACH;
> +	return *tp == ICMPV6_DEST_UNREACH || *tp == NDISC_REDIRECT;
>  }
>  
>  struct sk_buff *nf_reject_skb_v6_unreach(struct net *net,
> @@ -143,8 +144,13 @@ struct sk_buff *nf_reject_skb_v6_unreach(struct net *net,
>  	if (!nf_reject_ip6hdr_validate(oldskb))
>  		return NULL;
>  
> -	/* Don't reply to ICMPV6_DEST_UNREACH with ICMPV6_DEST_UNREACH */
> -	if (nf_skb_is_icmp6_unreach(oldskb))
> +	/* Don't reply to ICMPV6_DEST_UNREACH with ICMPV6_DEST_UNREACH, and
> +	 * per RFC 4443 section 2.4(e.2) never originate an ICMPv6 error in
> +	 * response to an ICMPv6 Redirect. The L3 reject path enforces this
> +	 * via icmpv6_send()/is_ineligible(); this bridge/netdev path builds
> +	 * the packet itself, so it must check explicitly.

Please, shorter comment. With all these AI generated comments,
codebase will bloat soon. Comments are usually reserve to non-obvious
stuff IIRC, this extra validation can be possibly documented in the
new nf_skb_is_icmp6_unreach_or_redirect_or_something_else().

> +	 */
> +	if (nf_skb_is_icmp6_unreach_or_redirect(oldskb))

Please, use more generic function:

        nf_skb_icmp6_is_valid()

so this can grown with any other future extension without adding
another _or_something_else() to the function name.

Thanks.

>  		return NULL;
>  
>  	/* Include "As much of invoking packet as possible without the ICMPv6
> -- 
> 2.54.0
>