Skip to content

Commit 2313c52

Browse files
fw-strlenfw-strlen
committed
netfilter: nf_tables: upfront validation of data via nft_data_init()
JIRA: https://issues.redhat.com/browse/RHEL-1720 JIRA: https://issues.redhat.com/browse/RHEL-1721 Upstream Status: commit 341b694 Conflicts: net/netfilter/nf_tables_api.c Patch applied in reverse order: f323ef3 ("netfilter: nf_tables: disallow jump to implicit chain from set element") already in cs9, but was applied later than this change upstream net/netfilter/nft_bitwise.c context, cs9 lacks 00bd435 ("netfilter: bitwise: improve error goto labels") net/netfilter/nft_cmp.c context, cs9 lacks 23f68d4 ("netfilter: nft_cmp: optimize comparison for 16-bytes"), drop changes in those places. commit 341b694 Author: Pablo Neira Ayuso <[email protected]> Date: Mon Aug 8 19:30:06 2022 +0200 netfilter: nf_tables: upfront validation of data via nft_data_init() Instead of parsing the data and then validate that type and length are correct, pass a description of the expected data so it can be validated upfront before parsing it to bail out earlier. This patch adds a new .size field to specify the maximum size of the data area. The .len field is optional and it is used as an input/output field, it provides the specific length of the expected data in the input path. If then .len field is not specified, then obtained length from the netlink attribute is stored. This is required by cmp, bitwise, range and immediate, which provide no netlink attribute that describes the data length. The immediate expression uses the destination register type to infer the expected data type. Relying on opencoded validation of the expected data might lead to subtle bugs as described in 7e6bc1f ("netfilter: nf_tables: stricter validation of element data"). Signed-off-by: Pablo Neira Ayuso <[email protected]> Signed-off-by: Florian Westphal <[email protected]>
1 parent db8d3a5 commit 2313c52

File tree

6 files changed

+123
-114
lines changed

6 files changed

+123
-114
lines changed

