netdev - Re: [PATCH nf] netfilter: nf_tables: nft_exthdr: the presence return value should be little-endian

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200803221534.GR29169@breakpoint.cc>
Date:   Tue, 4 Aug 2020 00:15:34 +0200
From:   Florian Westphal <fw@...len.de>
To:     Stephen Suryaputra <ssuryaextr@...il.com>
Cc:     netfilter-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH nf] netfilter: nf_tables: nft_exthdr: the presence return
 value should be little-endian

Stephen Suryaputra <ssuryaextr@...il.com> wrote:
> On big-endian machine, the returned register data when the exthdr is
> present is not being compared correctly because little-endian is
> assumed. The function nft_cmp_fast_mask(), called by nft_cmp_fast_eval()
> and nft_cmp_fast_init(), calls cpu_to_le32().
> 
> The following dump also shows that little endian is assumed:
> 
> $ nft --debug=netlink add rule ip recordroute forward ip option rr exists counter
> ip
>   [ exthdr load ipv4 1b @ 7 + 0 present => reg 1 ]
>   [ cmp eq reg 1 0x01000000 ]
>   [ counter pkts 0 bytes 0 ]
> 
> Lastly, debug print in nft_cmp_fast_init() and nft_cmp_fast_eval() when
> RR option exists in the packet shows that the comparison fails because
> the assumption:
> 
> nft_cmp_fast_init:189 priv->sreg=4 desc.len=8 mask=0xff000000 data.data[0]=0x10003e0
> nft_cmp_fast_eval:57 regs->data[priv->sreg=4]=0x1 mask=0xff000000 priv->data=0x1000000

Right, nft userspace assumes a boolean data type when it does existence
check.

> diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
> index 07782836fad6..50e4935585e3 100644
> --- a/net/netfilter/nft_exthdr.c
> +++ b/net/netfilter/nft_exthdr.c
> @@ -44,7 +44,7 @@ static void nft_exthdr_ipv6_eval(const struct nft_expr *expr,
>  
>  	err = ipv6_find_hdr(pkt->skb, &offset, priv->type, NULL, NULL);
>  	if (priv->flags & NFT_EXTHDR_F_PRESENT) {
> -		*dest = (err >= 0);
> +		*dest = cpu_to_le32(err >= 0);

Both should probably use nft_reg_store8(dst, err >= 0) for consistency
with the rest.

But the patch looks correct to me, thanks.