Re: [PATCH net 06/10] netfilter: nf_reject: initialize IPCB at inet ingress

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new IPv6 initializer in nf_send_unreach6() sets
  IP6CB(skb_in)->iif = skb_in->skb_iif unconditionally.

Pre-existing issues:
- [Low] At NF_INET_INGRESS, the reject helpers do not check the L4
  checksum of CHECKSUM_COMPLETE packets.

--

netfilter: nf_reject: initialize IPCB at inet ingress

The inet ingress hook runs before ip_rcv_core() and ip6_rcv_core(), so
skb->cb can still hold stale data from an earlier layer. When called from
NF_INET_INGRESS, nf_send_unreach() and nf_send_unreach6() now clear
IPCB/IP6CB and restore iif. This stops stale IPv4 option offsets from
overflowing the stack buffer in __ip_options_echo().

> diff --git a/net/ipv4/netfilter/nf_reject_ipv4.c b/net/ipv4/netfilter/nf_reject_ipv4.c
> index 59ec465a9df9..7cb33af671e2 100644
> --- a/net/ipv4/netfilter/nf_reject_ipv4.c
> +++ b/net/ipv4/netfilter/nf_reject_ipv4.c
> @@ -357,6 +357,11 @@ void nf_send_unreach(struct sk_buff *skb_in, int code, int hook)
>  	if (!skb_valid_dst(skb_in) && nf_reject_fill_skb_dst(skb_in) < 0)
>  		return;
>  
> +	if (hook == NF_INET_INGRESS) {
> +		memset(IPCB(skb_in), 0, sizeof(*IPCB(skb_in)));
> +		IPCB(skb_in)->iif = skb_in->skb_iif;
> +	}
> +
>  	if (skb_csum_unnecessary(skb_in) ||
>  	    !nf_reject_verify_csum(skb_in, dataoff, proto)) {
>  		icmp_send(skb_in, ICMP_DEST_UNREACH, code, 0);

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. Since
this path now has NF_INET_INGRESS handling, though: is the L4 checksum of
a CHECKSUM_COMPLETE packet ever checked at this hook?

Further down, nf_send_unreach() does:

	if (nf_ip_checksum(skb_in, hook, dataoff, proto) == 0)
		icmp_send(skb_in, ICMP_DEST_UNREACH, code, 0);

nf_ip_checksum() in net/netfilter/utils.c only verifies for PRE_ROUTING
and LOCAL_IN:

	case CHECKSUM_COMPLETE:
		if (hook != NF_INET_PRE_ROUTING && hook != NF_INET_LOCAL_IN)
			break;

With hook == NF_INET_INGRESS, csum stays 0, and the reject code reads that
as a good checksum.

nf_ip6_checksum() has the same check. It affects
reject6_csum_ok()->icmpv6_send() in nf_send_unreach6(), and also the TCP
RST path through nf_reject_ip6_tcphdr_get().

So can an inet ingress reject rule answer a packet with a bad L4 checksum
with an ICMP/ICMPv6 unreachable or a TCP RST? The same packet would get no
reply at PRE_ROUTING, or at netdev ingress (NF_NETDEV_INGRESS has the same
value as NF_INET_PRE_ROUTING).

Adding NF_INET_INGRESS to that hook list may not be enough on its own. At
ingress, skb->len can still include link-layer trailer padding, which
ip_rcv_core()/ip6_rcv_core() would normally trim.

This came in with commit 117ca1f8920c ("netfilter: nft_reject_inet: allow
to use reject from inet ingress").

> diff --git a/net/ipv6/netfilter/nf_reject_ipv6.c b/net/ipv6/netfilter/nf_reject_ipv6.c
> index 07cdaa10da0d..5e08d8529cc8 100644
> --- a/net/ipv6/netfilter/nf_reject_ipv6.c
> +++ b/net/ipv6/netfilter/nf_reject_ipv6.c
> @@ -444,6 +444,11 @@ void nf_send_unreach6(struct net *net, struct sk_buff *skb_in,
>  	if (hooknum == NF_INET_LOCAL_OUT && skb_in->dev == NULL)
>  		skb_in->dev = net->loopback_dev;
>  
> +	if (hooknum == NF_INET_INGRESS) {
> +		memset(IP6CB(skb_in), 0, sizeof(*IP6CB(skb_in)));
> +		IP6CB(skb_in)->iif = skb_in->skb_iif;
> +	}

[Severity: Medium]
The commit message says this matches ip6_rcv_core(). Does that hold for
packets looped back through lo? ip6_rcv_core() chooses iif based on the
dst:

	IP6CB(skb)->iif = skb_valid_dst(skb) ?
				ip6_dst_idev(skb_dst(skb))->dev->ifindex :
				dev->ifindex;

Its comment explains that a locally generated packet sent to a local
address on ethX arrives through lo. For scoping, though, it has to be
treated as arriving on ethX.

loopback_xmit() keeps the dst with skb_dst_force(). Then
__netif_receive_skb_core() sets skb->skb_iif to lo's ifindex before
nf_ingress() runs. For an inet ingress reject rule on lo, this code sets
iif to lo, while ip6_rcv_core() would use the dst's interface.

icmp6_send() uses parm->iif to pick the source address, through
dev_get_by_index() and ipv6_dev_get_saddr(). It also passes parm->iif to
icmp6_ext_append() for the RFC 5837 interface information.

Can the resulting ICMPv6 error get the wrong source address, or report lo
as the incoming interface?

nf_reject6_fill_skb_dst() runs after this block, so the original dst is
still attached here. Would something like this be closer to
ip6_rcv_core()?

	IP6CB(skb_in)->iif = skb_valid_dst(skb_in) ?
		ip6_dst_idev(skb_dst(skb_in))->dev->ifindex :
		skb_in->skb_iif;

> +
>  	if (!skb_valid_dst(skb_in) && nf_reject6_fill_skb_dst(skb_in) < 0)
>  		return;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019150249.1879516-2-pablo%40netfilter.org