[PATCH net 0/7] Netfilter/IPVS fixes for net

[PATCH net 0/7] Netfilter/IPVS fixes for net

[Pablo Neira Ayuso]

Hi,

The following patchset provides fixes for Netfilter/IPVS:

1) Fix KMSAN reports an uninit-value in nf_nat_setup_info() for netmap,
   from Theodor Arsenij Larionov Trichkine.

2) Restrict deletion of netdevice in basechain and flowtable to exact
   matching only, from Fernando F. Mancera.

3) Fix nf_nat_register_fn() error path allowing for a memleak.

4) Revalidate ihl before calling icmp_send() in IPVS,
   from Julian Anastasov.

5) Hold reference on ct until flow is released to address, otherwise
   access to release ct->ext or different ct due to typesafe RCU
   semantics.

6) Use kzalloc_obj() to allocate timer object in xt_IDLETIMER as
   reported by sashiko to address uninitialized access.

7) Hold reference on module during netlink for cttimeout, cthelper
   ctnetlink and nfnetlink_acct.

Please, pull these changes from:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git nf-26-09-10

Thanks.

----------------------------------------------------------------

The following changes since commit 1b8e56030d52cd3e52c9ad4df1985ad0440be67d:

  Merge tag 'nf-26-09-07' of git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf (2026-09-08 13:53:17 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git nf-26-09-10

for you to fetch changes up to c56f665eeaf77cd19f0a09c8fa7628b744d5918d:

  netfilter: hold reference on module during netlink dump (2026-09-09 23:15:36 +0200)

----------------------------------------------------------------
netfilter pull request 26-09-10

----------------------------------------------------------------
Fernando Fernandez Mancera (1):
      netfilter: nf_tables: fix device name and prefix match in hook lookup

Julian Anastasov (1):
      ipvs: revalidate ihl before icmp_send

Pablo Neira Ayuso (4):
      netfilter: nf_nat: unregister and release hooks on error
      netfilter: flowtable: hold reference on ct until flow is released
      netfilter: xt_IDLETIMER: allocate timer with kzalloc()
      netfilter: hold reference on module during netlink dump

Theodor Arsenij Larionov Trichkine (1):
      netfilter: nft_nat: fully initialise new_addr in netmap setup

 net/netfilter/ipset/ip_set_core.c    |  1 +
 net/netfilter/ipvs/ip_vs_core.c      |  9 ++++++-
 net/netfilter/nf_conntrack_netlink.c |  7 ++++++
 net/netfilter/nf_flow_table_core.c   | 11 +++++++--
 net/netfilter/nf_nat_core.c          | 46 +++++++++++++++++++++++-------------
 net/netfilter/nf_tables_api.c        | 22 ++++++++++-------
 net/netfilter/nfnetlink_acct.c       |  1 +
 net/netfilter/nfnetlink_cthelper.c   |  1 +
 net/netfilter/nfnetlink_cttimeout.c  |  1 +
 net/netfilter/nft_nat.c              |  2 +-
 net/netfilter/xt_IDLETIMER.c         |  2 +-
 11 files changed, 73 insertions(+), 30 deletions(-)

[PATCH net 1/7] netfilter: nft_nat: fully initialise new_addr in netmap setup

[Pablo Neira Ayuso]

From: Theodor Arsenij Larionov Trichkine <theodorlarionov@gmail.com>

nft_nat_setup_netmap() builds the mapped address in an on-stack
union nf_inet_addr. For an IPv4 mapping it writes only the 4-byte .ip
member and the loop runs a single 32-bit iteration, but it then copies
the whole 16-byte union into range->min_addr and range->max_addr, so the
upper 12 bytes reach nf_nat_setup_info() uninitialised.

KMSAN reports an uninit-value in nf_nat_setup_info() reached from
nft_nat_eval(). The IPv6 path fills all 16 bytes and is not affected.

Zero-initialise new_addr.

Fixes: 3ff7ddb1353d ("netfilter: nft_nat: add netmap support")
Signed-off-by: Theodor Arsenij Larionov Trichkine <theodorlarionov@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nft_nat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nft_nat.c b/net/netfilter/nft_nat.c
index e32cd9fbc7c2..cdbd800cac96 100644
--- a/net/netfilter/nft_nat.c
+++ b/net/netfilter/nft_nat.c
@@ -64,8 +64,8 @@ static void nft_nat_setup_netmap(struct nf_nat_range2 *range,
 				 const struct nft_pktinfo *pkt,
 				 const struct nft_nat *priv)
 {
+	union nf_inet_addr new_addr = {};
 	struct sk_buff *skb = pkt->skb;
-	union nf_inet_addr new_addr;
 	__be32 netmask;
 	int i, len = 0;
 
-- 
2.47.3

[PATCH net 2/7] netfilter: nf_tables: fix device name and prefix match in hook lookup

[Pablo Neira Ayuso]

From: Fernando Fernandez Mancera <fmancera@suse.de>

Currently, a netdev chain or flowtable hooked to a device prefix can be
unintentionally deleted by a control-plane request targeting an exact
device name or even a shorter one due to the usage of min() to calculate
the length to match.

Fix this by making sure an exact device match never matches a prefix and
that both the target and the candidate have the same length during
delete operation. The add and update paths retain the existing overlap
matching to prevent a single device from matching multiple hooks.

Reported-by: Wei Fang <void0red@gmail.com>
Closes: https://lore.kernel.org/netfilter-devel/CANE+tVrDeNCHQVmsqkV2ozeBqyE3GtRDMhZgsg1bhw10yGNTRQ@mail.gmail.com/
Fixes: 6d07a289504a ("netfilter: nf_tables: Support wildcard netdev hook specs")
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 765a92fa90d6..3ab478ea7acb 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2440,11 +2440,14 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
 }
 
 static struct nft_hook *nft_hook_list_find(struct list_head *hook_list,
-					   const struct nft_hook *this)
+					   const struct nft_hook *this,
+					   bool strict)
 {
 	struct nft_hook *hook;
 
 	list_for_each_entry(hook, hook_list, list) {
+		if (strict && hook->ifnamelen != this->ifnamelen)
+			continue;
 		if (!strncmp(hook->ifname, this->ifname,
 			     min(hook->ifnamelen, this->ifnamelen))) {
 			if (hook->flags & NFT_HOOK_REMOVE)
@@ -2486,7 +2489,7 @@ static int nf_tables_parse_netdev_hooks(struct net *net,
 			err = PTR_ERR(hook);
 			goto err_hook;
 		}
-		if (nft_hook_list_find(hook_list, hook)) {
+		if (nft_hook_list_find(hook_list, hook, false)) {
 			NL_SET_BAD_ATTR(extack, tmp);
 			nft_netdev_hook_free(hook);
 			err = -EEXIST;
@@ -2943,7 +2946,7 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
 					ops->hook	= basechain->ops.hook;
 				}
 
-				if (nft_hook_list_find(&basechain->hook_list, h)) {
+				if (nft_hook_list_find(&basechain->hook_list, h, false)) {
 					list_del(&h->list);
 					nft_netdev_hook_free(h);
 					continue;
@@ -2956,7 +2959,8 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
 					    !nft_trans_chain_update(trans))
 						continue;
 
-					if (nft_hook_list_find(&nft_trans_chain_hooks(trans), h)) {
+					if (nft_hook_list_find(&nft_trans_chain_hooks(trans),
+							       h, false)) {
 						nft_chain_release_hook(&hook);
 						return -EEXIST;
 					}
@@ -3257,7 +3261,7 @@ static int nft_delchain_hook(struct nft_ctx *ctx,
 		return err;
 
 	list_for_each_entry(this, &chain_hook.list, list) {
-		hook = nft_hook_list_find(&basechain->hook_list, this);
+		hook = nft_hook_list_find(&basechain->hook_list, this, true);
 		if (!hook) {
 			err = -ENOENT;
 			goto err_chain_del_hook;
@@ -9053,7 +9057,7 @@ static int nft_register_flowtable_net_hooks(struct net *net,
 			if (!nft_is_active_next(net, ft))
 				continue;
 
-			if (nft_hook_list_find(&ft->hook_list, hook)) {
+			if (nft_hook_list_find(&ft->hook_list, hook, false)) {
 				err = -EEXIST;
 				goto err_unregister_net_hooks;
 			}
@@ -9130,7 +9134,7 @@ static int nft_flowtable_update(struct nft_ctx *ctx, const struct nlmsghdr *nlh,
 		return err;
 
 	list_for_each_entry_safe(hook, next, &flowtable_hook.list, list) {
-		if (nft_hook_list_find(&flowtable->hook_list, hook)) {
+		if (nft_hook_list_find(&flowtable->hook_list, hook, false)) {
 			list_del(&hook->list);
 			nft_netdev_hook_free(hook);
 			continue;
@@ -9143,7 +9147,7 @@ static int nft_flowtable_update(struct nft_ctx *ctx, const struct nlmsghdr *nlh,
 			    !nft_trans_flowtable_update(trans))
 				continue;
 
-			if (nft_hook_list_find(&nft_trans_flowtable_hooks(trans), hook)) {
+			if (nft_hook_list_find(&nft_trans_flowtable_hooks(trans), hook, false)) {
 				err = -EEXIST;
 				goto err_flowtable_update_hook;
 			}
@@ -9363,7 +9367,7 @@ static int nft_delflowtable_hook(struct nft_ctx *ctx,
 		return err;
 
 	list_for_each_entry(this, &flowtable_hook.list, list) {
-		hook = nft_hook_list_find(&flowtable->hook_list, this);
+		hook = nft_hook_list_find(&flowtable->hook_list, this, true);
 		if (!hook) {
 			err = -ENOENT;
 			goto err_flowtable_del_hook;
-- 
2.47.3

[PATCH net 3/7] netfilter: nf_nat: unregister and release hooks on error

[Pablo Neira Ayuso]

After the base nat hooks are registered, the first nested nat hook is
added through nf_hook_entries_insert_raw() and ->users is bumped.

However, sashiko reports that nf_hook_entries_insert_raw() fails, then
->users count remains at zero and nf_nat_unregister_fn() could hit
WARN_ON() since base nat hooks with no users is unexpected in the
unregistration path.

Postpone setting nat_proto_net->nat_hook_ops when the hooks are
registered to simplify the error path to decide whether the nat hooks
need unwinding.

Fixes: 1cd472bf036c ("netfilter: nf_nat: add nat hook register functions to nf_nat")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_nat_core.c | 46 ++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index 8ac326e1eb5b..a4858c2b2d65 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -1224,31 +1224,45 @@ int nf_nat_register_fn(struct net *net, u8 pf, const struct nf_hook_ops *ops,
 		}
 
 		ret = nf_register_net_hooks(net, nat_ops, ops_count);
-		if (ret < 0) {
-			mutex_unlock(&nf_nat_proto_mutex);
-			for (i = 0; i < ops_count; i++) {
-				priv = nat_ops[i].priv;
-				kfree_rcu(priv, rcu_head);
-			}
-			kfree_rcu(nat_ops, rcu);
-			return ret;
-		}
-
-		nat_proto_net->nat_hook_ops = nat_ops;
+		if (ret < 0)
+			goto err_free_hooks;
+	} else {
+		nat_ops = nat_proto_net->nat_hook_ops;
 	}
 
-	nat_ops = nat_proto_net->nat_hook_ops;
 	priv = nat_ops[hooknum].priv;
 	if (WARN_ON_ONCE(!priv)) {
-		mutex_unlock(&nf_nat_proto_mutex);
-		return -EOPNOTSUPP;
+		ret = -EOPNOTSUPP;
+		goto err_unregister_hooks;
 	}
 
 	ret = nf_hook_entries_insert_raw(&priv->entries, ops);
-	if (ret == 0)
-		nat_proto_net->users++;
+	if (ret)
+		goto err_unregister_hooks;
+
+	if (!nat_proto_net->nat_hook_ops)
+		nat_proto_net->nat_hook_ops = nat_ops;
+
+	nat_proto_net->users++;
 
 	mutex_unlock(&nf_nat_proto_mutex);
+
+	return 0;
+
+err_unregister_hooks:
+	if (nat_proto_net->nat_hook_ops) {
+		mutex_unlock(&nf_nat_proto_mutex);
+		return ret;
+	}
+	nf_unregister_net_hooks(net, nat_ops, ops_count);
+err_free_hooks:
+	mutex_unlock(&nf_nat_proto_mutex);
+	for (i = 0; i < ops_count; i++) {
+		priv = nat_ops[i].priv;
+		kfree_rcu(priv, rcu_head);
+	}
+	kfree_rcu(nat_ops, rcu);
+
 	return ret;
 }
 
-- 
2.47.3

[PATCH net 4/7] ipvs: revalidate ihl before icmp_send

[Pablo Neira Ayuso]

From: Julian Anastasov <ja@ssi.bg>

While the outer IP header is already pulled into the skb head,
we must be careful and revalidate the embedded headers after
reading them from the skb frags to prevent out-of-bounds
access.

One such place reported by Sashiko is ip_vs_in_icmp() where
local process can change the ihl field and after
pskb_may_pull() we can see larger value which is a problem
for the icmp_send() call.

Add check to drop the packet if the ihl field is changed and
make sure the transport header is updated.

Also, provide correct protocol to ipv4_update_pmtu().

Fixes: 7fcc2fe39fed ("net: icmp: avoid invalid transport header access in icmp_send tracepoint")
Fixes: f2edb9f7706d ("ipvs: implement passive PMTUD for IPIP packets")
Link: https://sashiko.dev/#/patchset/20260806105211.34622-1-ja%40ssi.bg
Signed-off-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/ipvs/ip_vs_core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index ba0957798bad..c41fac3ed873 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -1934,7 +1934,8 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
 			IP_VS_DBG(12, "ICMP for %s %pI4->%pI4: mtu=%u\n",
 				  outer_proto, &ip_hdr(skb)->saddr,
 				  &ip_hdr(skb)->daddr, mtu);
-			ipv4_update_pmtu(skb, ipvs->net, mtu, 0, 0);
+			ipv4_update_pmtu(skb, ipvs->net, mtu, 0,
+					 ip_hdr(skb)->protocol);
 			/* Client uses PMTUD? */
 			if (!(frag_off & htons(IP_DF)))
 				goto ignore_tunnel;
@@ -1960,6 +1961,12 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
 		/* Ensure the IP header is present in headroom */
 		if (!pskb_may_pull(skb, hlen_orig))
 			goto ignore_tunnel;
+		skb_set_transport_header(skb, hlen_orig);
+		/* Before now we may used ihl from skb frag, revalidate it after
+		 * copying it into skb head to prevent out-of-bounds access
+		 */
+		if (ip_hdr(skb)->ihl * 4 != hlen_orig)
+			goto ignore_tunnel;
 		IP_VS_DBG(12, "Sending ICMP for %pI4->%pI4: t=%u, c=%u, i=%u\n",
 			&ip_hdr(skb)->saddr, &ip_hdr(skb)->daddr,
 			type, code, ntohl(info));
-- 
2.47.3

[PATCH net 5/7] netfilter: flowtable: hold reference on ct until flow is released

[Pablo Neira Ayuso]

nf_ct_put() releases the ct->ext area inmediately, the rcu typesafe
semantics also allow to refer to the wrong conntrack from the flowtable
datapath. Hold reference on ct until flow is released after rcu grace
period.

Fixes: 0ff90b6c2034 ("netfilter: nf_flow_offload: fix use-after-free and a resource leak")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_core.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index 03241d4bfd5e..141502081784 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -258,6 +258,14 @@ static void flow_offload_route_release(struct flow_offload *flow)
 	nft_flow_dst_release(flow, FLOW_OFFLOAD_DIR_REPLY);
 }
 
+static void flow_offload_free_rcu(struct rcu_head *rcu_head)
+{
+	struct flow_offload *flow = container_of(rcu_head, struct flow_offload, rcu_head);
+
+	nf_ct_put(flow->ct);
+	kfree(flow);
+}
+
 void flow_offload_free(struct flow_offload *flow)
 {
 	switch (flow->type) {
@@ -267,8 +275,7 @@ void flow_offload_free(struct flow_offload *flow)
 	default:
 		break;
 	}
-	nf_ct_put(flow->ct);
-	kfree_rcu(flow, rcu_head);
+	call_rcu(&flow->rcu_head, flow_offload_free_rcu);
 }
 EXPORT_SYMBOL_GPL(flow_offload_free);
 
-- 
2.47.3

[PATCH net 6/7] netfilter: xt_IDLETIMER: allocate timer with kzalloc()

[Pablo Neira Ayuso]

According to sashiko:

"The embedded struct device_attribute attr has its show and mode fields
initialized later in this function, but its store function pointer
remains uninitialized heap garbage.

When the sysfs file is created, a process with the DAC_OVERRIDE
capability can bypass the 0444 mode restriction and write to the file.
This causes dev_attr_store() to execute the uninitialized store pointer,
leading to arbitrary kernel code execution."

Use kzalloc_obj() instead to address this issue.

Fixes: 68983a354a65 ("netfilter: xtables: Add snapshot of hardidletimer target")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_IDLETIMER.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/xt_IDLETIMER.c b/net/netfilter/xt_IDLETIMER.c
index fe7d8d19629b..8a1d95d21991 100644
--- a/net/netfilter/xt_IDLETIMER.c
+++ b/net/netfilter/xt_IDLETIMER.c
@@ -196,7 +196,7 @@ static int idletimer_tg_create_v1(struct idletimer_tg_info_v1 *info)
 {
 	int ret;
 
-	info->timer = kmalloc_obj(*info->timer);
+	info->timer = kzalloc_obj(*info->timer);
 	if (!info->timer) {
 		ret = -ENOMEM;
 		goto out;
-- 
2.47.3

[PATCH net 7/7] netfilter: hold reference on module during netlink dump

[Pablo Neira Ayuso]

Modules can be removed in the middle of a netlink dump. Set .module to
THIS_MODULE in struct netlink_dump_control to hold a reference on the
module while netlink dump is in progress.

See commit 6dc878a8ca39 ("netlink: add reference of module in
netlink_dump_start") which describes this problem.

Fixes: a7b4f989a629 ("netfilter: ipset: IP set core support")
Fixes: c1d10adb4a52 ("[NETFILTER]: Add ctnetlink port for nf_conntrack")
Fixes: 50978462300f ("netfilter: add cttimeout infrastructure for fine timeout tuning")
Fixes: 9413902796f5 ("netfilter: add extended accounting infrastructure over nfnetlink")
Fixes: 12f7a505331e ("netfilter: add user-space connection tracking helper infrastructure")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/ipset/ip_set_core.c    | 1 +
 net/netfilter/nf_conntrack_netlink.c | 7 +++++++
 net/netfilter/nfnetlink_acct.c       | 1 +
 net/netfilter/nfnetlink_cthelper.c   | 1 +
 net/netfilter/nfnetlink_cttimeout.c  | 1 +
 5 files changed, 11 insertions(+)

diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index 0a86a170ba90..624c5f14acf6 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -1733,6 +1733,7 @@ static int ip_set_dump(struct sk_buff *skb, const struct nfnl_info *info,
 			.start = ip_set_dump_start,
 			.dump = ip_set_dump_do,
 			.done = ip_set_dump_done,
+			.module = THIS_MODULE,
 		};
 		return netlink_dump_start(info->sk, skb, info->nlh, &c);
 	}
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 579ada063b1b..2340885b2076 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1682,6 +1682,7 @@ static int ctnetlink_get_conntrack(struct sk_buff *skb,
 			.start = ctnetlink_start,
 			.dump = ctnetlink_dump_table,
 			.done = ctnetlink_done,
+			.module = THIS_MODULE,
 			.data = (void *)cda,
 		};
 
@@ -1826,6 +1827,7 @@ static int ctnetlink_get_ct_dying(struct sk_buff *skb,
 	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {
 		struct netlink_dump_control c = {
 			.dump = ctnetlink_dump_dying,
+			.module = THIS_MODULE,
 		};
 		return netlink_dump_start(info->sk, skb, info->nlh, &c);
 	}
@@ -1840,6 +1842,7 @@ static int ctnetlink_get_ct_unconfirmed(struct sk_buff *skb,
 	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {
 		struct netlink_dump_control c = {
 			.dump = ctnetlink_dump_unconfirmed,
+			.module = THIS_MODULE,
 		};
 		return netlink_dump_start(info->sk, skb, info->nlh, &c);
 	}
@@ -2555,6 +2558,7 @@ static int ctnetlink_stat_ct_cpu(struct sk_buff *skb,
 	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {
 		struct netlink_dump_control c = {
 			.dump = ctnetlink_ct_stat_cpu_dump,
+			.module = THIS_MODULE,
 		};
 		return netlink_dump_start(info->sk, skb, info->nlh, &c);
 	}
@@ -3278,6 +3282,7 @@ static int ctnetlink_dump_exp_ct(struct net *net, struct sock *ctnl,
 		.dump = ctnetlink_exp_ct_dump_table,
 		.start = ctnetlink_dump_exp_ct_start,
 		.done = ctnetlink_dump_exp_ct_done,
+		.module = THIS_MODULE,
 	};
 
 	err = ctnetlink_parse_tuple(cda, &tuple, CTA_EXPECT_MASTER,
@@ -3327,6 +3332,7 @@ static int ctnetlink_get_expect(struct sk_buff *skb,
 		else {
 			struct netlink_dump_control c = {
 				.dump = ctnetlink_exp_dump_table,
+				.module = THIS_MODULE,
 			};
 			return netlink_dump_start(info->sk, skb, info->nlh, &c);
 		}
@@ -3748,6 +3754,7 @@ static int ctnetlink_stat_exp_cpu(struct sk_buff *skb,
 	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {
 		struct netlink_dump_control c = {
 			.dump = ctnetlink_exp_stat_cpu_dump,
+			.module = THIS_MODULE,
 		};
 		return netlink_dump_start(info->sk, skb, info->nlh, &c);
 	}
diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index 8ff1e0ad5cb0..39a62bb9a6e6 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -284,6 +284,7 @@ static int nfnl_acct_get(struct sk_buff *skb, const struct nfnl_info *info,
 			.dump = nfnl_acct_dump,
 			.start = nfnl_acct_start,
 			.done = nfnl_acct_done,
+			.module = THIS_MODULE,
 			.data = (void *)tb[NFACCT_FILTER],
 		};
 
diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index 5b6a09f46903..1331aba007ba 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -644,6 +644,7 @@ static int nfnl_cthelper_get(struct sk_buff *skb, const struct nfnl_info *info,
 	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {
 		struct netlink_dump_control c = {
 			.dump = nfnl_cthelper_dump_table,
+			.module = THIS_MODULE,
 		};
 		return netlink_dump_start(info->sk, skb, info->nlh, &c);
 	}
diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c
index 132c02ac7c4e..29de875d1713 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -277,6 +277,7 @@ static int cttimeout_get_timeout(struct sk_buff *skb,
 	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {
 		struct netlink_dump_control c = {
 			.dump = ctnl_timeout_dump,
+			.module = THIS_MODULE,
 		};
 		return netlink_dump_start(info->sk, skb, info->nlh, &c);
 	}
-- 
2.47.3

Re: [PATCH net 3/7] netfilter: nf_nat: unregister and release hooks on error

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: nf_nat: unregister and release hooks on error

This defers assigning nat_proto_net->nat_hook_ops until the nested nat hook
has actually been inserted, and adds an error path that unregisters the base
nat hooks and frees the nat_ops array plus the per-hook private data when
insertion fails.  The code change itself looks right and restores the
"nat_hook_ops != NULL implies users > 0" invariant.

The question below is only about the changelog wording.

> However, sashiko reports that nf_hook_entries_insert_raw() fails, then
> ->users count remains at zero and nf_nat_unregister_fn() could hit
> WARN_ON() since base nat hooks with no users is unexpected in the
> unregistration path.

[Severity: Medium]
Is this WARN_ON() actually reachable from the described sequence?

The WARN is the users == 0 check at the top of nf_nat_unregister_fn():

	mutex_lock(&nf_nat_proto_mutex);
	if (WARN_ON(nat_proto_net->users == 0))
		goto unlock;

Every in-tree caller seems to unregister only ops whose registration
returned 0.  ipt_nat_register_lookups() (and the ip6 counterpart) unwinds
with:

	ret = nf_nat_ipv4_register_fn(net, &ops[i]);
	if (ret) {
		while (i)
			nf_nat_ipv4_unregister_fn(net, &ops[--i]);

so the failing index is never unregistered, and for i == 0 nothing is
unregistered at all.

nf_nat_inet_register_fn() in nf_nat_proto.c unregisters only the IPv6 half
on IPv4 failure, and that half had returned 0, so users for IPv6 is at
least 1:

	ret = nf_nat_register_fn(net, NFPROTO_IPV4, ops, nf_nat_ipv4_ops,
				 ARRAY_SIZE(nf_nat_ipv4_ops));
	if (ret)
		nf_nat_unregister_fn(net, NFPROTO_IPV6, ops,
					ARRAY_SIZE(nf_nat_ipv6_ops));

For nft_chain_nat, nf_tables_addchain()'s err_register_hook path does
nft_chain_del() plus nft_trans_destroy() without calling
nf_tables_unregister_hook(), and nf_tables_table_enable() disables only the
chains that registered successfully.

The leftover state after the pre-patch failure also stays self-consistent:
nat_hook_ops is non-NULL with users == 0, so the next successful
nf_nat_register_fn() takes the existing-ops path, bumps users to 1 without
re-registering base hooks, and its matching unregister brings users back to
0 with exactly one nf_unregister_net_hooks().

> +err_unregister_hooks:
> +	if (nat_proto_net->nat_hook_ops) {
> +		mutex_unlock(&nf_nat_proto_mutex);
> +		return ret;
> +	}
> +	nf_unregister_net_hooks(net, nat_ops, ops_count);
> +err_free_hooks:
> +	mutex_unlock(&nf_nat_proto_mutex);
> +	for (i = 0; i < ops_count; i++) {
> +		priv = nat_ops[i].priv;
> +		kfree_rcu(priv, rcu_head);
> +	}
> +	kfree_rcu(nat_ops, rcu);

Isn't the failure this new path actually fixes a leak rather than a WARN?

On the first nat registration for a family in a netns, an
nf_hook_entries_insert_raw() failure (for example the allocation inside
nf_hook_entries_grow()) previously left the base nat hooks registered in
that netns forever, still traversing every packet, and leaked both the
kmemdup_array()'d nat_ops array and the four nf_nat_lookup_hook_priv
allocations.  Nothing reclaims them later, since nat_net_ops declares only
.id and .size:

	static struct pernet_operations nat_net_ops = {
		.id = &nat_net_id,
		.size = sizeof(struct nat_net),
	};

Could the changelog describe that signature instead, so anyone bisecting or
backporting looks for leaked hook registrations and memory rather than a
WARN splat?

Re: [PATCH net 4/7] ipvs: revalidate ihl before icmp_send

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ipvs: revalidate ihl before icmp_send

This adds a check in ip_vs_in_icmp() that drops the packet when the
embedded IP header's ihl no longer matches the length that was validated
from the frag copy, sets the transport header before icmp_send(), and
passes the tunnel protocol to ipv4_update_pmtu() instead of 0.

Link: https://sashiko.dev/#/patchset/20260806105211.34622-1-ja%40ssi.bg

A few questions below.

> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index ba0957798bad0..c41fac3ed873e 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -1934,7 +1934,8 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
>  			IP_VS_DBG(12, "ICMP for %s %pI4->%pI4: mtu=%u\n",
>  				  outer_proto, &ip_hdr(skb)->saddr,
>  				  &ip_hdr(skb)->daddr, mtu);
> -			ipv4_update_pmtu(skb, ipvs->net, mtu, 0, 0);
> +			ipv4_update_pmtu(skb, ipvs->net, mtu, 0,
> +					 ip_hdr(skb)->protocol);

[Severity: Medium]

The new protocol argument becomes fl4.flowi4_proto inside
ipv4_update_pmtu()->__build_flow_key(), and it is used both by
__ip_route_output_key() and by the fib_lookup() in
__ip_rt_update_pmtu().

The route IPVS actually transmits these tunnel packets on is looked up
and cached by do_output_route4(), which leaves flowi4_proto at zero:

net/netfilter/ipvs/ip_vs_xmit.c:do_output_route4() {
	memset(&fl4, 0, sizeof(fl4));
	fl4.daddr = daddr;
	fl4.flowi4_flags = (rt_mode & IP_VS_RT_MODE_KNOWN_NH) ?
			   FLOWI_FLAG_KNOWN_NH : 0;
	...
}

and fib4_rule_match() keys on that field:

net/ipv4/fib_rules.c:fib4_rule_match() {
	...
	if (rule->ip_proto && (rule->ip_proto != fl4->flowi4_proto))
		return 0;
	...
}

With an FRA_IP_PROTO rule configured, can the two lookups now land in
different tables, so the exception ends up on a nexthop IPVS never
consults?  The read-back immediately below would then still return the
unreduced MTU:

			if (dest) {
				struct ip_vs_dest_dst *dest_dst;

				dest_dst = rcu_dereference(dest->dest_dst);
				if (dest_dst)
					mtu = dst_mtu(dest_dst->dst_cache);
			}

and the ICMP relayed to the client would advertise the stale larger
value, leaving the passive PMTUD added by f2edb9f7706d ineffective for
such setups.

The multipath variant of this does not seem to apply, since
__ip_rt_update_pmtu() installs the exception on every path when
fib_info_num_path(res.fi) > 1.

Other in-tree callers of ipv4_update_pmtu() pass the real protocol, but
they also set flowi4_proto on their transmit lookup; IPVS does not.
Would it make sense to set flowi4_proto in do_output_route4() as well,
so the two keys agree?

[Severity: Medium]

Under the same assumption the commit message makes, that a local process
can mutate the bytes between the frag read and the pull, is the protocol
byte read here trusted?

Everything that authorized this tunnel was checked on the on-stack copy
taken earlier in ip_vs_in_icmp():

	cih = skb_header_pointer(skb, offset, sizeof(_ciph), &_ciph);
	if (!cih)
		return NF_ACCEPT;	/* The packet looks wrong, ignore */
	hlen_ipip = cih->ihl * 4;
	if (!(cih->version == 4 && hlen_ipip >= sizeof(struct iphdr)))
		return NF_ACCEPT;	/* The packet looks wrong, ignore */
	raddr = (union nf_inet_addr *)&cih->daddr;
	...
	if (cih->protocol == IPPROTO_IPIP) {
	...
	} else if ((cih->protocol == IPPROTO_UDP ||
		    cih->protocol == IPPROTO_GRE) &&

whereas ip_hdr(skb)->protocol here is re-read from the linear area after
pskb_may_pull(skb, hlen_ipip).  If those bytes can change, the value fed
to ipv4_update_pmtu(), and hence fl4.flowi4_proto for
__ip_route_output_key() and update_or_create_fnhe(), is not necessarily
the IPIP/UDP/GRE value that was validated, which works against the
"provide correct protocol to ipv4_update_pmtu()" goal.

Would saving cih->protocol into a local at validation time and passing
that local here be preferable to re-reading the header?

For what it is worth, the saddr/daddr/tos that __build_flow_key() reads
from skb->data at this call site have the same exposure, but that part
predates this patch.

>  			/* Client uses PMTUD? */
>  			if (!(frag_off & htons(IP_DF)))
>  				goto ignore_tunnel;
> @@ -1960,6 +1961,12 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
>  		/* Ensure the IP header is present in headroom */
>  		if (!pskb_may_pull(skb, hlen_orig))
>  			goto ignore_tunnel;
> +		skb_set_transport_header(skb, hlen_orig);
> +		/* Before now we may used ihl from skb frag, revalidate it after
> +		 * copying it into skb head to prevent out-of-bounds access
> +		 */
> +		if (ip_hdr(skb)->ihl * 4 != hlen_orig)
> +			goto ignore_tunnel;

[Severity: Medium]

Is the "out-of-bounds access" wording in this comment, and in the commit
message, accurate for the current tree?

The commit message says:

    "we must be careful and revalidate the embedded headers after
    reading them from the skb frags to prevent out-of-bounds
    access"

and

    "local process can change the ihl field and after
    pskb_may_pull() we can see larger value which is a problem
    for the icmp_send() call."

Every ihl-derived access reachable from this icmp_send() call looks
bounds-checked.  The tracepoint in include/trace/events/icmp.h reads the
quoted header through skb_header_pointer() and copes with a NULL return:

	if (iph->protocol == IPPROTO_UDP)
		uh = skb_header_pointer(skb,
					skb_network_offset(skb) +
					(iph->ihl << 2),
					sizeof(_uh), &_uh);

	if (!uh) {
	...

__icmp_send() does the same for its inner probe:

net/ipv4/icmp.c:__icmp_send() {
	...
		itp = skb_header_pointer(skb_in,
					 skb_network_header(skb_in) +
					 (iph->ihl << 2) + ...
		if (!itp)
			goto out;
	...
}

and __ip_options_echo() works on IPCB(skb)->opt, which is zeroed just
above the pull:

		memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));

so it returns immediately on optlen == 0.  With that, does a mutated ihl
today produce anything worse than wrong or zeroed tracepoint fields?

Related question on the Fixes tag:

    Fixes: 7fcc2fe39fed ("net: icmp: avoid invalid transport header access in icmp_send tracepoint")

That commit replaced the skb_transport_header() dereference in the
tracepoint with the bounds-checked skb_header_pointer() read shown above,
so it introduced the ihl-based offset but not an out-of-bounds access.
Since there is no remaining skb_transport_header() consumer in the
icmp_send() path, what does the added skb_set_transport_header(skb,
hlen_orig) feed?  Could the tag and the comment be reworded to describe
this as hardening plus a transport-header fixup, rather than a fix for an
out-of-bounds read?

>  		IP_VS_DBG(12, "Sending ICMP for %pI4->%pI4: t=%u, c=%u, i=%u\n",
>  			&ip_hdr(skb)->saddr, &ip_hdr(skb)->daddr,
>  			type, code, ntohl(info));

Re: [PATCH net 6/7] netfilter: xt_IDLETIMER: allocate timer with kzalloc()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: xt_IDLETIMER: allocate timer with kzalloc()

This switches the struct idletimer_tg allocation in
idletimer_tg_create_v1() from kmalloc_obj() to kzalloc_obj() so the
embedded struct device_attribute does not carry uninitialized function
pointers.  The v0 path already used kzalloc_obj(), so this aligns the
two.

> diff --git a/net/netfilter/xt_IDLETIMER.c b/net/netfilter/xt_IDLETIMER.c
> index fe7d8d19629b4..8a1d95d219917 100644
> --- a/net/netfilter/xt_IDLETIMER.c
> +++ b/net/netfilter/xt_IDLETIMER.c
> @@ -196,7 +196,7 @@ static int idletimer_tg_create_v1(struct idletimer_tg_info_v1 *info)
>  {
>  	int ret;
>  
> -	info->timer = kmalloc_obj(*info->timer);
> +	info->timer = kzalloc_obj(*info->timer);
>  	if (!info->timer) {
>  		ret = -ENOMEM;
>  		goto out;

[Severity: Medium]
The change itself looks right, and zeroing does remove the uninitialized
attr.store / attr.store_const pointers, but does the mechanism described in
the commit message actually work?

The message says:

  "When the sysfs file is created, a process with the DAC_OVERRIDE
  capability can bypass the 0444 mode restriction and write to the file.
  This causes dev_attr_store() to execute the uninitialized store pointer,
  leading to arbitrary kernel code execution."

sysfs creates its kernfs root with the extra open permission check:

fs/sysfs/mount.c:sysfs_init() {
	sysfs_root = kernfs_create_root(NULL, KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
					NULL);
	...
}

and kernfs_fop_open() rejects the write open before any ->store dispatch:

fs/kernfs/file.c:kernfs_fop_open() {
	...
	if (root->flags & KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK) {
		if ((file->f_mode & FMODE_WRITE) &&
		    (!(inode->i_mode & S_IWUGO) || !has_write))
			goto err_out;
	...
}

idletimer_tg_create_v1() sets attr.attr.mode = 0444, and
sysfs_add_file_mode_ns() installs that mode verbatim via mode & 0777, so
S_IWUGO is clear on the inode and the write open fails with -EACCES
regardless of CAP_DAC_OVERRIDE.

Reaching the garbage pointer seems to require a privileged chmod of the
sysfs file first (permitted by kernfs_iop_setattr()), which is a different
and much narrower trigger than the capability bypass described.

Could the commit message be reworded to describe the actual trigger, given
that the Fixes: tag and the "arbitrary kernel code execution" wording will
drive stable backports?

Re: [PATCH net 7/7] netfilter: hold reference on module during netlink dump

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: hold reference on module during netlink dump

This adds .module = THIS_MODULE to the struct netlink_dump_control
initializers in ip_set_core.c, nf_conntrack_netlink.c, nfnetlink_acct.c,
nfnetlink_cthelper.c and nfnetlink_cttimeout.c, with five Fixes: tags
referencing the original netfilter netlink dump implementations.

No Link: tags were present in the commit header.

> netfilter: hold reference on module during netlink dump
>
>     Modules can be removed in the middle of a netlink dump. Set .module to
>     THIS_MODULE in struct netlink_dump_control to hold a reference on the
>     module while netlink dump is in progress.
>
>     See commit 6dc878a8ca39 ("netlink: add reference of module in
>     netlink_dump_start") which describes this problem.
>
>     Fixes: a7b4f989a629 ("netfilter: ipset: IP set core support")
>     Fixes: c1d10adb4a52 ("[NETFILTER]: Add ctnetlink port for nf_conntrack")
>     Fixes: 50978462300f ("netfilter: add cttimeout infrastructure for fine timeout tuning")
>     Fixes: 9413902796f5 ("netfilter: add extended accounting infrastructure over nfnetlink")
>     Fixes: 12f7a505331e ("netfilter: add user-space connection tracking helper infrastructure")

[Severity: High]

Do any of these assignments change behaviour?  The wrapper in
include/linux/netlink.h already fills in .module when the caller left it
unset:

  static inline int netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
                                      const struct nlmsghdr *nlh,
                                      struct netlink_dump_control *control)
  {
          if (!control->module)
                  control->module = THIS_MODULE;

          return __netlink_dump_start(ssk, skb, nlh, control);
  }

Since this is a static inline in a header, THIS_MODULE expands in the
caller's translation unit, so it already resolved to ip_set.ko,
nf_conntrack_netlink.ko, nfnetlink_acct.ko, nfnetlink_cthelper.ko and
nfnetlink_cttimeout.ko respectively.

Every touched call site builds a local struct netlink_dump_control with
designated initializers (so .module was NULL on entry) and then calls the
inline netlink_dump_start(), never __netlink_dump_start() directly.  So
__netlink_dump_start() was already doing:

  /* add reference of module which cb->dump belongs to */
  if (!try_module_get(control->module)) {
          ret = -EPROTONOSUPPORT;
          goto error_unlock;
  }
  ...
  cb->module = control->module;

with the matching module_put() in netlink_dump(), in the error_put path and
in netlink_sock_destruct().

Given that, can a module actually be removed in the middle of one of these
dumps, as the changelog states?  If not, are the five Fixes: tags correct?
They point stable and CVE triage at a use-after-free in five netfilter
subsystems, and commit 6dc878a8ca39 cited in the changelog is what
introduced the implicit default precisely so that individual callers would
not need to set .module.

Would it make sense to either drop the patch, or drop the Fixes: tags and
describe it as an explicitness cleanup?  Alternatively, if there is a path
where the wrapper default does not apply (for example a dump callback living
in a different module than the caller), could the changelog name it?

On that last point, ip_set_dump() is the one site with a plausible
cross-module concern, but a reference on the ip_set core does not cover the
separate ip_set_hash_* / ip_set_list_* set type modules whose ->list()
callbacks run during the dump; those are pinned separately at set creation
time.  Does the change help there either?

> diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
> index 0a86a170ba906..624c5f14acf69 100644
> --- a/net/netfilter/ipset/ip_set_core.c
> +++ b/net/netfilter/ipset/ip_set_core.c
> @@ -1733,6 +1733,7 @@ static int ip_set_dump(struct sk_buff *skb, const struct nfnl_info *info,
>  			.start = ip_set_dump_start,
>  			.dump = ip_set_dump_do,
>  			.done = ip_set_dump_done,
> +			.module = THIS_MODULE,
>  		};
>  		return netlink_dump_start(info->sk, skb, info->nlh, &c);
>  	}
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 579ada063b1bb..2340885b2076d 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -1682,6 +1682,7 @@ static int ctnetlink_get_conntrack(struct sk_buff *skb,
>  			.start = ctnetlink_start,
>  			.dump = ctnetlink_dump_table,
>  			.done = ctnetlink_done,
> +			.module = THIS_MODULE,
>  			.data = (void *)cda,
>  		};
>  

[ ... remaining ctnetlink, nfnetlink_acct, nfnetlink_cthelper and
nfnetlink_cttimeout hunks snipped, the same question applies to each ... ]

Re: [PATCH net 0/7] Netfilter/IPVS fixes for net

[Jakub Kicinski]

On Thu, 10 Sep 2026 00:18:37 +0200 Pablo Neira Ayuso wrote:
> The following patchset provides fixes for Netfilter/IPVS:

Sorry for the delay, patchwork breakage on Sunday created a persistent
backlog for our CI, which will only fully clear on the weekend :/
The last commit seems unnecessary, if not more of them..

Re: [PATCH net 4/7] ipvs: revalidate ihl before icmp_send

[Julian Anastasov]

	Hello,

On Thu, 10 Sep 2026, Jakub Kicinski wrote:

> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> ipvs: revalidate ihl before icmp_send
> 
> This adds a check in ip_vs_in_icmp() that drops the packet when the
> embedded IP header's ihl no longer matches the length that was validated
> from the frag copy, sets the transport header before icmp_send(), and
> passes the tunnel protocol to ipv4_update_pmtu() instead of 0.
> 
> Link: https://sashiko.dev/#/patchset/20260806105211.34622-1-ja%40ssi.bg
> 
> A few questions below.

	I'll send v2 to address the problems, thanks!

> 
> > diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> > index ba0957798bad0..c41fac3ed873e 100644
> > --- a/net/netfilter/ipvs/ip_vs_core.c
> > +++ b/net/netfilter/ipvs/ip_vs_core.c
> > @@ -1934,7 +1934,8 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
> >  			IP_VS_DBG(12, "ICMP for %s %pI4->%pI4: mtu=%u\n",
> >  				  outer_proto, &ip_hdr(skb)->saddr,
> >  				  &ip_hdr(skb)->daddr, mtu);
> > -			ipv4_update_pmtu(skb, ipvs->net, mtu, 0, 0);
> > +			ipv4_update_pmtu(skb, ipvs->net, mtu, 0,
> > +					 ip_hdr(skb)->protocol);
> 
> [Severity: Medium]
> 
> The new protocol argument becomes fl4.flowi4_proto inside
> ipv4_update_pmtu()->__build_flow_key(), and it is used both by
> __ip_route_output_key() and by the fib_lookup() in
> __ip_rt_update_pmtu().
> 
> The route IPVS actually transmits these tunnel packets on is looked up
> and cached by do_output_route4(), which leaves flowi4_proto at zero:
> 
> net/netfilter/ipvs/ip_vs_xmit.c:do_output_route4() {
> 	memset(&fl4, 0, sizeof(fl4));
> 	fl4.daddr = daddr;
> 	fl4.flowi4_flags = (rt_mode & IP_VS_RT_MODE_KNOWN_NH) ?
> 			   FLOWI_FLAG_KNOWN_NH : 0;
> 	...
> }
> 
> and fib4_rule_match() keys on that field:
> 
> net/ipv4/fib_rules.c:fib4_rule_match() {
> 	...
> 	if (rule->ip_proto && (rule->ip_proto != fl4->flowi4_proto))
> 		return 0;
> 	...
> }
> 
> With an FRA_IP_PROTO rule configured, can the two lookups now land in
> different tables, so the exception ends up on a nexthop IPVS never
> consults?  The read-back immediately below would then still return the
> unreduced MTU:
> 
> 			if (dest) {
> 				struct ip_vs_dest_dst *dest_dst;
> 
> 				dest_dst = rcu_dereference(dest->dest_dst);
> 				if (dest_dst)
> 					mtu = dst_mtu(dest_dst->dst_cache);
> 			}
> 
> and the ICMP relayed to the client would advertise the stale larger
> value, leaving the passive PMTUD added by f2edb9f7706d ineffective for
> such setups.
> 
> The multipath variant of this does not seem to apply, since
> __ip_rt_update_pmtu() installs the exception on every path when
> fib_info_num_path(res.fi) > 1.
> 
> Other in-tree callers of ipv4_update_pmtu() pass the real protocol, but
> they also set flowi4_proto on their transmit lookup; IPVS does not.
> Would it make sense to set flowi4_proto in do_output_route4() as well,
> so the two keys agree?
> 
> [Severity: Medium]
> 
> Under the same assumption the commit message makes, that a local process
> can mutate the bytes between the frag read and the pull, is the protocol
> byte read here trusted?
> 
> Everything that authorized this tunnel was checked on the on-stack copy
> taken earlier in ip_vs_in_icmp():
> 
> 	cih = skb_header_pointer(skb, offset, sizeof(_ciph), &_ciph);
> 	if (!cih)
> 		return NF_ACCEPT;	/* The packet looks wrong, ignore */
> 	hlen_ipip = cih->ihl * 4;
> 	if (!(cih->version == 4 && hlen_ipip >= sizeof(struct iphdr)))
> 		return NF_ACCEPT;	/* The packet looks wrong, ignore */
> 	raddr = (union nf_inet_addr *)&cih->daddr;
> 	...
> 	if (cih->protocol == IPPROTO_IPIP) {
> 	...
> 	} else if ((cih->protocol == IPPROTO_UDP ||
> 		    cih->protocol == IPPROTO_GRE) &&
> 
> whereas ip_hdr(skb)->protocol here is re-read from the linear area after
> pskb_may_pull(skb, hlen_ipip).  If those bytes can change, the value fed
> to ipv4_update_pmtu(), and hence fl4.flowi4_proto for
> __ip_route_output_key() and update_or_create_fnhe(), is not necessarily
> the IPIP/UDP/GRE value that was validated, which works against the
> "provide correct protocol to ipv4_update_pmtu()" goal.
> 
> Would saving cih->protocol into a local at validation time and passing
> that local here be preferable to re-reading the header?
> 
> For what it is worth, the saddr/daddr/tos that __build_flow_key() reads
> from skb->data at this call site have the same exposure, but that part
> predates this patch.
> 
> >  			/* Client uses PMTUD? */
> >  			if (!(frag_off & htons(IP_DF)))
> >  				goto ignore_tunnel;
> > @@ -1960,6 +1961,12 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
> >  		/* Ensure the IP header is present in headroom */
> >  		if (!pskb_may_pull(skb, hlen_orig))
> >  			goto ignore_tunnel;
> > +		skb_set_transport_header(skb, hlen_orig);
> > +		/* Before now we may used ihl from skb frag, revalidate it after
> > +		 * copying it into skb head to prevent out-of-bounds access
> > +		 */
> > +		if (ip_hdr(skb)->ihl * 4 != hlen_orig)
> > +			goto ignore_tunnel;
> 
> [Severity: Medium]
> 
> Is the "out-of-bounds access" wording in this comment, and in the commit
> message, accurate for the current tree?
> 
> The commit message says:
> 
>     "we must be careful and revalidate the embedded headers after
>     reading them from the skb frags to prevent out-of-bounds
>     access"
> 
> and
> 
>     "local process can change the ihl field and after
>     pskb_may_pull() we can see larger value which is a problem
>     for the icmp_send() call."
> 
> Every ihl-derived access reachable from this icmp_send() call looks
> bounds-checked.  The tracepoint in include/trace/events/icmp.h reads the
> quoted header through skb_header_pointer() and copes with a NULL return:
> 
> 	if (iph->protocol == IPPROTO_UDP)
> 		uh = skb_header_pointer(skb,
> 					skb_network_offset(skb) +
> 					(iph->ihl << 2),
> 					sizeof(_uh), &_uh);
> 
> 	if (!uh) {
> 	...
> 
> __icmp_send() does the same for its inner probe:
> 
> net/ipv4/icmp.c:__icmp_send() {
> 	...
> 		itp = skb_header_pointer(skb_in,
> 					 skb_network_header(skb_in) +
> 					 (iph->ihl << 2) + ...
> 		if (!itp)
> 			goto out;
> 	...
> }
> 
> and __ip_options_echo() works on IPCB(skb)->opt, which is zeroed just
> above the pull:
> 
> 		memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
> 
> so it returns immediately on optlen == 0.  With that, does a mutated ihl
> today produce anything worse than wrong or zeroed tracepoint fields?
> 
> Related question on the Fixes tag:
> 
>     Fixes: 7fcc2fe39fed ("net: icmp: avoid invalid transport header access in icmp_send tracepoint")
> 
> That commit replaced the skb_transport_header() dereference in the
> tracepoint with the bounds-checked skb_header_pointer() read shown above,
> so it introduced the ihl-based offset but not an out-of-bounds access.
> Since there is no remaining skb_transport_header() consumer in the
> icmp_send() path, what does the added skb_set_transport_header(skb,
> hlen_orig) feed?  Could the tag and the comment be reworded to describe
> this as hardening plus a transport-header fixup, rather than a fix for an
> out-of-bounds read?
> 
> >  		IP_VS_DBG(12, "Sending ICMP for %pI4->%pI4: t=%u, c=%u, i=%u\n",
> >  			&ip_hdr(skb)->saddr, &ip_hdr(skb)->daddr,
> >  			type, code, ntohl(info));
> 

Regards

--
Julian Anastasov <ja@ssi.bg>