Skip to content

Commit 6489469

Browse files
ummakynesFlorian Westphal
ummakynes
authored and
Florian Westphal
committed
netfilter: nft_set_rbtree: validate open interval overlap
Open intervals do not have an end element, in particular an open interval at the end of the set is hard to validate because of it is lacking the end element, and interval validation relies on such end element to perform the checks. This patch adds a new flag field to struct nft_set_elem, this is not an issue because this is a temporary object that is allocated in the stack from the insert/deactivate path. This flag field is used to specify that this is the last element in this add/delete command. The last flag is used, in combination with the start element cookie, to check if there is a partial overlap, eg. Already exists: 255.255.255.0-255.255.255.254 Add interval: 255.255.255.0-255.255.255.255 ~~~~~~~~~~~~~ start element overlap Basically, the idea is to check for an existing end element in the set if there is an overlap with an existing start element. However, the last open interval can come in any position in the add command, the corner case can get a bit more complicated: Already exists: 255.255.255.0-255.255.255.254 Add intervals: 255.255.255.0-255.255.255.255,255.255.255.0-255.255.255.254 ~~~~~~~~~~~~~ start element overlap To catch this overlap, annotate that the new start element is a possible overlap, then report the overlap if the next element is another start element that confirms that previous element in an open interval at the end of the set. For deletions, do not update the start cookie when deleting an open interval, otherwise this can trigger spurious EEXIST when adding new elements. Unfortunately, there is no NFT_SET_ELEM_INTERVAL_OPEN flag which would make easier to detect open interval overlaps. Fixes: 7c84d41 ("netfilter: nft_set_rbtree: Detect partial overlaps on insertion") Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> Signed-off-by: Florian Westphal <fw@strlen.de>
1 parent 782f268 commit 6489469

3 files changed

Lines changed: 82 additions & 14 deletions

File tree

‎include/net/netfilter/nf_tables.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,8 @@ struct nft_userdata {
277277
unsigned char data[];
278278
};
279279

280+
#define NFT_SET_ELEM_INTERNAL_LAST 0x1
281+
280282
/* placeholder structure for opaque set element backend representation. */
281283
struct nft_elem_priv { };
282284

@@ -286,6 +288,7 @@ struct nft_elem_priv { };
286288
* @key: element key
287289
* @key_end: closing element key
288290
* @data: element data
291+
* @flags: flags
289292
* @priv: element private data and extensions
290293
*/
291294
struct nft_set_elem {
@@ -301,6 +304,7 @@ struct nft_set_elem {
301304
u32 buf[NFT_DATA_VALUE_MAXLEN / sizeof(u32)];
302305
struct nft_data val;
303306
} data;
307+
u32 flags;
304308
struct nft_elem_priv *priv;
305309
};
306310

‎net/netfilter/nf_tables_api.c‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7270,7 +7270,8 @@ static u32 nft_set_maxsize(const struct nft_set *set)
72707270
}
72717271

