diff mbox series

[nf-next,2/6] netfilter: nf_tables: Reject tables of unsupported family

Message ID 20220315091513.66544-3-pablo@netfilter.org (mailing list archive)
State Accepted
Commit f1082dd31fe461d482d69da2a8eccfeb7bf07ac2
Delegated to: Netdev Maintainers
Headers show
Series [nf-next,1/6] Revert "netfilter: conntrack: mark UDP zero checksum as CHECKSUM_UNNECESSARY" | expand

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Pull request is its own cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/cc_maintainers warning 3 maintainers not CCed: kadlec@netfilter.org coreteam@netfilter.org fw@strlen.de
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 16 this patch: 16
netdev/checkpatch warning CHECK: Logical continuations should be on the previous line
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Pablo Neira Ayuso March 15, 2022, 9:15 a.m. UTC 
From: Phil Sutter <phil@nwl.cc>

An nftables family is merely a hollow container, its family just a
number and such not reliant on compile-time options other than nftables
support itself. Add an artificial check so attempts at using a family
the kernel can't support fail as early as possible. This helps user
space detect kernels which lack e.g. NFPROTO_INET.

Signed-off-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Jakub Kicinski March 15, 2022, 6:56 p.m. UTC | #1 
On Tue, 15 Mar 2022 10:15:09 +0100 Pablo Neira Ayuso wrote:
> +	return false
> +#ifdef CONFIG_NF_TABLES_INET
> +		|| family == NFPROTO_INET
> +#endif
> +#ifdef CONFIG_NF_TABLES_IPV4
> +		|| family == NFPROTO_IPV4
> +#endif
> +#ifdef CONFIG_NF_TABLES_ARP
> +		|| family == NFPROTO_ARP
> +#endif
> +#ifdef CONFIG_NF_TABLES_NETDEV
> +		|| family == NFPROTO_NETDEV
> +#endif
> +#if IS_ENABLED(CONFIG_NF_TABLES_BRIDGE)

is there a reason this one is IS_ENABLED() and everything else is ifdef?

> +		|| family == NFPROTO_BRIDGE
> +#endif
> +#ifdef CONFIG_NF_TABLES_IPV6
> +		|| family == NFPROTO_IPV6
> +#endif
> +		;

	return (IS_ENABLED(CONFIG_NF_TABLES_INET) && family == NFPROTO_INET)) ||
	       (IS_ENABLED(CONFIG_NF_TABLES_IPV4) && family == NFPROTO_IPV4)) ||
		...

would have also been an option, for future reference.
Phil Sutter March 15, 2022, 8:05 p.m. UTC | #2 
Hi Jakub,

On Tue, Mar 15, 2022 at 11:56:44AM -0700, Jakub Kicinski wrote:
> On Tue, 15 Mar 2022 10:15:09 +0100 Pablo Neira Ayuso wrote:
> > +	return false
> > +#ifdef CONFIG_NF_TABLES_INET
> > +		|| family == NFPROTO_INET
> > +#endif
> > +#ifdef CONFIG_NF_TABLES_IPV4
> > +		|| family == NFPROTO_IPV4
> > +#endif
> > +#ifdef CONFIG_NF_TABLES_ARP
> > +		|| family == NFPROTO_ARP
> > +#endif
> > +#ifdef CONFIG_NF_TABLES_NETDEV
> > +		|| family == NFPROTO_NETDEV
> > +#endif
> > +#if IS_ENABLED(CONFIG_NF_TABLES_BRIDGE)
> 
> is there a reason this one is IS_ENABLED() and everything else is ifdef?

I based my patch on the existing ifdefs in nft_chain_filter.c where
these config symbols are checked exactly like above. Looking at git
history, the check was changed from a simple ifdef in commit
dfee0e99bcff7 ("netfilter: bridge: make NF_TABLES_BRIDGE tristate").

> > +		|| family == NFPROTO_BRIDGE
> > +#endif
> > +#ifdef CONFIG_NF_TABLES_IPV6
> > +		|| family == NFPROTO_IPV6
> > +#endif
> > +		;
> 
> 	return (IS_ENABLED(CONFIG_NF_TABLES_INET) && family == NFPROTO_INET)) ||
> 	       (IS_ENABLED(CONFIG_NF_TABLES_IPV4) && family == NFPROTO_IPV4)) ||
> 		...
> 
> would have also been an option, for future reference.

Yes, that is indeed much cleaner. I wasn't aware of this possibility
using IS_ENABLED. What do you think, worth a follow-up?

