Skip to content

Commit 341b694

Browse files
ummakynesummakynes
committed
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]>
1 parent 1349416 commit 341b694

File tree

6 files changed

+126
-115
lines changed

6 files changed

+126
-115
lines changed

‎include/net/netfilter/nf_tables.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -223,11 +223,11 @@ struct nft_ctx {
223223

224224
struct nft_data_desc {
225225
enum nft_data_types type;
226+
unsigned int size;
226227
unsigned int len;
227228
};
228229

229-
int nft_data_init(const struct nft_ctx *ctx,
230-
struct nft_data *data, unsigned int size,
230+
int nft_data_init(const struct nft_ctx *ctx, struct nft_data *data,
231231
struct nft_data_desc *desc, const struct nlattr *nla);
232232
void nft_data_hold(const struct nft_data *data, enum nft_data_types type);
233233
void nft_data_release(const struct nft_data *data, enum nft_data_types type);

‎net/netfilter/nf_tables_api.c‎

Lines changed: 40 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -5202,19 +5202,13 @@ static int nft_setelem_parse_flags(const struct nft_set *set,
52025202
static int nft_setelem_parse_key(struct nft_ctx *ctx, struct nft_set *set,
52035203
struct nft_data *key, struct nlattr *attr)
52045204
{
5205-
struct nft_data_desc desc;
5206-
int err;
5207-
5208-
err = nft_data_init(ctx, key, NFT_DATA_VALUE_MAXLEN, &desc, attr);
5209-
if (err < 0)
5210-
return err;
5211-
5212-
if (desc.type != NFT_DATA_VALUE || desc.len != set->klen) {
5213-
nft_data_release(key, desc.type);
5214-
return -EINVAL;
5215-
}
5205+
struct nft_data_desc desc = {
5206+
.type = NFT_DATA_VALUE,
5207+
.size = NFT_DATA_VALUE_MAXLEN,
5208+
.len = set->klen,
5209+
};
52165210

5217-
return 0;
5211+
return nft_data_init(ctx, key, &desc, attr);
52185212
}
52195213

52205214
static int nft_setelem_parse_data(struct nft_ctx *ctx, struct nft_set *set,
@@ -5223,24 +5217,17 @@ static int nft_setelem_parse_data(struct nft_ctx *ctx, struct nft_set *set,
52235217
struct nlattr *attr)
52245218
{
52255219
u32 dtype;
5226-
int err;
5227-
5228-
err = nft_data_init(ctx, data, NFT_DATA_VALUE_MAXLEN, desc, attr);
5229-
if (err < 0)
5230-
return err;
52315220

52325221
if (set->dtype == NFT_DATA_VERDICT)
52335222
dtype = NFT_DATA_VERDICT;
52345223
else
52355224
dtype = NFT_DATA_VALUE;
52365225

5237-
if (dtype != desc->type ||
5238-
set->dlen != desc->len) {
5239-
nft_data_release(data, desc->type);
5240-
return -EINVAL;
5241-
}
5226+
desc->type = dtype;
5227+
desc->size = NFT_DATA_VALUE_MAXLEN;
5228+
desc->len = set->dlen;
52425229

5243-
return 0;
5230+
return nft_data_init(ctx, data, desc, attr);
52445231
}
52455232

52465233
static void *nft_setelem_catchall_get(const struct net *net,
@@ -9685,7 +9672,7 @@ static int nft_verdict_init(const struct nft_ctx *ctx, struct nft_data *data,
96859672
}
96869673

96879674
desc->len = sizeof(data->verdict);
9688-
desc->type = NFT_DATA_VERDICT;
9675+
96899676
return 0;
96909677
}
96919678

@@ -9738,20 +9725,25 @@ int nft_verdict_dump(struct sk_buff *skb, int type, const struct nft_verdict *v)
97389725
}
97399726

97409727
static int nft_value_init(const struct nft_ctx *ctx,
9741-
struct nft_data *data, unsigned int size,
9742-
struct nft_data_desc *desc, const struct nlattr *nla)
9728+
struct nft_data *data, struct nft_data_desc *desc,
9729+
const struct nlattr *nla)
97439730
{
97449731
unsigned int len;
97459732

97469733
len = nla_len(nla);
97479734
if (len == 0)
97489735
return -EINVAL;
9749-
if (len > size)
9736+
if (len > desc->size)
97509737
return -EOVERFLOW;
9738+
if (desc->len) {
9739+
if (len != desc->len)
9740+
return -EINVAL;
9741+
} else {
9742+
desc->len = len;
9743+
}
97519744

97529745
nla_memcpy(data->data, nla, len);
9753-
desc->type = NFT_DATA_VALUE;
9754-
desc->len = len;
9746+
97559747
return 0;
97569748
}
97579749

@@ -9771,7 +9763,6 @@ static const struct nla_policy nft_data_policy[NFTA_DATA_MAX + 1] = {
97719763
*
97729764
* @ctx: context of the expression using the data
97739765
* @data: destination struct nft_data
9774-
* @size: maximum data length
97759766
* @desc: data description
97769767
* @nla: netlink attribute containing data
97779768
*
@@ -9781,24 +9772,35 @@ static const struct nla_policy nft_data_policy[NFTA_DATA_MAX + 1] = {
97819772
* The caller can indicate that it only wants to accept data of type
97829773
* NFT_DATA_VALUE by passing NULL for the ctx argument.
97839774
*/
9784-
int nft_data_init(const struct nft_ctx *ctx,
9785-
struct nft_data *data, unsigned int size,
9775+
int nft_data_init(const struct nft_ctx *ctx, struct nft_data *data,
97869776
struct nft_data_desc *desc, const struct nlattr *nla)
97879777
{
97889778
struct nlattr *tb[NFTA_DATA_MAX + 1];
97899779
int err;
97909780

9781+
if (WARN_ON_ONCE(!desc->size))
9782+
return -EINVAL;
9783+
97919784
err = nla_parse_nested_deprecated(tb, NFTA_DATA_MAX, nla,
97929785
nft_data_policy, NULL);
97939786
if (err < 0)
97949787
return err;
97959788

9796-
if (tb[NFTA_DATA_VALUE])
9797-
return nft_value_init(ctx, data, size, desc,
9798-
tb[NFTA_DATA_VALUE]);
9799-
if (tb[NFTA_DATA_VERDICT] && ctx != NULL)
9800-
return nft_verdict_init(ctx, data, desc, tb[NFTA_DATA_VERDICT]);
9801-
return -EINVAL;
9789+
if (tb[NFTA_DATA_VALUE]) {
9790+
if (desc->type != NFT_DATA_VALUE)
9791+
return -EINVAL;
9792+
9793+
err = nft_value_init(ctx, data, desc, tb[NFTA_DATA_VALUE]);
9794+
} else if (tb[NFTA_DATA_VERDICT] && ctx != NULL) {
9795+
if (desc->type != NFT_DATA_VERDICT)
9796+
return -EINVAL;
9797+
9798+
err = nft_verdict_init(ctx, data, desc, tb[NFTA_DATA_VERDICT]);
9799+
} else {
9800+
err = -EINVAL;
9801+
}
9802+
9803+
return err;
98029804
}
98039805
EXPORT_SYMBOL_GPL(nft_data_init);
98049806

‎net/netfilter/nft_bitwise.c‎

Lines changed: 33 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,37 +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 err_mask_release;
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 err_mask_release;
119-
if (xor.type != NFT_DATA_VALUE || xor.len != priv->len) {
120-
err = -EINVAL;
121-
goto err_xor_release;
122-
}
121+
goto err_xor_err;
123122

124123
return 0;
125124

126-
err_xor_release:
127-
nft_data_release(&priv->xor, xor.type);
128-
err_mask_release:
125+
err_xor_err:
129126
nft_data_release(&priv->mask, mask.type);
127+
130128
return err;
131129
}
132130

133131
static int nft_bitwise_init_shift(struct nft_bitwise *priv,
134132
const struct nlattr *const tb[])
135133
{
136-
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+
};
137139
int err;
138140

139141
if (tb[NFTA_BITWISE_MASK] ||
@@ -143,13 +145,12 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
143145
if (!tb[NFTA_BITWISE_DATA])
144146
return -EINVAL;
145147

146-
err = nft_data_init(NULL, &priv->data, sizeof(priv->data), &d,
147-
tb[NFTA_BITWISE_DATA]);
148+
err = nft_data_init(NULL, &priv->data, &desc, tb[NFTA_BITWISE_DATA]);
148149
if (err < 0)
149150
return err;
150-
if (d.type != NFT_DATA_VALUE || d.len != sizeof(u32) ||
151-
priv->data.data[0] >= BITS_PER_TYPE(u32)) {
152-
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);
153154
return -EINVAL;
154155
}
155156

@@ -339,22 +340,21 @@ static const struct nft_expr_ops nft_bitwise_ops = {
339340
static int
340341
nft_bitwise_extract_u32_data(const struct nlattr * const tb, u32 *out)
341342
{
342-
struct nft_data_desc desc;
343343
struct nft_data data;
344-
int err = 0;
344+
struct nft_data_desc desc = {
345+
.type = NFT_DATA_VALUE,
346+
.size = sizeof(data),
347+
.len = sizeof(u32),
348+
};
349+
int err;
345350

346-
err = nft_data_init(NULL, &data, sizeof(data), &desc, tb);
351+
err = nft_data_init(NULL, &data, &desc, tb);
347352
if (err < 0)
348353
return err;
349354

350-
if (desc.type != NFT_DATA_VALUE || desc.len != sizeof(u32)) {
351-
err = -EINVAL;
352-
goto err;
353-
}
354355
*out = data.data[0];
355-
err:
356-
nft_data_release(&data, desc.type);
357-
return err;
356+
357+
return 0;
358358
}
359359

360360
static int nft_bitwise_fast_init(const struct nft_ctx *ctx,

‎net/netfilter/nft_cmp.c‎

Lines changed: 20 additions & 24 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;
@@ -214,12 +210,14 @@ static int nft_cmp_fast_init(const struct nft_ctx *ctx,
214210
const struct nlattr * const tb[])
215211
{
216212
struct nft_cmp_fast_expr *priv = nft_expr_priv(expr);
217-
struct nft_data_desc desc;
218213
struct nft_data data;
214+
struct nft_data_desc desc = {
215+
.type = NFT_DATA_VALUE,
216+
.size = sizeof(data),
217+
};
219218
int err;
220219

221-
err = nft_data_init(NULL, &data, sizeof(data), &desc,
222-
tb[NFTA_CMP_DATA]);
220+
err = nft_data_init(NULL, &data, &desc, tb[NFTA_CMP_DATA]);
223221
if (err < 0)
224222
return err;
225223

@@ -313,11 +311,13 @@ static int nft_cmp16_fast_init(const struct nft_ctx *ctx,
313311
const struct nlattr * const tb[])
314312
{
315313
struct nft_cmp16_fast_expr *priv = nft_expr_priv(expr);
316-
struct nft_data_desc desc;
314+
struct nft_data_desc desc = {
315+
.type = NFT_DATA_VALUE,
316+
.size = sizeof(priv->data),
317+
};
317318
int err;
318319

319-
err = nft_data_init(NULL, &priv->data, sizeof(priv->data), &desc,
320-
tb[NFTA_CMP_DATA]);
320+
err = nft_data_init(NULL, &priv->data, &desc, tb[NFTA_CMP_DATA]);
321321
if (err < 0)
322322
return err;
323323

@@ -380,8 +380,11 @@ const struct nft_expr_ops nft_cmp16_fast_ops = {
380380
static const struct nft_expr_ops *
381381
nft_cmp_select_ops(const struct nft_ctx *ctx, const struct nlattr * const tb[])
382382
{
383-
struct nft_data_desc desc;
384383
struct nft_data data;
384+
struct nft_data_desc desc = {
385+
.type = NFT_DATA_VALUE,
386+
.size = sizeof(data),
387+
};
385388
enum nft_cmp_ops op;
386389
u8 sreg;
387390
int err;
@@ -404,14 +407,10 @@ nft_cmp_select_ops(const struct nft_ctx *ctx, const struct nlattr * const tb[])
404407
return ERR_PTR(-EINVAL);
405408
}
406409

407-
err = nft_data_init(NULL, &data, sizeof(data), &desc,
408-
tb[NFTA_CMP_DATA]);
410+
err = nft_data_init(NULL, &data, &desc, tb[NFTA_CMP_DATA]);
409411
if (err < 0)
410412
return ERR_PTR(err);
411413

412-
if (desc.type != NFT_DATA_VALUE)
413-
goto err1;
414-
415414
sreg = ntohl(nla_get_be32(tb[NFTA_CMP_SREG]));
416415

417416
if (op == NFT_CMP_EQ || op == NFT_CMP_NEQ) {
@@ -423,9 +422,6 @@ nft_cmp_select_ops(const struct nft_ctx *ctx, const struct nlattr * const tb[])
423422
return &nft_cmp16_fast_ops;
424423
}
425424
return &nft_cmp_ops;
426-
err1:
427-
nft_data_release(&data, desc.type);
428-
return ERR_PTR(-EINVAL);
429425
}
430426

431427
struct nft_expr_type nft_cmp_type __read_mostly = {

0 commit comments

Comments
 (0)