| Message ID | 20241003183053.8555-1-fw@strlen.de |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [nf] netfilter: xt_cluster: restrict to ip/ip6tables | expand |
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);
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.
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 > >
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.
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);