netdev - Re: [net PATCH] flow_dissector: Fix unaligned access in __skb_flow_dissector when used by eth_get

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:	Fri, 29 Jan 2016 20:46:00 -0800
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	David Miller <davem@...emloft.net>
Cc:	Eric Dumazet <eric.dumazet@...il.com>,
	Alex Duyck <aduyck@...antis.com>,
	Netdev <netdev@...r.kernel.org>,
	Sowmini Varadhan <sowmini.varadhan@...cle.com>,
	Tom Herbert <tom@...bertland.com>
Subject: Re: [net PATCH] flow_dissector: Fix unaligned access in
 __skb_flow_dissector when used by eth_get_headlen

On Fri, Jan 29, 2016 at 7:35 PM, David Miller <davem@...emloft.net> wrote:
> From: Eric Dumazet <eric.dumazet@...il.com>
> Date: Fri, 29 Jan 2016 19:23:54 -0800
>
>> On Fri, 2016-01-29 at 18:49 -0800, Alexander Duyck wrote:
>>> This patch fixes an issue with unaligned accesses when using
>>> eth_get_headlen on a page that was DMA aligned instead of being IP aligned.
>>> The fact is when trying to check the length we don't need to be looking at
>>> the flow label so we can reorder the checks to first check if we are
>>> supposed to gather the flow label and then make the call to actually get
>>> it.
>>>
>>> Reported-by: Sowmini Varadhan <sowmini.varadhan@...cle.com>
>>> Signed-off-by: Alexander Duyck <aduyck@...antis.com>
>>> ---
>>
>>
>> You did not quite follow the discussion Alexander, we did not exactly
>> took a decision about this bug.
>>
>> As we mentioned, there are other parts that need care.
>>
>> key_keyid->keyid = *keyid;
>>
>> Please address them, instead of having to 'wait' for the next crash.

The key_id bit is not part of the "basic" keys.  The basic keys only
require a 2 byte alignment, it is when you start parsing to populate a
hash that you get into the wider fields.  As far as I know there
aren't any length fields that are wider than 2 bytes.

> Indeed, this is a more fundamental issue.
>
> This change in and of itself isn't bad, and is probably a suitable
> micro-optimization for net-next, but it doesn't fix the bug in
> question.

Actually it does fix the bug as reported.  The problem as reported was
for ixgbe using the function for parsing the length.  In the case of
just getting the length my patch resolves the issue as there are no
other accesses wider than 2 bytes used when *just* computing the
length.  This is something that will have to get addressed sooner or
later anyway, and at least with this patch in the basic case of IPv6
over ixgbe won't cause any issues.

The key_id bit that Eric is pointing out is called in a path that is
only executed if you are actually attempting to compute a hash.  Odds
are the fix for that is going to be much more complicated and extends
well outside this function since it is a fundamental issue with VXLAN
and GRE TEB tunnels as either the inner or outer headers end up being
unaligned unless you want to break them into two buffers so that they
can both be aligned at the same time.

Extending beyond this has anyone taken a look at the transmit path for
these tunnels?  I would suspect we probably also have a number of
issues there since either the inner or outer IP headers have to be
unaligned when we are setting those up.

- Alex