| Message ID | 20190719123921.1249-1-phil@nwl.cc |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf,v2,1/2] net: nf_tables: Make nft_meta expression more robust | expand |
On Fri, Jul 19, 2019 at 02:39:20PM +0200, Phil Sutter wrote: > nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in > situations where required data is missing breaks inverted checks > like e.g.: > > | meta iifname != eth0 accept > > This rule will never match if there is no input interface (or it is not > known) which is not intuitive and, what's worse, breaks consistency of > iptables-nft with iptables-legacy. > > Fix this by falling back to placing a value in dreg which never matches > (avoiding accidental matches): > > {I,O}IF: > Use invalid ifindex value zero. > > {BRI_,}{I,O}IFNAME, {I,O}IFKIND: > Use an empty string which is neither a valid interface name nor > kind. > > {I,O}IFTYPE: > Use ARPHRD_VOID (0xFFFF). What could it be done with? NFT_META_BRI_IIFPVID NFT_META_BRI_IIFPVPROTO Those will still not work for meta ibrpvid != 40 if interface is not available. For VPROTO probably it's possible. I don't have a solution for IIFPVID.
Hi, On Fri, Jul 19, 2019 at 06:35:21PM +0200, Pablo Neira Ayuso wrote: > On Fri, Jul 19, 2019 at 02:39:20PM +0200, Phil Sutter wrote: > > nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in > > situations where required data is missing breaks inverted checks > > like e.g.: > > > > | meta iifname != eth0 accept > > > > This rule will never match if there is no input interface (or it is not > > known) which is not intuitive and, what's worse, breaks consistency of > > iptables-nft with iptables-legacy. > > > > Fix this by falling back to placing a value in dreg which never matches > > (avoiding accidental matches): > > > > {I,O}IF: > > Use invalid ifindex value zero. > > > > {BRI_,}{I,O}IFNAME, {I,O}IFKIND: > > Use an empty string which is neither a valid interface name nor > > kind. > > > > {I,O}IFTYPE: > > Use ARPHRD_VOID (0xFFFF). > > What could it be done with? > > NFT_META_BRI_IIFPVID > NFT_META_BRI_IIFPVPROTO > > Those will still not work for > > meta ibrpvid != 40 > > if interface is not available. > > For VPROTO probably it's possible. I don't have a solution for > IIFPVID. VLAN IDs 0 and 4095 are reserved, we could use those. I refrained from changing bridge VLAN matches because of IIFPVPROTO, no idea if there's an illegal value we could use for that. If you have an idea, I'm all for it. :) Thanks, Phil
On Sat, Jul 20, 2019 at 05:15:02PM +0200, Phil Sutter wrote: > Hi, > > On Fri, Jul 19, 2019 at 06:35:21PM +0200, Pablo Neira Ayuso wrote: > > On Fri, Jul 19, 2019 at 02:39:20PM +0200, Phil Sutter wrote: > > > nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in > > > situations where required data is missing breaks inverted checks > > > like e.g.: > > > > > > | meta iifname != eth0 accept > > > > > > This rule will never match if there is no input interface (or it is not > > > known) which is not intuitive and, what's worse, breaks consistency of > > > iptables-nft with iptables-legacy. > > > > > > Fix this by falling back to placing a value in dreg which never matches > > > (avoiding accidental matches): > > > > > > {I,O}IF: > > > Use invalid ifindex value zero. > > > > > > {BRI_,}{I,O}IFNAME, {I,O}IFKIND: > > > Use an empty string which is neither a valid interface name nor > > > kind. > > > > > > {I,O}IFTYPE: > > > Use ARPHRD_VOID (0xFFFF). > > > > What could it be done with? > > > > NFT_META_BRI_IIFPVID > > NFT_META_BRI_IIFPVPROTO > > > > Those will still not work for > > > > meta ibrpvid != 40 > > > > if interface is not available. > > > > For VPROTO probably it's possible. I don't have a solution for > > IIFPVID. > > VLAN IDs 0 and 4095 are reserved, we could use those. I refrained from > changing bridge VLAN matches because of IIFPVPROTO, no idea if there's > an illegal value we could use for that. If you have an idea, I'm all for > it. :) I think we can add something like: NFT_META_BRI_IIFVLAN just to check for br_vlan_enabled(), from userspace we can check for exists/missing as a boolean, so we don't have to worry on assuming an unused value for things like this. This can be added in the next release cycle. Regarding IIFTYPE / OIFTYPE, if there is no in-trees interfaces using "", I think we are fine, probably send a patch to propose to disallow this to net. These ones are missing: NFT_META_IIFGROUP NFT_META_OIFGROUP For these two, the default group (0) should be fine since every interface is falling under this category by default. I can squash this small patch to this one and push it one. Thanks.
Hi Pablo, On Mon, Jul 22, 2019 at 09:53:21PM +0200, Pablo Neira Ayuso wrote: > On Sat, Jul 20, 2019 at 05:15:02PM +0200, Phil Sutter wrote: > > Hi, > > > > On Fri, Jul 19, 2019 at 06:35:21PM +0200, Pablo Neira Ayuso wrote: > > > On Fri, Jul 19, 2019 at 02:39:20PM +0200, Phil Sutter wrote: > > > > nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in > > > > situations where required data is missing breaks inverted checks > > > > like e.g.: > > > > > > > > | meta iifname != eth0 accept > > > > > > > > This rule will never match if there is no input interface (or it is not > > > > known) which is not intuitive and, what's worse, breaks consistency of > > > > iptables-nft with iptables-legacy. > > > > > > > > Fix this by falling back to placing a value in dreg which never matches > > > > (avoiding accidental matches): > > > > > > > > {I,O}IF: > > > > Use invalid ifindex value zero. > > > > > > > > {BRI_,}{I,O}IFNAME, {I,O}IFKIND: > > > > Use an empty string which is neither a valid interface name nor > > > > kind. > > > > > > > > {I,O}IFTYPE: > > > > Use ARPHRD_VOID (0xFFFF). > > > > > > What could it be done with? > > > > > > NFT_META_BRI_IIFPVID > > > NFT_META_BRI_IIFPVPROTO > > > > > > Those will still not work for > > > > > > meta ibrpvid != 40 > > > > > > if interface is not available. > > > > > > For VPROTO probably it's possible. I don't have a solution for > > > IIFPVID. > > > > VLAN IDs 0 and 4095 are reserved, we could use those. I refrained from > > changing bridge VLAN matches because of IIFPVPROTO, no idea if there's > > an illegal value we could use for that. If you have an idea, I'm all for > > it. :) > > I think we can add something like: > > NFT_META_BRI_IIFVLAN > > just to check for br_vlan_enabled(), from userspace we can check for > exists/missing as a boolean, so we don't have to worry on assuming an > unused value for things like this. This can be added in the next > release cycle. Adding existence checks where missing is indeed a good idea, but doesn't quite solve the problem we're facing here. :) [...] > These ones are missing: > > NFT_META_IIFGROUP > NFT_META_OIFGROUP > > For these two, the default group (0) should be fine since every > interface is falling under this category by default. > > I can squash this small patch to this one and push it one. My problem with these "sane defaults" is that we may cause inconsistent behaviour in rulesets: In prerouting, 'meta oifgroup 0' will match no matter which interface the packet will be routed to. Yes, prerouting implies there is no output interface (yet), but I consider this an implementation detail and there will likely be cases where it is not as easy to spot why something can't work. Cheers, Phil
On Tue, Jul 23, 2019 at 05:06:44PM +0200, Phil Sutter wrote: > Hi Pablo, > > On Mon, Jul 22, 2019 at 09:53:21PM +0200, Pablo Neira Ayuso wrote: > > On Sat, Jul 20, 2019 at 05:15:02PM +0200, Phil Sutter wrote: > > > Hi, > > > > > > On Fri, Jul 19, 2019 at 06:35:21PM +0200, Pablo Neira Ayuso wrote: > > > > On Fri, Jul 19, 2019 at 02:39:20PM +0200, Phil Sutter wrote: > > > > > nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in > > > > > situations where required data is missing breaks inverted checks > > > > > like e.g.: > > > > > > > > > > | meta iifname != eth0 accept > > > > > > > > > > This rule will never match if there is no input interface (or it is not > > > > > known) which is not intuitive and, what's worse, breaks consistency of > > > > > iptables-nft with iptables-legacy. > > > > > > > > > > Fix this by falling back to placing a value in dreg which never matches > > > > > (avoiding accidental matches): > > > > > > > > > > {I,O}IF: > > > > > Use invalid ifindex value zero. > > > > > > > > > > {BRI_,}{I,O}IFNAME, {I,O}IFKIND: > > > > > Use an empty string which is neither a valid interface name nor > > > > > kind. > > > > > > > > > > {I,O}IFTYPE: > > > > > Use ARPHRD_VOID (0xFFFF). > > > > > > > > What could it be done with? > > > > > > > > NFT_META_BRI_IIFPVID > > > > NFT_META_BRI_IIFPVPROTO > > > > > > > > Those will still not work for > > > > > > > > meta ibrpvid != 40 > > > > > > > > if interface is not available. > > > > > > > > For VPROTO probably it's possible. I don't have a solution for > > > > IIFPVID. > > > > > > VLAN IDs 0 and 4095 are reserved, we could use those. I refrained from > > > changing bridge VLAN matches because of IIFPVPROTO, no idea if there's > > > an illegal value we could use for that. If you have an idea, I'm all for > > > it. :) > > > > I think we can add something like: > > > > NFT_META_BRI_IIFVLAN > > > > just to check for br_vlan_enabled(), from userspace we can check for > > exists/missing as a boolean, so we don't have to worry on assuming an > > unused value for things like this. This can be added in the next > > release cycle. > > Adding existence checks where missing is indeed a good idea, but doesn't > quite solve the problem we're facing here. :) I'm not proposing to use this approach for _every key_. For this one specifically I think it's meaningful to have a NFT_META_BRI_IIFVLAN since vlan is an optional configuration. > [...] > > These ones are missing: > > > > NFT_META_IIFGROUP > > NFT_META_OIFGROUP > > > > For these two, the default group (0) should be fine since every > > interface is falling under this category by default. > > > > I can squash this small patch to this one and push it one. > > My problem with these "sane defaults" is that we may cause inconsistent > behaviour in rulesets: In prerouting, 'meta oifgroup 0' will match no > matter which interface the packet will be routed to. Yes, prerouting > implies there is no output interface (yet), but I consider this an > implementation detail and there will likely be cases where it is not as > easy to spot why something can't work. I think disallowing 'meta oifgroup 0' from prerouting (and the input path in general) is fine from the control plane configuration, but you know Florian disagrees with this. Probably this can be left as is for IIFNAME and OIFNAME, which as used by iptables-nft for compatibility with legacy. For new keys, check for invalid configuration I would argue that is fine. But let's wait for Florian to say what he thinks :-)
diff --git a/net/bridge/netfilter/nft_meta_bridge.c b/net/bridge/netfilter/nft_meta_bridge.c index bed66f536b345..a98dec2cf0cfd 100644 --- a/net/bridge/netfilter/nft_meta_bridge.c +++ b/net/bridge/netfilter/nft_meta_bridge.c @@ -30,13 +30,9 @@ static void nft_meta_bridge_get_eval(const struct nft_expr *expr, switch (priv->key) { case NFT_META_BRI_IIFNAME: br_dev = nft_meta_get_bridge(in); - if (!br_dev) - goto err; break; case NFT_META_BRI_OIFNAME: br_dev = nft_meta_get_bridge(out); - if (!br_dev) - goto err; break; case NFT_META_BRI_IIFPVID: { u16 p_pvid; @@ -64,7 +60,7 @@ static void nft_meta_bridge_get_eval(const struct nft_expr *expr, goto out; } - strncpy((char *)dest, br_dev->name, IFNAMSIZ); + strncpy((char *)dest, br_dev ? br_dev->name : "", IFNAMSIZ); return; out: return nft_meta_get_eval(expr, regs, pkt); diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c index 76866f77e3435..ee3b54692cc7e 100644 --- a/net/netfilter/nft_meta.c +++ b/net/netfilter/nft_meta.c @@ -11,6 +11,7 @@ #include <linux/netlink.h> #include <linux/netfilter.h> #include <linux/netfilter/nf_tables.h> +#include <linux/if_arp.h> #include <linux/in.h> #include <linux/ip.h> #include <linux/ipv6.h> @@ -60,34 +61,22 @@ void nft_meta_get_eval(const struct nft_expr *expr, *dest = skb->mark; break; case NFT_META_IIF: - if (in == NULL) - goto err; - *dest = in->ifindex; + *dest = in ? in->ifindex : 0; break; case NFT_META_OIF: - if (out == NULL) - goto err; - *dest = out->ifindex; + *dest = out ? out->ifindex : 0; break; case NFT_META_IIFNAME: - if (in == NULL) - goto err; - strncpy((char *)dest, in->name, IFNAMSIZ); + strncpy((char *)dest, in ? in->name : "", IFNAMSIZ); break; case NFT_META_OIFNAME: - if (out == NULL) - goto err; - strncpy((char *)dest, out->name, IFNAMSIZ); + strncpy((char *)dest, out ? out->name : "", IFNAMSIZ); break; case NFT_META_IIFTYPE: - if (in == NULL) - goto err; - nft_reg_store16(dest, in->type); + nft_reg_store16(dest, in ? in->type : ARPHRD_VOID); break; case NFT_META_OIFTYPE: - if (out == NULL) - goto err; - nft_reg_store16(dest, out->type); + nft_reg_store16(dest, out ? out->type : ARPHRD_VOID); break; case NFT_META_SKUID: sk = skb_to_full_sk(skb); @@ -216,16 +205,20 @@ void nft_meta_get_eval(const struct nft_expr *expr, nft_reg_store8(dest, secpath_exists(skb)); break; #endif - case NFT_META_IIFKIND: - if (in == NULL || in->rtnl_link_ops == NULL) - goto err; - strncpy((char *)dest, in->rtnl_link_ops->kind, IFNAMSIZ); + case NFT_META_IIFKIND: { + const struct rtnl_link_ops *rl_ops = + in ? in->rtnl_link_ops : NULL; + + strncpy((char *)dest, rl_ops ? rl_ops->kind : "", IFNAMSIZ); break; - case NFT_META_OIFKIND: - if (out == NULL || out->rtnl_link_ops == NULL) - goto err; - strncpy((char *)dest, out->rtnl_link_ops->kind, IFNAMSIZ); + } + case NFT_META_OIFKIND: { + const struct rtnl_link_ops *rl_ops = + out ? out->rtnl_link_ops : NULL; + + strncpy((char *)dest, rl_ops ? rl_ops->kind : "", IFNAMSIZ); break; + } default: WARN_ON(1); goto err;
nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in situations where required data is missing breaks inverted checks like e.g.: | meta iifname != eth0 accept This rule will never match if there is no input interface (or it is not known) which is not intuitive and, what's worse, breaks consistency of iptables-nft with iptables-legacy. Fix this by falling back to placing a value in dreg which never matches (avoiding accidental matches): {I,O}IF: Use invalid ifindex value zero. {BRI_,}{I,O}IFNAME, {I,O}IFKIND: Use an empty string which is neither a valid interface name nor kind. {I,O}IFTYPE: Use ARPHRD_VOID (0xFFFF). Signed-off-by: Phil Sutter <phil@nwl.cc> --- Changes since v1: - Apply same fix to net/bridge/netfilter/nft_meta_bridge.c as well. --- net/bridge/netfilter/nft_meta_bridge.c | 6 +--- net/netfilter/nft_meta.c | 45 +++++++++++--------------- 2 files changed, 20 insertions(+), 31 deletions(-)