[PATCH nf] netfilter: xt_cluster: restrict to ip/ip6tables

[PATCH nf] netfilter: xt_cluster: restrict to ip/ip6tables

[Florian Westphal]

Restrict this match to iptables/ip6tables.
syzbot managed to call it via ebtables:

 WARNING: CPU: 0 PID: 11 at net/netfilter/xt_cluster.c:72 xt_cluster_mt+0x196/0x780
 [..]
 ebt_do_table+0x174b/0x2a40

Module registers to NFPROTO_UNSPEC, but it assumes ipv4/ipv6 packet
processing.  As this is only useful to restrict locally terminating
TCP/UDP traffic, reject non-ip families at rule load time.

Reported-by: syzbot+256c348558aa5cf611a9@syzkaller.appspotmail.com
Tested-by: syzbot+256c348558aa5cf611a9@syzkaller.appspotmail.com
Fixes: 0269ea493734 ("netfilter: xtables: add cluster match")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/xt_cluster.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/netfilter/xt_cluster.c b/net/netfilter/xt_cluster.c
index a047a545371e..fa45af1c48a9 100644
--- a/net/netfilter/xt_cluster.c
+++ b/net/netfilter/xt_cluster.c
@@ -124,6 +124,14 @@ static int xt_cluster_mt_checkentry(const struct xt_mtchk_param *par)
 	struct xt_cluster_match_info *info = par->matchinfo;
 	int ret;
 
+	switch (par->family) {
+	case NFPROTO_IPV4:
+	case NFPROTO_IPV6:
+		break;
+	default:
+		return -EAFNOSUPPORT;
+	}
+
 	if (info->total_nodes > XT_CLUSTER_NODES_MAX) {
 		pr_info_ratelimited("you have exceeded the maximum number of cluster nodes (%u > %u)\n",
 				    info->total_nodes, XT_CLUSTER_NODES_MAX);
-- 
2.45.2

Re: [PATCH nf] netfilter: xt_cluster: enable ebtables operation?

[Jan Engelhardt]

On Thursday 2024-10-03 20:30, Florian Westphal wrote:
>
>Module registers to NFPROTO_UNSPEC, but it assumes ipv4/ipv6 packet
>processing.  As this is only useful to restrict locally terminating
>TCP/UDP traffic, reject non-ip families at rule load time.
>
>@@ -124,6 +124,14 @@ static int xt_cluster_mt_checkentry(const struct xt_mtchk_param *par)
> 	struct xt_cluster_match_info *info = par->matchinfo;
> 	int ret;
> 
>+	switch (par->family) {
>+	case NFPROTO_IPV4:
>+	case NFPROTO_IPV6:
>+		break;
>+	default:
>+		return -EAFNOSUPPORT;
>+	}

I wonder if we could just implement the logic for it.
Like this patch [untested!]:


From d534984879b9b3c4b8cf536cad1044c29b843a2d Mon Sep 17 00:00:00 2001
From: Jan Engelhardt <jengelh@inai.de>
Date: Thu, 3 Oct 2024 20:49:02 +0200
Subject: [PATCH] xt_cluster: add logic for use from NFPROTO_BRIDGE

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 net/netfilter/xt_cluster.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/netfilter/xt_cluster.c b/net/netfilter/xt_cluster.c
index a047a545371e..cf4a74d68577 100644
--- a/net/netfilter/xt_cluster.c
+++ b/net/netfilter/xt_cluster.c
@@ -68,6 +68,9 @@ xt_cluster_is_multicast_addr(const struct sk_buff *skb, u_int8_t family)
 	case NFPROTO_IPV6:
 		is_multicast = ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr);
 		break;
+	case NFPROTO_BRIDGE:
+		is_multicast = is_multicast_ether_addr(eth_hdr(skb)->h_dest);
+		break;
 	default:
 		WARN_ON(1);
 		break;