‎include/net/netfilter/nf_tables.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -197,12 +197,12 @@ enum nft_data_desc_flags {
197197

198198
struct nft_data_desc {
199199
enum nft_data_types type;
200+
unsigned int size;
200201
unsigned int len;
201202
unsigned int flags;
202203
};
203204

204-
int nft_data_init(const struct nft_ctx *ctx,
205-
struct nft_data *data, unsigned int size,
205+
int nft_data_init(const struct nft_ctx *ctx, struct nft_data *data,
206206
struct nft_data_desc *desc, const struct nlattr *nla);
207207
void nft_data_hold(const struct nft_data *data, enum nft_data_types type);
208208
void nft_data_release(const struct nft_data *data, enum nft_data_types type);

‎net/netfilter/nf_tables_api.c‎

Lines changed: 41 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -5393,19 +5393,13 @@ static int nft_setelem_parse_flags(const struct nft_set *set,
53935393
static int nft_setelem_parse_key(struct nft_ctx *ctx, struct nft_set *set,
53945394
struct nft_data *key, struct nlattr *attr)
53955395
{
5396-
struct nft_data_desc desc;
5397-
int err;
5398-
5399-
err = nft_data_init(ctx, key, NFT_DATA_VALUE_MAXLEN, &desc, attr);
5400-
if (err < 0)
5401-
return err;
5402-
5403-
if (desc.type != NFT_DATA_VALUE || desc.len != set->klen) {
5404-
nft_data_release(key, desc.type);
5405-
return -EINVAL;
5406-
}
5396+
struct nft_data_desc desc = {
5397+
.type = NFT_DATA_VALUE,
5398+
.size = NFT_DATA_VALUE_MAXLEN,
5399+
.len = set->klen,
5400+
};
54075401

5408-
return 0;
5402+
return nft_data_init(ctx, key, &desc, attr);
54095403
}
54105404

54115405
static int nft_setelem_parse_data(struct nft_ctx *ctx, struct nft_set *set,
@@ -5414,26 +5408,18 @@ static int nft_setelem_parse_data(struct nft_ctx *ctx, struct nft_set *set,
54145408
struct nlattr *attr)
54155409
{
54165410
u32 dtype;
5417-
int err;
5418-
5419-
desc->flags = NFT_DATA_DESC_SETELEM;
5420-
5421-
err = nft_data_init(ctx, data, NFT_DATA_VALUE_MAXLEN, desc, attr);
5422-
if (err < 0)
5423-
return err;
54245411

54255412
if (set->dtype == NFT_DATA_VERDICT)
54265413
dtype = NFT_DATA_VERDICT;
54275414
else
54285415
dtype = NFT_DATA_VALUE;
54295416

5430-
if (dtype != desc->type ||
5431-
set->dlen != desc->len) {
5432-
nft_data_release(data, desc->type);
5433-
return -EINVAL;
5434-
}
5417+
desc->type = dtype;
5418+
desc->size = NFT_DATA_VALUE_MAXLEN;
5419+
desc->len = set->dlen;
5420+
desc->flags = NFT_DATA_DESC_SETELEM;
54355421

5436-
return 0;
5422+
return nft_data_init(ctx, data, desc, attr);
54375423
}
54385424

54395425
static void *nft_setelem_catchall_get(const struct net *net,
@@ -9908,7 +9894,7 @@ static int nft_verdict_init(const struct nft_ctx *ctx, struct nft_data *data,
99089894
}
99099895

99109896
desc->len = sizeof(data->verdict);
9911-
desc->type = NFT_DATA_VERDICT;
9897+
99129898
return 0;
99139899
}
99149900

@@ -9951,20 +9937,25 @@ int nft_verdict_dump(struct sk_buff *skb, int type, const struct nft_verdict *v)
99519937
}
99529938

99539939
static int nft_value_init(const struct nft_ctx *ctx,
9954-
struct nft_data *data, unsigned int size,
9955-
struct nft_data_desc *desc, const struct nlattr *nla)
9940+
struct nft_data *data, struct nft_data_desc *desc,
9941+
const struct nlattr *nla)
99569942
{
99579943
unsigned int len;
99589944

99599945
len = nla_len(nla);
99609946
if (len == 0)
99619947
return -EINVAL;
9962-
if (len > size)
9948+
if (len > desc->size)
99639949
return -EOVERFLOW;
9950+
if (desc->len) {
9951+
if (len != desc->len)
9952+
return -EINVAL;
9953+
} else {
9954+
desc->len = len;
9955+
}
99649956

99659957
nla_memcpy(data->data, nla, len);
9966-
desc->type = NFT_DATA_VALUE;
9967-
desc->len = len;
9958+
99689959
return 0;
99699960
}
99709961

@@ -9984,7 +9975,6 @@ static const struct nla_policy nft_data_policy[NFTA_DATA_MAX + 1] = {
99849975
*
99859976
* @ctx: context of the expression using the data
99869977
* @data: destination struct nft_data
9987-
* @size: maximum data length
99889978
* @desc: data description
99899979
* @nla: netlink attribute containing data
99909980
*
@@ -9994,24 +9984,35 @@ static const struct nla_policy nft_data_policy[NFTA_DATA_MAX + 1] = {
99949984
* The caller can indicate that it only wants to accept data of type
99959985
* NFT_DATA_VALUE by passing NULL for the ctx argument.
99969986
*/
9997-
int nft_data_init(const struct nft_ctx *ctx,
9998-
struct nft_data *data, unsigned int size,
9987+
int nft_data_init(const struct nft_ctx *ctx, struct nft_data *data,
99999988
struct nft_data_desc *desc, const struct nlattr *nla)
100009989
{
100019990
struct nlattr *tb[NFTA_DATA_MAX + 1];
100029991
int err;
100039992

9993+
if (WARN_ON_ONCE(!desc->size))
9994+
return -EINVAL;
9995+
100049996
err = nla_parse_nested_deprecated(tb, NFTA_DATA_MAX, nla,
100059997
nft_data_policy, NULL);
100069998
if (err < 0)
100079999
return err;
1000810000

10009-
if (tb[NFTA_DATA_VALUE])
10010-
return nft_value_init(ctx, data, size, desc,
10011-
tb[NFTA_DATA_VALUE]);
10012-
if (tb[NFTA_DATA_VERDICT] && ctx != NULL)
10013-
return nft_verdict_init(ctx, data, desc, tb[NFTA_DATA_VERDICT]);
10014-
return -EINVAL;
10001+
if (tb[NFTA_DATA_VALUE]) {
10002+
if (desc->type != NFT_DATA_VALUE)
10003+
return -EINVAL;
10004+
10005+
err = nft_value_init(ctx, data, desc, tb[NFTA_DATA_VALUE]);
10006+
} else if (tb[NFTA_DATA_VERDICT] && ctx != NULL) {
10007+
if (desc->type != NFT_DATA_VERDICT)
10008+
return -EINVAL;
10009+
10010+
err = nft_verdict_init(ctx, data, desc, tb[NFTA_DATA_VERDICT]);
10011+
} else {
10012+
err = -EINVAL;
10013+
}
10014+
10015+
return err;
1001510016
}
1001610017
EXPORT_SYMBOL_GPL(nft_data_init);
1001710018

‎net/netfilter/nft_bitwise.c‎

Lines changed: 34 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,16 @@ static const struct nla_policy nft_bitwise_policy[NFTA_BITWISE_MAX + 1] = {
9393
static int nft_bitwise_init_bool(struct nft_bitwise *priv,
9494
const struct nlattr *const tb[])
9595
{
96-
struct nft_data_desc mask, xor;
96+
struct nft_data_desc mask = {
97+
.type = NFT_DATA_VALUE,
98+
.size = sizeof(priv->mask),
99+
.len = priv->len,
100+
};
101+
struct nft_data_desc xor = {
102+
.type = NFT_DATA_VALUE,
103+
.size = sizeof(priv->xor),
104+
.len = priv->len,
105+
};
97106
int err;
98107

99108
if (tb[NFTA_BITWISE_DATA])
@@ -103,36 +112,30 @@ static int nft_bitwise_init_bool(struct nft_bitwise *priv,
103112
!tb[NFTA_BITWISE_XOR])
104113
return -EINVAL;
105114

106-
err = nft_data_init(NULL, &priv->mask, sizeof(priv->mask), &mask,
107-
tb[NFTA_BITWISE_MASK]);
115+
err = nft_data_init(NULL, &priv->mask, &mask, tb[NFTA_BITWISE_MASK]);
108116
if (err < 0)
109117
return err;
110-
if (mask.type != NFT_DATA_VALUE || mask.len != priv->len) {
111-
err = -EINVAL;
112-
goto err1;
113-
}
114118

115-
err = nft_data_init(NULL, &priv->xor, sizeof(priv->xor), &xor,
116-
tb[NFTA_BITWISE_XOR]);
119+
err = nft_data_init(NULL, &priv->xor, &xor, tb[NFTA_BITWISE_XOR]);
117120
if (err < 0)
118-
goto err1;
119-
if (xor.type != NFT_DATA_VALUE || xor.len != priv->len) {
120-
err = -EINVAL;
121-
goto err2;
122-
}
121+
goto err_xor_err;
123122

124123
return 0;
125-
err2:
126-
nft_data_release(&priv->xor, xor.type);
127-
err1:
124+
125+
err_xor_err:
128126
nft_data_release(&priv->mask, mask.type);
127+
129128
return err;
130129
}
131130

132131
static int nft_bitwise_init_shift(struct nft_bitwise *priv,
133132
const struct nlattr *const tb[])
134133
{
135-
struct nft_data_desc d;
134+
struct nft_data_desc desc = {
135+
.type = NFT_DATA_VALUE,
136+
.size = sizeof(priv->data),
137+
.len = sizeof(u32),
138+
};
136139
int err;
137140

138141
if (tb[NFTA_BITWISE_MASK] ||
@@ -142,13 +145,12 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
142145
if (!tb[NFTA_BITWISE_DATA])
143146
return -EINVAL;
144147

145-
err = nft_data_init(NULL, &priv->data, sizeof(priv->data), &d,
146-
tb[NFTA_BITWISE_DATA]);
148+
err = nft_data_init(NULL, &priv->data, &desc, tb[NFTA_BITWISE_DATA]);
147149
if (err < 0)
148150
return err;
149-
if (d.type != NFT_DATA_VALUE || d.len != sizeof(u32) ||
150-
priv->data.data[0] >= BITS_PER_TYPE(u32)) {
151-
nft_data_release(&priv->data, d.type);
151+
152+
if (priv->data.data[0] >= BITS_PER_TYPE(u32)) {
153+
nft_data_release(&priv->data, desc.type);
152154
return -EINVAL;
153155
}
154156

@@ -291,22 +293,21 @@ static const struct nft_expr_ops nft_bitwise_ops = {
291293
static int
292294
nft_bitwise_extract_u32_data(const struct nlattr * const tb, u32 *out)
293295
{
294-
struct nft_data_desc desc;
295296
struct nft_data data;
296-
int err = 0;
297+
struct nft_data_desc desc = {
298+
.type = NFT_DATA_VALUE,
299+
.size = sizeof(data),
300+
.len = sizeof(u32),
301+
};
302+
int err;
297303

298-
err = nft_data_init(NULL, &data, sizeof(data), &desc, tb);
304+
err = nft_data_init(NULL, &data, &desc, tb);
299305
if (err < 0)
300306
return err;
301307

302-
if (desc.type != NFT_DATA_VALUE || desc.len != sizeof(u32)) {
303-
err = -EINVAL;
304-
goto err;
305-
}
306308
*out = data.data[0];
307-
err:
308-
nft_data_release(&data, desc.type);
309-
return err;
309+
310+
return 0;
310311
}
311312

312313
static int nft_bitwise_fast_init(const struct nft_ctx *ctx,

‎net/netfilter/nft_cmp.c‎

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -73,20 +73,16 @@ static int nft_cmp_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
7373
const struct nlattr * const tb[])
7474
{
7575
struct nft_cmp_expr *priv = nft_expr_priv(expr);
76-
struct nft_data_desc desc;
76+
struct nft_data_desc desc = {
77+
.type = NFT_DATA_VALUE,
78+
.size = sizeof(priv->data),
79+
};
7780
int err;
7881

79-
err = nft_data_init(NULL, &priv->data, sizeof(priv->data), &desc,
80-
tb[NFTA_CMP_DATA]);
82+
err = nft_data_init(NULL, &priv->data, &desc, tb[NFTA_CMP_DATA]);
8183
if (err < 0)
8284
return err;
8385

84-
if (desc.type != NFT_DATA_VALUE) {
85-
err = -EINVAL;
86-
nft_data_release(&priv->data, desc.type);
87-
return err;
88-
}
89-
9086
err = nft_parse_register_load(tb[NFTA_CMP_SREG], &priv->sreg, desc.len);
9187
if (err < 0)
9288
return err;
@@ -202,12 +198,14 @@ static int nft_cmp_fast_init(const struct nft_ctx *ctx,
202198
const struct nlattr * const tb[])
203199
{
204200
struct nft_cmp_fast_expr *priv = nft_expr_priv(expr);
205-
struct nft_data_desc desc;
206201
struct nft_data data;
202+
struct nft_data_desc desc = {
203+
.type = NFT_DATA_VALUE,
204+
.size = sizeof(data),
205+
};
207206
int err;
208207

209-
err = nft_data_init(NULL, &data, sizeof(data), &desc,
210-
tb[NFTA_CMP_DATA]);
208+
err = nft_data_init(NULL, &data, &desc, tb[NFTA_CMP_DATA]);
211209
if (err < 0)
212210
return err;
213211

@@ -277,8 +275,11 @@ const struct nft_expr_ops nft_cmp_fast_ops = {
277275
static const struct nft_expr_ops *
278276
nft_cmp_select_ops(const struct nft_ctx *ctx, const struct nlattr * const tb[])
279277
{
280-
struct nft_data_desc desc;
281278
struct nft_data data;
279+
struct nft_data_desc desc = {
280+
.type = NFT_DATA_VALUE,
281+
.size = sizeof(data),
282+
};
282283
enum nft_cmp_ops op;
283284
int err;
284285

@@ -300,21 +301,14 @@ nft_cmp_select_ops(const struct nft_ctx *ctx, const struct nlattr * const tb[])
300301
return ERR_PTR(-EINVAL);
301302
}
302303

303-
err = nft_data_init(NULL, &data, sizeof(data), &desc,
304-
tb[NFTA_CMP_DATA]);
304+
err = nft_data_init(NULL, &data, &desc, tb[NFTA_CMP_DATA]);
305305
if (err < 0)
306306
return ERR_PTR(err);
307307

308-
if (desc.type != NFT_DATA_VALUE)
309-
goto err1;
310-
311308
if (desc.len <= sizeof(u32) && (op == NFT_CMP_EQ || op == NFT_CMP_NEQ))
312309
return &nft_cmp_fast_ops;
313310

314311
return &nft_cmp_ops;
315-
err1:
316-
nft_data_release(&data, desc.type);
317-
return ERR_PTR(-EINVAL);
318312
}
319313

320314
struct nft_expr_type nft_cmp_type __read_mostly = {

0 commit comments

Comments
 (0)