netdev - RE: [PATCH 05/10] netfilter: nf_tables: fix mismatch in big-endian system

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]

Date:   Thu, 16 Mar 2017 10:58:04 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Pablo Neira Ayuso' <pablo@...filter.org>,
        "netfilter-devel@...r.kernel.org" <netfilter-devel@...r.kernel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH 05/10] netfilter: nf_tables: fix mismatch in big-endian
 system

From: Pablo Neira Ayuso
> Sent: 15 March 2017 17:01
> From: Liping Zhang <zlpnobody@...il.com>
> 
> Currently, there are two different methods to store an u16 integer to
> the u32 data register. For example:
>   u32 *dest = &regs->data[priv->dreg];
>   1. *dest = 0; *(u16 *) dest = val_u16;
>   2. *dest = val_u16;
> 
> For method 1, the u16 value will be stored like this, either in
> big-endian or little-endian system:
>   0          15           31
>   +-+-+-+-+-+-+-+-+-+-+-+-+
>   |   Value   |     0     |
>   +-+-+-+-+-+-+-+-+-+-+-+-+
> 
> For method 2, in little-endian system, the u16 value will be the same
> as listed above. But in big-endian system, the u16 value will be stored
> like this:
>   0          15           31
>   +-+-+-+-+-+-+-+-+-+-+-+-+
>   |     0     |   Value   |
>   +-+-+-+-+-+-+-+-+-+-+-+-+
> 
> So later we use "memcmp(&regs->data[priv->sreg], data, 2);" to do
> compare in nft_cmp, nft_lookup expr ..., method 2 will get the wrong
> result in big-endian system, as 0~15 bits will always be zero.
> 
> For the similar reason, when loading an u16 value from the u32 data
> register, we should use "*(u16 *) sreg;" instead of "(u16)*sreg;",
> the 2nd method will get the wrong value in the big-endian system.
...

That seems to be papering over some of the obvious cracks.

The fact that the entire 32bits are zeroed makes me suspect that
in some places values are written as 8 bits and read later as 32.
The only way that can work on BE is if the values are always written
as 32 bit ones (assignment style 2).

OTOH using memcmp(,,2) relies on the data being in the lower addressed
bytes.

If the data does need to be in the lower addressed bytes I'd suggest
using a union rather than all the casts.

	David