@@ -124,6 +127,15 @@ static int xt_cluster_mt_checkentry(const struct xt_mtchk_param *par)
 	struct xt_cluster_match_info *info = par->matchinfo;
 	int ret;
 
+	switch (par->family) {
+	case NFPROTO_IPV4:
+	case NFPROTO_IPV6:
+	case NFPROTO_BRIDGE:
+		break;
+	default:
+		return -EAFNOSUPPORT;
+	}
+
 	if (info->total_nodes > XT_CLUSTER_NODES_MAX) {
 		pr_info_ratelimited("you have exceeded the maximum number of cluster nodes (%u > %u)\n",
 				    info->total_nodes, XT_CLUSTER_NODES_MAX);
-- 
2.46.1

Re: [PATCH nf] netfilter: xt_cluster: enable ebtables operation?

[Florian Westphal]

Jan Engelhardt <ej@inai.de> wrote:
> >Module registers to NFPROTO_UNSPEC, but it assumes ipv4/ipv6 packet
> >processing.  As this is only useful to restrict locally terminating
                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >TCP/UDP traffic, reject non-ip families at rule load time.

> >@@ -124,6 +124,14 @@ static int xt_cluster_mt_checkentry(const struct xt_mtchk_param *par)
> > 	struct xt_cluster_match_info *info = par->matchinfo;
> > 	int ret;
> > 
> >+	switch (par->family) {
> >+	case NFPROTO_IPV4:
> >+	case NFPROTO_IPV6:
> >+		break;
> >+	default:
> >+		return -EAFNOSUPPORT;
> >+	}
> 
> I wonder if we could just implement the logic for it.

Whats the use case?

> Like this patch [untested!]:
> 
> From d534984879b9b3c4b8cf536cad1044c29b843a2d Mon Sep 17 00:00:00 2001
> From: Jan Engelhardt <jengelh@inai.de>
> Date: Thu, 3 Oct 2024 20:49:02 +0200
> Subject: [PATCH] xt_cluster: add logic for use from NFPROTO_BRIDGE
> 
> Signed-off-by: Jan Engelhardt <jengelh@inai.de>
> ---
>  net/netfilter/xt_cluster.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/net/netfilter/xt_cluster.c b/net/netfilter/xt_cluster.c
> index a047a545371e..cf4a74d68577 100644
> --- a/net/netfilter/xt_cluster.c
> +++ b/net/netfilter/xt_cluster.c
> @@ -68,6 +68,9 @@ xt_cluster_is_multicast_addr(const struct sk_buff *skb, u_int8_t family)
>  	case NFPROTO_IPV6:
>  		is_multicast = ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr);
>  		break;
> +	case NFPROTO_BRIDGE:
> +		is_multicast = is_multicast_ether_addr(eth_hdr(skb)->h_dest);
> +		break;

AFAIU this is always true: l2 address is always a multicast mac in
xt_cluster setups, we would need to peek into the L3 address to see if
its also multicast or if its the expected l3-unicast-in-l2-mcast.

I don't see a use case for supporting this from a bridge, but maybe I
missed something.

Re: [PATCH nf] netfilter: xt_cluster: restrict to ip/ip6tables

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]

On Thu, Oct 03, 2024 at 08:30:46PM +0200, Florian Westphal wrote:
> Restrict this match to iptables/ip6tables.
> syzbot managed to call it via ebtables:
> 
>  WARNING: CPU: 0 PID: 11 at net/netfilter/xt_cluster.c:72 xt_cluster_mt+0x196/0x780
>  [..]
>  ebt_do_table+0x174b/0x2a40
> 
> Module registers to NFPROTO_UNSPEC, but it assumes ipv4/ipv6 packet
> processing.  As this is only useful to restrict locally terminating
> TCP/UDP traffic, reject non-ip families at rule load time.

Fine with me. I had a similar patch looking like this.

This was never intented to be used by ebtables.

