* nft: meta l4proto range printing broken on 32bit
@ 2015-07-16 13:39 Florian Westphal
2015-07-16 13:42 ` Florian Westphal
2015-07-16 16:54 ` Pablo Neira Ayuso
0 siblings, 2 replies; 5+ messages in thread
From: Florian Westphal @ 2015-07-16 13:39 UTC (permalink / raw)
To: netfilter-devel
Hi Pablo
09565a4b1ed4863d44c4509a93c50f44efd12771
(netlink_delinearize: consolidate range printing) causes nft
to segfault on 32bit machine when printing l4proto ranges.
The problem is that meta_expr_pctx_update() assumes that
right is a value, but after this change it can also be a value.
Thus, expr->value contents are undefined (its union).
On x86_64 this is also broken but by virtue of struct layout
and pointer sizes, value->_mp_size will almost always be 0
so mpz_get_uint8() returns 0.
But on x86-32 _mp_size will be huge value (contains expr->right pointer
of range), so we crash in libgmp.
This fixes it for me:
diff --git a/src/meta.c b/src/meta.c
index bfc1258..e6d69e3 100644
--- a/src/meta.c
+++ b/src/meta.c
@@ -462,7 +462,7 @@ static void meta_expr_pctx_update(struct proto_ctx *ctx,
{
const struct hook_proto_desc *h = &hook_proto_desc[ctx->family];
const struct expr *left = expr->left, *right = expr->right;
- const struct proto_desc *desc;
+ const struct proto_desc *desc = NULL;
assert(expr->op == OP_EQ);
@@ -485,8 +485,10 @@ static void meta_expr_pctx_update(struct proto_ctx *ctx,
proto_ctx_update(ctx, PROTO_BASE_NETWORK_HDR, &expr->location, desc);
break;
case NFT_META_L4PROTO:
- desc = proto_find_upper(&proto_inet_service,
- mpz_get_uint8(right->value));
+ if (right->ops->type == EXPR_VALUE)
+ desc = proto_find_upper(&proto_inet_service,
+ mpz_get_uint8(right->value));
+
if (desc == NULL)
desc = &proto_unknown;
If you agree, I'll push this fix to nftables.
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: nft: meta l4proto range printing broken on 32bit
2015-07-16 13:39 nft: meta l4proto range printing broken on 32bit Florian Westphal
@ 2015-07-16 13:42 ` Florian Westphal
2015-07-16 16:54 ` Pablo Neira Ayuso
1 sibling, 0 replies; 5+ messages in thread
From: Florian Westphal @ 2015-07-16 13:42 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
Florian Westphal <fw@strlen.de> wrote:
[ nft crashes on 32bit when trying to print meta l4proto 23-42 ]
> The problem is that meta_expr_pctx_update() assumes that
> right is a value, but after this change it can also be a value.
Grrr, seems I need a break. Meant "... can now also be EXPR_RANGE."
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: nft: meta l4proto range printing broken on 32bit
2015-07-16 13:39 nft: meta l4proto range printing broken on 32bit Florian Westphal
2015-07-16 13:42 ` Florian Westphal
@ 2015-07-16 16:54 ` Pablo Neira Ayuso
2015-07-16 16:54 ` Pablo Neira Ayuso
1 sibling, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2015-07-16 16:54 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
On Thu, Jul 16, 2015 at 03:39:29PM +0200, Florian Westphal wrote:
> Hi Pablo
>
> 09565a4b1ed4863d44c4509a93c50f44efd12771
> (netlink_delinearize: consolidate range printing) causes nft
> to segfault on 32bit machine when printing l4proto ranges.
>
> The problem is that meta_expr_pctx_update() assumes that
> right is a value, but after this change it can also be a value.
>
> Thus, expr->value contents are undefined (its union).
> On x86_64 this is also broken but by virtue of struct layout
> and pointer sizes, value->_mp_size will almost always be 0
> so mpz_get_uint8() returns 0.
>
> But on x86-32 _mp_size will be huge value (contains expr->right pointer
> of range), so we crash in libgmp.
Good catch. Could you give a try to this patch instead?
We shouldn't call pctx_update(), before the transformation we had
there a expr->op == { OP_GT, OP_GTE, OP_LT, OP_LTE }. So we never
entered that path as the assert in payload_expr_pctx_update()
indicates.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: nft: meta l4proto range printing broken on 32bit
2015-07-16 16:54 ` Pablo Neira Ayuso
@ 2015-07-16 16:54 ` Pablo Neira Ayuso
2015-07-16 20:29 ` Florian Westphal
0 siblings, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2015-07-16 16:54 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel, kaber
[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]
On Thu, Jul 16, 2015 at 06:54:05PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Jul 16, 2015 at 03:39:29PM +0200, Florian Westphal wrote:
> > Hi Pablo
> >
> > 09565a4b1ed4863d44c4509a93c50f44efd12771
> > (netlink_delinearize: consolidate range printing) causes nft
> > to segfault on 32bit machine when printing l4proto ranges.
> >
> > The problem is that meta_expr_pctx_update() assumes that
> > right is a value, but after this change it can also be a value.
> >
> > Thus, expr->value contents are undefined (its union).
> > On x86_64 this is also broken but by virtue of struct layout
> > and pointer sizes, value->_mp_size will almost always be 0
> > so mpz_get_uint8() returns 0.
> >
> > But on x86-32 _mp_size will be huge value (contains expr->right pointer
> > of range), so we crash in libgmp.
>
> Good catch. Could you give a try to this patch instead?
>
> We shouldn't call pctx_update(), before the transformation we had
> there a expr->op == { OP_GT, OP_GTE, OP_LT, OP_LTE }. So we never
> entered that path as the assert in payload_expr_pctx_update()
> indicates.
Forgot patch, here it comes.
[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 433 bytes --]
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 6d60be3..4226b82 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -983,6 +983,9 @@ static void meta_match_postprocess(struct rule_pp_ctx *ctx,
switch (expr->op) {
case OP_EQ:
+ if (expr->right->ops->type == EXPR_RANGE)
+ break;
+
expr->left->ops->pctx_update(&ctx->pctx, expr);
if (ctx->pbase == PROTO_BASE_INVALID &&
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: nft: meta l4proto range printing broken on 32bit
2015-07-16 16:54 ` Pablo Neira Ayuso
@ 2015-07-16 20:29 ` Florian Westphal
0 siblings, 0 replies; 5+ messages in thread
From: Florian Westphal @ 2015-07-16 20:29 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel, kaber
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Jul 16, 2015 at 06:54:05PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Jul 16, 2015 at 03:39:29PM +0200, Florian Westphal wrote:
> > > Hi Pablo
> > >
> > > 09565a4b1ed4863d44c4509a93c50f44efd12771
> > > (netlink_delinearize: consolidate range printing) causes nft
> > > to segfault on 32bit machine when printing l4proto ranges.
> > >
> > > The problem is that meta_expr_pctx_update() assumes that
> > > right is a value, but after this change it can also be a value.
> > >
> > > Thus, expr->value contents are undefined (its union).
> > > On x86_64 this is also broken but by virtue of struct layout
> > > and pointer sizes, value->_mp_size will almost always be 0
> > > so mpz_get_uint8() returns 0.
> > >
> > > But on x86-32 _mp_size will be huge value (contains expr->right pointer
> > > of range), so we crash in libgmp.
> >
> > Good catch. Could you give a try to this patch instead?
> >
> > We shouldn't call pctx_update(), before the transformation we had
> > there a expr->op == { OP_GT, OP_GTE, OP_LT, OP_LTE }. So we never
> > entered that path as the assert in payload_expr_pctx_update()
> > indicates.
>
> Forgot patch, here it comes.
> diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
> index 6d60be3..4226b82 100644
> --- a/src/netlink_delinearize.c
> +++ b/src/netlink_delinearize.c
> @@ -983,6 +983,9 @@ static void meta_match_postprocess(struct rule_pp_ctx *ctx,
>
> switch (expr->op) {
> case OP_EQ:
> + if (expr->right->ops->type == EXPR_RANGE)
> + break;
> +
> expr->left->ops->pctx_update(&ctx->pctx, expr);
>
Right thats better -- and it also avoids the crash.
nft testsuite now passes with a few warnings and no crashes.
Thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-07-16 20:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-16 13:39 nft: meta l4proto range printing broken on 32bit Florian Westphal
2015-07-16 13:42 ` Florian Westphal
2015-07-16 16:54 ` Pablo Neira Ayuso
2015-07-16 16:54 ` Pablo Neira Ayuso
2015-07-16 20:29 ` Florian Westphal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).