netdev - Re: [stmmac][bug?] endianness of Flexible RX Parser code

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] [day] [month] [year] [list]

Message-ID: <d0fea751-a995-acb4-a1df-15f93a8b9039@synopsys.com>
Date:   Mon, 6 Aug 2018 09:56:50 +0100
From:   Jose Abreu <Jose.Abreu@...opsys.com>
To:     Al Viro <viro@...IV.linux.org.uk>,
        Jose Abreu <Jose.Abreu@...opsys.com>
CC:     "David S. Miller" <davem@...emloft.net>, <netdev@...r.kernel.org>
Subject: Re: [stmmac][bug?] endianness of Flexible RX Parser code

Hi Al,

On 04-08-2018 02:19, Al Viro wrote:
> 	The values passed in struct tc_u32_sel ->mask and ->val are
> 32bit net-endian.  Your tc_fill_entry() does this:
>
>         data = sel->keys[0].val;
>         mask = sel->keys[0].mask;
>
> ...
>                 entry->frag_ptr = frag;
>                 entry->val.match_en = (mask << (rem * 8)) &
>                         GENMASK(31, rem * 8);
>                 entry->val.match_data = (data << (rem * 8)) &
>                         GENMASK(31, rem * 8);
>                 entry->val.frame_offset = real_off;
>                 entry->prio = prio;
>
>                 frag->val.match_en = (mask >> (rem * 8)) &
>                         GENMASK(rem * 8 - 1, 0);
>                 frag->val.match_data = (data >> (rem * 8)) &
>                         GENMASK(rem * 8 - 1, 0);
>                 frag->val.frame_offset = real_off + 1;
>                 frag->prio = prio;
>                 frag->is_frag = true;
>
> and that looks very odd.  rem here is offset modulo 4.  Suppose offset is
> equal to 5, val contains {V0, V1, V2, V3} and mask - {M0, M1, M2, M3}.
> Then on little-endian host we get
> entry->val.match_en:	{0, M0, M1, M2}
> entry->val.match_data:	{0, V0, V1, V2}
> entry->val.frame_offset = 1;
> frag->val.match_en:	{M3, 0, 0, 0}
> frag->val.match_data:	{V3, 0, 0, 0}
> frag->val.frame_offset = 2;
> and on big-endian
> entry->val.match_en:	{M1, M2, M3, 0}
> entry->val.match_data:	{V1, V2, V3, 0}
> entry->val.frame_offset = 1;
> frag->val.match_en:	{0, 0, 0, M0}
> frag->val.match_data:	{0, 0, 0, V0}
> frag->val.frame_offset = 2;
>
> Little-endian variant looks like we mask octets 5, 6, 7 and 8 with
> M0..M3 resp. and want V0..V3 in those.  On big-endian, though, we
> look at the octets 11, 4, 5 and 6 instead.
>
> I don't know the hardware (and it might be pulling any kind of weird
> endianness-dependent stunts), but that really smells like a bug.

There is a feature in HW that supports Byte-Invariant Big-Endian
data transfer. It's not activated by default though ...

> It looks like that code is trying to do something like
>
>         data = ntohl(sel->keys[0].val);
>         mask = ntohl(sel->keys[0].mask);
> 	shift = rem * 8;
>
> 	entry->val.match_en = htonl(mask >> shift);
> 	entry->val.match_data = htonl(data >> shift);
> 	entry->val.frame_offset = real_off;
> 	...
> 	frag->val.match_en = htonl(mask << (32 - shift));
> 	frag->val.match_data = htonl(data << (32 - shift));
> 	entry->val.frame_offset = real_off + 1;
>
> Comments?

Looks good. Can you send a formal patch and a simple command that
I can use to validate this situation?

I used at the time at least two commands: one for validating
simple match by using 1 entry (i.e. plain match) and another to
validate 2 entries (i.e. two matches) and did not encounter any
problem ...

Thanks and Best Regards,
Jose Miguel Abreu