> Reported-by: syzbot+256c348558aa5cf611a9@syzkaller.appspotmail.com
> Tested-by: syzbot+256c348558aa5cf611a9@syzkaller.appspotmail.com
> Fixes: 0269ea493734 ("netfilter: xtables: add cluster match")
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/netfilter/xt_cluster.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/netfilter/xt_cluster.c b/net/netfilter/xt_cluster.c
> index a047a545371e..fa45af1c48a9 100644
> --- a/net/netfilter/xt_cluster.c
> +++ b/net/netfilter/xt_cluster.c
> @@ -124,6 +124,14 @@ static int xt_cluster_mt_checkentry(const struct xt_mtchk_param *par)
>  	struct xt_cluster_match_info *info = par->matchinfo;
>  	int ret;
>  
> +	switch (par->family) {
> +	case NFPROTO_IPV4:
> +	case NFPROTO_IPV6:
> +		break;
> +	default:
> +		return -EAFNOSUPPORT;
> +	}
> +
>  	if (info->total_nodes > XT_CLUSTER_NODES_MAX) {
>  		pr_info_ratelimited("you have exceeded the maximum number of cluster nodes (%u > %u)\n",
>  				    info->total_nodes, XT_CLUSTER_NODES_MAX);
> -- 
> 2.45.2
> 
> 

[-- Attachment #2: 0001-netfilter-xt_cluster-restrict-it-to-NFPROTO_IPV4-and.patch --]
[-- Type: text/x-diff, Size: 4139 bytes --]

From b41722e5a41b953702cf378f184f191049dba790 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Fri, 4 Oct 2024 10:34:10 +0200
Subject: [PATCH] netfilter: xt_cluster: restrict it to NFPROTO_IPV4 and
 NFPROTO_IPV6

This match was designed to run on iptables and ip6tables only.

------------[ cut here ]------------
WARNING: CPU: 0 PID: 11 at net/netfilter/xt_cluster.c:72 xt_cluster_is_multicast_addr net/netfilter/xt_cluster.c:72 [inline]
WARNING: CPU: 0 PID: 11 at net/netfilter/xt_cluster.c:72 xt_cluster_mt+0x196/0x780 net/netfilter/xt_cluster.c:104
Modules linked in:
CPU: 0 UID: 0 PID: 11 Comm: kworker/u8:0 Not tainted 6.11.0-rc7-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/06/2024
Workqueue: ipv6_addrconf addrconf_dad_work
RIP: 0010:xt_cluster_is_multicast_addr net/netfilter/xt_cluster.c:72 [inline]
RIP: 0010:xt_cluster_mt+0x196/0x780 net/netfilter/xt_cluster.c:104
Code: f0 00 00 00 23 2b bf e0 00 00 00 89 ee e8 32 ee a1 f7 81 fd e0 00 00 00 75 1c e8 e5 e9 a1 f7 e9 83 00 00 00 e8 db e9 a1 f7 90 <0f>
+0b 90 eb 0c e8 d0 e9 a1 f7 eb 05 e8 c9 e9 a1 f7 4d 8d af 80 00
RSP: 0018:ffffc90000006c88 EFLAGS: 00010246
RAX: ffffffff89f1a2d5 RBX: 0000000000000007 RCX: ffff88801ced3c00
RDX: 0000000000000100 RSI: ffffffff8fd2a440 RDI: 0000000000000007
RBP: ffffc90000006e68 R08: 0000000000000001 R09: ffffffff89f1a1c4
R10: 0000000000000002 R11: ffff88801ced3c00 R12: dffffc0000000000
R13: 1ffff92000159c18 R14: ffffc90000ace140 R15: ffff8880251bf280
FS:  0000000000000000(0000) GS:ffff8880b8800000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007efc6d6b6440 CR3: 000000000e734000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <IRQ>
 ebt_do_match net/bridge/netfilter/ebtables.c:109 [inline]
 ebt_do_table+0x174b/0x2a40 net/bridge/netfilter/ebtables.c:230
 nf_hook_entry_hookfn include/linux/netfilter.h:154 [inline]
 nf_hook_slow+0xc3/0x220 net/netfilter/core.c:626
 nf_hook include/linux/netfilter.h:269 [inline]
 NF_HOOK+0x2a7/0x460 include/linux/netfilter.h:312

Fixes: 0269ea493734 ("netfilter: xtables: add cluster match")
Reported-by: syzbot+256c348558aa5cf611a9@syzkaller.appspotmail.com
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_cluster.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/net/netfilter/xt_cluster.c b/net/netfilter/xt_cluster.c
index a047a545371e..908fd5f2c3c8 100644
--- a/net/netfilter/xt_cluster.c
+++ b/net/netfilter/xt_cluster.c
@@ -146,24 +146,37 @@ static void xt_cluster_mt_destroy(const struct xt_mtdtor_param *par)
 	nf_ct_netns_put(par->net, par->family);
 }
 
-static struct xt_match xt_cluster_match __read_mostly = {
-	.name		= "cluster",
-	.family		= NFPROTO_UNSPEC,
-	.match		= xt_cluster_mt,
-	.checkentry	= xt_cluster_mt_checkentry,
-	.matchsize	= sizeof(struct xt_cluster_match_info),
-	.destroy	= xt_cluster_mt_destroy,
-	.me		= THIS_MODULE,
+static struct xt_match xt_cluster_match[] __read_mostly = {
+	{
+		.name		= "cluster",
+		.family		= NFPROTO_IPV4,
+		.match		= xt_cluster_mt,
+		.checkentry	= xt_cluster_mt_checkentry,
+		.matchsize	= sizeof(struct xt_cluster_match_info),
+		.destroy	= xt_cluster_mt_destroy,
+		.me		= THIS_MODULE,
+	},
+#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
+	{
+		.name		= "cluster",
+		.family		= NFPROTO_IPV6,
+		.match		= xt_cluster_mt,
+		.checkentry	= xt_cluster_mt_checkentry,
+		.matchsize	= sizeof(struct xt_cluster_match_info),
+		.destroy	= xt_cluster_mt_destroy,
+		.me		= THIS_MODULE,
+	},
+#endif
 };
 
 static int __init xt_cluster_mt_init(void)
 {
-	return xt_register_match(&xt_cluster_match);
+	return xt_register_matches(xt_cluster_match, ARRAY_SIZE(xt_cluster_match));
 }
 
 static void __exit xt_cluster_mt_fini(void)
 {
-	xt_unregister_match(&xt_cluster_match);
+	xt_unregister_matches(xt_cluster_match, ARRAY_SIZE(xt_cluster_match));
 }
 
 MODULE_AUTHOR("Pablo Neira Ayuso <pablo@netfilter.org>");
-- 
2.30.2

Re: [PATCH nf] netfilter: xt_cluster: enable ebtables operation?

[Pablo Neira Ayuso]

On Thu, Oct 03, 2024 at 08:50:12PM +0200, Jan Engelhardt wrote:
> 
> On Thursday 2024-10-03 20:30, Florian Westphal wrote:
> >
> >Module registers to NFPROTO_UNSPEC, but it assumes ipv4/ipv6 packet
> >processing.  As this is only useful to restrict locally terminating
> >TCP/UDP traffic, reject non-ip families at rule load time.
> >
> >@@ -124,6 +124,14 @@ static int xt_cluster_mt_checkentry(const struct xt_mtchk_param *par)
> > 	struct xt_cluster_match_info *info = par->matchinfo;
> > 	int ret;
> > 
> >+	switch (par->family) {
> >+	case NFPROTO_IPV4:
> >+	case NFPROTO_IPV6:
> >+		break;
> >+	default:
> >+		return -EAFNOSUPPORT;
> >+	}
> 
> I wonder if we could just implement the logic for it.
> Like this patch [untested!]:

Thanks, I considered this too, I don't think it is worth to support
this for ebtables, I don't have a use case for this.