nft: meta l4proto range printing broken on 32bit

nft: meta l4proto range printing broken on 32bit

[Florian Westphal]

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.

Re: nft: meta l4proto range printing broken on 32bit

[Florian Westphal]

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."

Re: nft: meta l4proto range printing broken on 32bit

[Pablo Neira Ayuso]

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.

Re: nft: meta l4proto range printing broken on 32bit

[Pablo Neira Ayuso]

[-- 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 &&

Re: nft: meta l4proto range printing broken on 32bit

[Florian Westphal]

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!