NACK/Cmt: [SRU][F][PATCH 1/1] netfilter: nf_tables: use timestamp to check for set element timeout
Thibault Ferrante
thibault.ferrante at canonical.com
Mon Sep 2 10:50:54 UTC 2024
On 30-08-2024 11:05, Massimiliano Pellizzer wrote:
> From: Pablo Neira Ayuso <pablo at netfilter.org>
>
> commit 7395dfacfff65e9938ac0889dafa1ab01e987d15 upstream
>
> Add a timestamp field at the beginning of the transaction, store it
> in the nftables per-netns area.
>
> Update set backend .insert, .deactivate and sync gc path to use the
> timestamp, this avoids that an element expires while control plane
> transaction is still unfinished.
>
> .lookup and .update, which are used from packet path, still use the
> current time to check if the element has expired. And .get path and dump
> also since this runs lockless under rcu read size lock. Then, there is
> async gc which also needs to check the current time since it runs
> asynchronously from a workqueue.
>
> [ NB: rbtree GC updates has been excluded because GC is asynchronous. ]
>
> Fixes: c3e1b005ed1c ("netfilter: nf_tables: add set element timeout support")
> Signed-off-by: Pablo Neira Ayuso <pablo at netfilter.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> (backported from commit eaf1a29ea5d7dba8e84e9e9f3b3f47d0cd540bfe linux-5.4.y)
> [mpellizzer: solved a merge conflict due to a variable declaration which
> does not affect the patch]
> CVE--2024-27397
CVE-2024-27397
Nacking this as I expect to break quite some automation to have this mistake.
> Signed-off-by: Massimiliano Pellizzer <massimiliano.pellizzer at canonical.com>
> ---
> include/net/netfilter/nf_tables.h | 21 +++++++++++++++++++--
> net/netfilter/nf_tables_api.c | 1 +
> net/netfilter/nft_set_hash.c | 8 +++++++-
> net/netfilter/nft_set_rbtree.c | 6 ++++--
> 4 files changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index 74ee59746f6b..a7672b8a909e 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -13,6 +13,7 @@
> #include <net/netfilter/nf_flow_table.h>
> #include <net/netlink.h>
> #include <net/flow_offload.h>
> +#include <net/netns/generic.h>
>
> struct module;
>
> @@ -656,10 +657,16 @@ static inline struct nft_expr *nft_set_ext_expr(const struct nft_set_ext *ext)
> return nft_set_ext(ext, NFT_SET_EXT_EXPR);
> }
>
> -static inline bool nft_set_elem_expired(const struct nft_set_ext *ext)
> +static inline bool __nft_set_elem_expired(const struct nft_set_ext *ext,
> + u64 tstamp)
> {
> return nft_set_ext_exists(ext, NFT_SET_EXT_EXPIRATION) &&
> - time_is_before_eq_jiffies64(*nft_set_ext_expiration(ext));
> + time_after_eq64(tstamp, *nft_set_ext_expiration(ext));
> +}
> +
> +static inline bool nft_set_elem_expired(const struct nft_set_ext *ext)
> +{
> + return __nft_set_elem_expired(ext, get_jiffies_64());
> }
>
> static inline struct nft_set_ext *nft_set_elem_ext(const struct nft_set *set,
> @@ -1488,9 +1495,19 @@ struct nftables_pernet {
> struct list_head module_list;
> struct list_head notify_list;
> struct mutex commit_mutex;
> + u64 tstamp;
> unsigned int base_seq;
> u8 validate_state;
> unsigned int gc_seq;
> };
>
> +extern unsigned int nf_tables_net_id;
> +
> +static inline u64 nft_net_tstamp(const struct net *net)
> +{
> + struct nftables_pernet *nft_net = net_generic(net, nf_tables_net_id);
> +
> + return nft_net->tstamp;
> +}
> +
> #endif /* _NET_NF_TABLES_H */
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 10e7d8e9df04..14f730a371bd 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -7810,6 +7810,7 @@ static bool nf_tables_valid_genid(struct net *net, u32 genid)
> bool genid_ok;
>
> mutex_lock(&nft_net->commit_mutex);
> + nft_net->tstamp = get_jiffies_64();
>
> genid_ok = genid == 0 || nft_net->base_seq == genid;
> if (!genid_ok)
> diff --git a/net/netfilter/nft_set_hash.c b/net/netfilter/nft_set_hash.c
> index 7f49c6c37c94..92ab00d17337 100644
> --- a/net/netfilter/nft_set_hash.c
> +++ b/net/netfilter/nft_set_hash.c
> @@ -38,6 +38,7 @@ struct nft_rhash_cmp_arg {
> const struct nft_set *set;
> const u32 *key;
> u8 genmask;
> + u64 tstamp;
> };
>
> static inline u32 nft_rhash_key(const void *data, u32 len, u32 seed)
> @@ -64,7 +65,7 @@ static inline int nft_rhash_cmp(struct rhashtable_compare_arg *arg,
> return 1;
> if (nft_set_elem_is_dead(&he->ext))
> return 1;
> - if (nft_set_elem_expired(&he->ext))
> + if (__nft_set_elem_expired(&he->ext, x->tstamp))
> return 1;
> if (!nft_set_elem_active(&he->ext, x->genmask))
> return 1;
> @@ -88,6 +89,7 @@ static bool nft_rhash_lookup(const struct net *net, const struct nft_set *set,
> .genmask = nft_genmask_cur(net),
> .set = set,
> .key = key,
> + .tstamp = get_jiffies_64(),
> };
>
> he = rhashtable_lookup(&priv->ht, &arg, nft_rhash_params);
> @@ -106,6 +108,7 @@ static void *nft_rhash_get(const struct net *net, const struct nft_set *set,
> .genmask = nft_genmask_cur(net),
> .set = set,
> .key = elem->key.val.data,
> + .tstamp = get_jiffies_64(),
> };
>
> he = rhashtable_lookup(&priv->ht, &arg, nft_rhash_params);
> @@ -129,6 +132,7 @@ static bool nft_rhash_update(struct nft_set *set, const u32 *key,
> .genmask = NFT_GENMASK_ANY,
> .set = set,
> .key = key,
> + .tstamp = get_jiffies_64(),
> };
>
> he = rhashtable_lookup(&priv->ht, &arg, nft_rhash_params);
> @@ -172,6 +176,7 @@ static int nft_rhash_insert(const struct net *net, const struct nft_set *set,
> .genmask = nft_genmask_next(net),
> .set = set,
> .key = elem->key.val.data,
> + .tstamp = nft_net_tstamp(net),
> };
> struct nft_rhash_elem *prev;
>
> @@ -214,6 +219,7 @@ static void *nft_rhash_deactivate(const struct net *net,
> .genmask = nft_genmask_next(net),
> .set = set,
> .key = elem->key.val.data,
> + .tstamp = nft_net_tstamp(net),
> };
>
> rcu_read_lock();
> diff --git a/net/netfilter/nft_set_rbtree.c b/net/netfilter/nft_set_rbtree.c
> index ec322d818de0..4594e1d88cf9 100644
> --- a/net/netfilter/nft_set_rbtree.c
> +++ b/net/netfilter/nft_set_rbtree.c
> @@ -316,6 +316,7 @@ static int __nft_rbtree_insert(const struct net *net, const struct nft_set *set,
> struct nft_rbtree *priv = nft_set_priv(set);
> u8 cur_genmask = nft_genmask_cur(net);
> u8 genmask = nft_genmask_next(net);
> + u64 tstamp = nft_net_tstamp(net);
> int d;
>
> /* Descend the tree to search for an existing element greater than the
> @@ -363,7 +364,7 @@ static int __nft_rbtree_insert(const struct net *net, const struct nft_set *set,
> /* perform garbage collection to avoid bogus overlap reports
> * but skip new elements in this transaction.
> */
> - if (nft_set_elem_expired(&rbe->ext) &&
> + if (__nft_set_elem_expired(&rbe->ext, tstamp) &&
> nft_set_elem_active(&rbe->ext, cur_genmask)) {
> const struct nft_rbtree_elem *removed_end;
>
> @@ -550,6 +551,7 @@ static void *nft_rbtree_deactivate(const struct net *net,
> const struct rb_node *parent = priv->root.rb_node;
> struct nft_rbtree_elem *rbe, *this = elem->priv;
> u8 genmask = nft_genmask_next(net);
> + u64 tstamp = nft_net_tstamp(net);
> int d;
>
> while (parent != NULL) {
> @@ -570,7 +572,7 @@ static void *nft_rbtree_deactivate(const struct net *net,
> nft_rbtree_interval_end(this)) {
> parent = parent->rb_right;
> continue;
> - } else if (nft_set_elem_expired(&rbe->ext)) {
> + } else if (__nft_set_elem_expired(&rbe->ext, tstamp)) {
> break;
> } else if (!nft_set_elem_active(&rbe->ext, genmask)) {
> parent = parent->rb_left;
--
--
Thibault
More information about the kernel-team
mailing list