Thanks, Phil
Pablo Neira Ayuso March 15, 2022, 8:10 p.m. UTC | #3 
On Tue, Mar 15, 2022 at 09:05:53PM +0100, Phil Sutter wrote:
> Hi Jakub,
> 
> On Tue, Mar 15, 2022 at 11:56:44AM -0700, Jakub Kicinski wrote:
> > On Tue, 15 Mar 2022 10:15:09 +0100 Pablo Neira Ayuso wrote:
> > > +	return false
> > > +#ifdef CONFIG_NF_TABLES_INET
> > > +		|| family == NFPROTO_INET
> > > +#endif
> > > +#ifdef CONFIG_NF_TABLES_IPV4
> > > +		|| family == NFPROTO_IPV4
> > > +#endif
> > > +#ifdef CONFIG_NF_TABLES_ARP
> > > +		|| family == NFPROTO_ARP
> > > +#endif
> > > +#ifdef CONFIG_NF_TABLES_NETDEV
> > > +		|| family == NFPROTO_NETDEV
> > > +#endif
> > > +#if IS_ENABLED(CONFIG_NF_TABLES_BRIDGE)
> > 
> > is there a reason this one is IS_ENABLED() and everything else is ifdef?
> 
> I based my patch on the existing ifdefs in nft_chain_filter.c where
> these config symbols are checked exactly like above. Looking at git
> history, the check was changed from a simple ifdef in commit
> dfee0e99bcff7 ("netfilter: bridge: make NF_TABLES_BRIDGE tristate").
> 
> > > +		|| family == NFPROTO_BRIDGE
> > > +#endif
> > > +#ifdef CONFIG_NF_TABLES_IPV6
> > > +		|| family == NFPROTO_IPV6
> > > +#endif
> > > +		;
> > 
> > 	return (IS_ENABLED(CONFIG_NF_TABLES_INET) && family == NFPROTO_INET)) ||
> > 	       (IS_ENABLED(CONFIG_NF_TABLES_IPV4) && family == NFPROTO_IPV4)) ||
> > 		...
> > 
> > would have also been an option, for future reference.
> 
> Yes, that is indeed much cleaner. I wasn't aware of this possibility
> using IS_ENABLED. What do you think, worth a follow-up?

CONFIG_NF_TABLES_INET and CONFIG_NF_TABLES_IPV4 are never modules, I
think IS_ENABLED is misleading there to the reader.
Pablo Neira Ayuso March 15, 2022, 8:27 p.m. UTC | #4 
On Tue, Mar 15, 2022 at 11:56:44AM -0700, Jakub Kicinski wrote:
> On Tue, 15 Mar 2022 10:15:09 +0100 Pablo Neira Ayuso wrote:
> > +	return false
> > +#ifdef CONFIG_NF_TABLES_INET
> > +		|| family == NFPROTO_INET
> > +#endif
> > +#ifdef CONFIG_NF_TABLES_IPV4
> > +		|| family == NFPROTO_IPV4
> > +#endif
> > +#ifdef CONFIG_NF_TABLES_ARP
> > +		|| family == NFPROTO_ARP
> > +#endif
> > +#ifdef CONFIG_NF_TABLES_NETDEV
> > +		|| family == NFPROTO_NETDEV
> > +#endif
> > +#if IS_ENABLED(CONFIG_NF_TABLES_BRIDGE)
> 
> is there a reason this one is IS_ENABLED() and everything else is ifdef?

bridge might be compiled as a module, if the bridge infrastructure
also comes a module as well.

Anything else is either built-in or off.
Jakub Kicinski March 15, 2022, 9:25 p.m. UTC | #5 
On Tue, 15 Mar 2022 21:10:33 +0100 Pablo Neira Ayuso wrote:
> > > 	return (IS_ENABLED(CONFIG_NF_TABLES_INET) && family == NFPROTO_INET)) ||
> > > 	       (IS_ENABLED(CONFIG_NF_TABLES_IPV4) && family == NFPROTO_IPV4)) ||
> > > 		...
> > > 
> > > would have also been an option, for future reference.  
> > 
> > Yes, that is indeed much cleaner. I wasn't aware of this possibility
> > using IS_ENABLED. What do you think, worth a follow-up?  
> 
> CONFIG_NF_TABLES_INET and CONFIG_NF_TABLES_IPV4 are never modules, I
> think IS_ENABLED is misleading there to the reader.

It's not about being a module, IS_ENABLED() is usable in C code, 
no need to use the pre-processor. But your call, obviously.
Jakub Kicinski March 15, 2022, 9:27 p.m. UTC | #6 
On Tue, 15 Mar 2022 21:27:45 +0100 Pablo Neira Ayuso wrote:
> > is there a reason this one is IS_ENABLED() and everything else is ifdef?  
> 
> bridge might be compiled as a module, if the bridge infrastructure
> also comes a module as well.
> 
> Anything else is either built-in or off.

:o I thought ifdef works for modules, after checking the code 
it makes sense, thanks!
diff mbox series

Patch

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 9cd1d7a62804..3168ad8cffd1 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1072,6 +1072,30 @@  static int nft_objname_hash_cmp(struct rhashtable_compare_arg *arg,
 	return strcmp(obj->key.name, k->name);
 }
 
+static bool nft_supported_family(u8 family)
+{
+	return false
+#ifdef CONFIG_NF_TABLES_INET
+		|| family == NFPROTO_INET
+#endif
+#ifdef CONFIG_NF_TABLES_IPV4
+		|| family == NFPROTO_IPV4
+#endif
+#ifdef CONFIG_NF_TABLES_ARP
+		|| family == NFPROTO_ARP
+#endif
+#ifdef CONFIG_NF_TABLES_NETDEV
+		|| family == NFPROTO_NETDEV
+#endif
+#if IS_ENABLED(CONFIG_NF_TABLES_BRIDGE)
+		|| family == NFPROTO_BRIDGE
+#endif
+#ifdef CONFIG_NF_TABLES_IPV6
+		|| family == NFPROTO_IPV6
+#endif
+		;
+}
+
 static int nf_tables_newtable(struct sk_buff *skb, const struct nfnl_info *info,
 			      const struct nlattr * const nla[])
 {
@@ -1086,6 +1110,9 @@  static int nf_tables_newtable(struct sk_buff *skb, const struct nfnl_info *info,
 	u32 flags = 0;
 	int err;
 
+	if (!nft_supported_family(family))
+		return -EOPNOTSUPP;
+
 	lockdep_assert_held(&nft_net->commit_mutex);
 	attr = nla[NFTA_TABLE_NAME];
 	table = nft_table_lookup(net, attr, family, genmask,