72727272
static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
7273-
const struct nlattr *attr, u32 nlmsg_flags)
7273+
const struct nlattr *attr, u32 nlmsg_flags,
7274+
bool last)
72747275
{
72757276
struct nft_expr *expr_array[NFT_SET_EXPR_MAX] = {};
72767277
struct nlattr *nla[NFTA_SET_ELEM_MAX + 1];
@@ -7556,6 +7557,11 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
75567557
if (flags)
75577558
*nft_set_ext_flags(ext) = flags;
75587559

7560+
if (last)
7561+
elem.flags = NFT_SET_ELEM_INTERNAL_LAST;
7562+
else
7563+
elem.flags = 0;
7564+
75597565
if (obj)
75607566
*nft_set_ext_obj(ext) = obj;
75617567

@@ -7719,7 +7725,8 @@ static int nf_tables_newsetelem(struct sk_buff *skb,
77197725
nft_ctx_init(&ctx, net, skb, info->nlh, family, table, NULL, nla);
77207726

77217727
nla_for_each_nested(attr, nla[NFTA_SET_ELEM_LIST_ELEMENTS], rem) {
7722-
err = nft_add_set_elem(&ctx, set, attr, info->nlh->nlmsg_flags);
7728+
err = nft_add_set_elem(&ctx, set, attr, info->nlh->nlmsg_flags,
7729+
nla_is_last(attr, rem));
77237730
if (err < 0) {
77247731
NL_SET_BAD_ATTR(extack, attr);
77257732
return err;
@@ -7843,7 +7850,7 @@ static void nft_trans_elems_destroy_abort(const struct nft_ctx *ctx,
78437850
}
78447851

78457852
static int nft_del_setelem(struct nft_ctx *ctx, struct nft_set *set,
7846-
const struct nlattr *attr)
7853+
const struct nlattr *attr, bool last)
78477854
{
78487855
struct nlattr *nla[NFTA_SET_ELEM_MAX + 1];
78497856
struct nft_set_ext_tmpl tmpl;
@@ -7911,6 +7918,11 @@ static int nft_del_setelem(struct nft_ctx *ctx, struct nft_set *set,
79117918
if (flags)
79127919
*nft_set_ext_flags(ext) = flags;
79137920

7921+
if (last)
7922+
elem.flags = NFT_SET_ELEM_INTERNAL_LAST;
7923+
else
7924+
elem.flags = 0;
7925+
79147926
trans = nft_trans_elem_alloc(ctx, NFT_MSG_DELSETELEM, set);
79157927
if (trans == NULL)
79167928
goto fail_trans;
@@ -8058,7 +8070,8 @@ static int nf_tables_delsetelem(struct sk_buff *skb,
80588070
return nft_set_flush(&ctx, set, genmask);
80598071

80608072
nla_for_each_nested(attr, nla[NFTA_SET_ELEM_LIST_ELEMENTS], rem) {
8061-
err = nft_del_setelem(&ctx, set, attr);
8073+
err = nft_del_setelem(&ctx, set, attr,
8074+
nla_is_last(attr, rem));
80628075
if (err == -ENOENT &&
80638076
NFNL_MSG_TYPE(info->nlh->nlmsg_type) == NFT_MSG_DESTROYSETELEM)
80648077
continue;

‎net/netfilter/nft_set_rbtree.c‎

Lines changed: 61 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -304,10 +304,19 @@ static void nft_rbtree_set_start_cookie(struct nft_rbtree *priv,
304304
priv->start_rbe_cookie = (unsigned long)rbe;
305305
}
306306

307+
static void nft_rbtree_set_start_cookie_open(struct nft_rbtree *priv,
308+
const struct nft_rbtree_elem *rbe,
309+
unsigned long open_interval)
310+
{
311+
priv->start_rbe_cookie = (unsigned long)rbe | open_interval;
312+
}
313+
314+
#define NFT_RBTREE_OPEN_INTERVAL 1UL
315+
307316
static bool nft_rbtree_cmp_start_cookie(struct nft_rbtree *priv,
308317
const struct nft_rbtree_elem *rbe)
309318
{
310-
return priv->start_rbe_cookie == (unsigned long)rbe;
319+
return (priv->start_rbe_cookie & ~NFT_RBTREE_OPEN_INTERVAL) == (unsigned long)rbe;
311320
}
312321

313322
static bool nft_rbtree_insert_same_interval(const struct net *net,
@@ -337,13 +346,14 @@ static bool nft_rbtree_insert_same_interval(const struct net *net,
337346

338347
static int __nft_rbtree_insert(const struct net *net, const struct nft_set *set,
339348
struct nft_rbtree_elem *new,
340-
struct nft_elem_priv **elem_priv, u64 tstamp)
349+
struct nft_elem_priv **elem_priv, u64 tstamp, bool last)
341350
{
342351
struct nft_rbtree_elem *rbe, *rbe_le = NULL, *rbe_ge = NULL, *rbe_prev;
343352
struct rb_node *node, *next, *parent, **p, *first = NULL;
344353
struct nft_rbtree *priv = nft_set_priv(set);
345354
u8 cur_genmask = nft_genmask_cur(net);
346355
u8 genmask = nft_genmask_next(net);
356+
unsigned long open_interval = 0;
347357
int d;
348358

349359
/* Descend the tree to search for an existing element greater than the
@@ -449,18 +459,46 @@ static int __nft_rbtree_insert(const struct net *net, const struct nft_set *set,
449459
}
450460
}
451461

452-
if (nft_rbtree_interval_null(set, new))
453-
priv->start_rbe_cookie = 0;
454-
else if (nft_rbtree_interval_start(new) && priv->start_rbe_cookie)
462+
if (nft_rbtree_interval_null(set, new)) {
455463
priv->start_rbe_cookie = 0;
464+
} else if (nft_rbtree_interval_start(new) && priv->start_rbe_cookie) {
465+
if (nft_set_is_anonymous(set)) {
466+
priv->start_rbe_cookie = 0;
467+
} else if (priv->start_rbe_cookie & NFT_RBTREE_OPEN_INTERVAL) {
468+
/* Previous element is an open interval that partially
469+
* overlaps with an existing non-open interval.
470+
*/
471+
return -ENOTEMPTY;
472+
}
473+
}
456474

457475
/* - new start element matching existing start element: full overlap
458476
* reported as -EEXIST, cleared by caller if NLM_F_EXCL is not given.
459477
*/
460478
if (rbe_ge && !nft_rbtree_cmp(set, new, rbe_ge) &&
461479
nft_rbtree_interval_start(rbe_ge) == nft_rbtree_interval_start(new)) {
462480
*elem_priv = &rbe_ge->priv;
463-
nft_rbtree_set_start_cookie(priv, rbe_ge);
481+
482+
/* - Corner case: new start element of open interval (which
483+
* comes as last element in the batch) overlaps the start of
484+
* an existing interval with an end element: partial overlap.
485+
*/
486+
node = rb_first(&priv->root);
487+
rbe = __nft_rbtree_next_active(node, genmask);
488+
if (rbe && nft_rbtree_interval_end(rbe)) {
489+
rbe = nft_rbtree_next_active(rbe, genmask);
490+
if (rbe &&
491+
nft_rbtree_interval_start(rbe) &&
492+
!nft_rbtree_cmp(set, new, rbe)) {
493+
if (last)
494+
return -ENOTEMPTY;
495+
496+
/* Maybe open interval? */
497+
open_interval = NFT_RBTREE_OPEN_INTERVAL;
498+
}
499+
}
500+
nft_rbtree_set_start_cookie_open(priv, rbe_ge, open_interval);
501+
464502
return -EEXIST;
465503
}
466504

@@ -515,6 +553,12 @@ static int __nft_rbtree_insert(const struct net *net, const struct nft_set *set,
515553
nft_rbtree_interval_end(rbe_ge) && nft_rbtree_interval_end(new))
516554
return -ENOTEMPTY;
517555

556+
/* - start element overlaps an open interval but end element is new:
557+
* partial overlap, reported as -ENOEMPTY.
558+
*/
559+
if (!rbe_ge && priv->start_rbe_cookie && nft_rbtree_interval_end(new))
560+
return -ENOTEMPTY;
561+
518562
/* Accepted element: pick insertion point depending on key value */
519563
parent = NULL;
520564
p = &priv->root.rb_node;
@@ -624,6 +668,7 @@ static int nft_rbtree_insert(const struct net *net, const struct nft_set *set,
624668
struct nft_elem_priv **elem_priv)
625669
{
626670
struct nft_rbtree_elem *rbe = nft_elem_priv_cast(elem->priv);
671+
bool last = !!(elem->flags & NFT_SET_ELEM_INTERNAL_LAST);
627672
struct nft_rbtree *priv = nft_set_priv(set);
628673
u64 tstamp = nft_net_tstamp(net);
629674
int err;
@@ -640,8 +685,12 @@ static int nft_rbtree_insert(const struct net *net, const struct nft_set *set,
640685
cond_resched();
641686

642687
write_lock_bh(&priv->lock);
643-
err = __nft_rbtree_insert(net, set, rbe, elem_priv, tstamp);
688+
err = __nft_rbtree_insert(net, set, rbe, elem_priv, tstamp, last);
644689
write_unlock_bh(&priv->lock);
690+
691+
if (nft_rbtree_interval_end(rbe))
692+
priv->start_rbe_cookie = 0;
693+
645694
} while (err == -EAGAIN);
646695

647696
return err;
@@ -729,6 +778,7 @@ nft_rbtree_deactivate(const struct net *net, const struct nft_set *set,
729778
const struct nft_set_elem *elem)
730779
{
731780
struct nft_rbtree_elem *rbe, *this = nft_elem_priv_cast(elem->priv);
781+
bool last = !!(elem->flags & NFT_SET_ELEM_INTERNAL_LAST);
732782
struct nft_rbtree *priv = nft_set_priv(set);
733783
const struct rb_node *parent = priv->root.rb_node;
734784
u8 genmask = nft_genmask_next(net);
@@ -769,9 +819,10 @@ nft_rbtree_deactivate(const struct net *net, const struct nft_set *set,
769819
continue;
770820
}
771821

772-
if (nft_rbtree_interval_start(rbe))
773-
nft_rbtree_set_start_cookie(priv, rbe);
774-
else if (!nft_rbtree_deactivate_same_interval(net, priv, rbe))
822+
if (nft_rbtree_interval_start(rbe)) {
823+
if (!last)
824+
nft_rbtree_set_start_cookie(priv, rbe);
825+
} else if (!nft_rbtree_deactivate_same_interval(net, priv, rbe))
775826
return NULL;
776827

777828
nft_rbtree_flush(net, set, &rbe->priv);

0 commit comments

Comments
 (0)