Re: [PATCH nftables 1/4] src: fix jumps on bigendian arches

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Tue, Aug 13, 2019 at 08:44:06PM +0200, Florian Westphal wrote:
> table bla {
>   chain foo { }
>   chain bar { jump foo }
>  }
> }
> 
> Fails to restore on big-endian platforms:
> jump.nft:5:2-9: Error: Could not process rule: No such file or directory
>  jump foo
> 
> nft passes a 0-length name to the kernel.
> 
> This is because when we export the value (the string), we provide
> the size of the destination buffer.
> 
> In earlier versions, the parser allocated the name with the same
> fixed size and all was fine.
> 
> After the fix, the export places the name in the wrong location
> in the destination buffer.
> 
> This makes tests/shell/testcases/chains/0001jumps_0 work on s390x.
> 
> Fixes: 142350f154c78 ("src: invalid read when importing chain name")
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  src/datatype.c | 26 +++++++++++++++++---------
>  src/netlink.c  | 16 +++++++++++++---
>  2 files changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/src/datatype.c b/src/datatype.c
> index 28f726f4e84c..6908bc22d783 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -244,10 +244,24 @@ const struct datatype invalid_type = {
>  	.print		= invalid_type_print,
>  };
>  
> -static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
> +static void verdict_jump_chain_print(const char *what, const struct expr *e,
> +				     struct output_ctx *octx)
>  {
>  	char chain[NFT_CHAIN_MAXNAMELEN];

Probably:

        chat chain[NFT_CHAIN_MAXNAMELEN + 1] = {};

to ensure space for \0.

> +	unsigned int len;
> +
> +	memset(chain, 0, sizeof(chain));

remove this memset then.

> +	len = e->len / BITS_PER_BYTE;

        div_round_up() ?

> +	if (len >= sizeof(chain))
> +		len = sizeof(chain) - 1;

Probably BUG() here instead if e->len > NFT_CHAIN_MAXNAMELEN? This
should not happen.

> +
> +	mpz_export_data(chain, e->value, BYTEORDER_HOST_ENDIAN, len);
> +	nft_print(octx, "%s %s", what, chain);
> +}
> +
> +static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
> +{
>  	switch (expr->verdict) {
>  	case NFT_CONTINUE:
>  		nft_print(octx, "continue");
> @@ -257,10 +271,7 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
>  		break;
>  	case NFT_JUMP:
>  		if (expr->chain->etype == EXPR_VALUE) {
> -			mpz_export_data(chain, expr->chain->value,
> -					BYTEORDER_HOST_ENDIAN,
> -					NFT_CHAIN_MAXNAMELEN);
> -			nft_print(octx, "jump %s", chain);
> +			verdict_jump_chain_print("jump", expr->chain, octx);
>  		} else {
>  			nft_print(octx, "jump ");
>  			expr_print(expr->chain, octx);
> @@ -268,10 +279,7 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
>  		break;
>  	case NFT_GOTO:
>  		if (expr->chain->etype == EXPR_VALUE) {
> -			mpz_export_data(chain, expr->chain->value,
> -					BYTEORDER_HOST_ENDIAN,
> -					NFT_CHAIN_MAXNAMELEN);
> -			nft_print(octx, "goto %s", chain);
> +			verdict_jump_chain_print("goto", expr->chain, octx);
>  		} else {
>  			nft_print(octx, "goto ");
>  			expr_print(expr->chain, octx);
> diff --git a/src/netlink.c b/src/netlink.c
> index aeeb12eaca93..f8e1120447d9 100644
> --- a/src/netlink.c
> +++ b/src/netlink.c
> @@ -222,17 +222,27 @@ static void netlink_gen_verdict(const struct expr *expr,
>  				struct nft_data_linearize *data)
>  {
>  	char chain[NFT_CHAIN_MAXNAMELEN];

        ...[NFT_CHAIN_MAXNAMELEN + 1] = {};

> +	unsigned int len;
>  
>  	data->verdict = expr->verdict;
>  
>  	switch (expr->verdict) {
>  	case NFT_JUMP:
>  	case NFT_GOTO:
> +		len = expr->chain->len / BITS_PER_BYTE;

                div_round_up()

> +
> +		if (!len)
> +			BUG("chain length is 0");
> +
> +		if (len > sizeof(chain))
> +			BUG("chain is too large (%u, %u max)",
> +			    len, (unsigned int)sizeof(chain));
> +
> +		memset(chain, 0, sizeof(chain));
> +
>  		mpz_export_data(chain, expr->chain->value,
> -				BYTEORDER_HOST_ENDIAN,
> -				NFT_CHAIN_MAXNAMELEN);
> +				BYTEORDER_HOST_ENDIAN, len);
>  		snprintf(data->chain, NFT_CHAIN_MAXNAMELEN, "%s", chain);
> -		data->chain[NFT_CHAIN_MAXNAMELEN-1] = '\0';
>  		break;
>  	}
>  }
> -- 
> 2.21.0
>