netdev - Re: [PATCH net-next] tcp: md5: do not use stack storage in crypto operations

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: <CALCETrXk1O2SatvZYN_TGOCN4M95oCdWLwH3JC1TPjAp+cu1=g@mail.gmail.com>
Date:	Mon, 27 Jun 2016 08:02:43 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	David Miller <davem@...emloft.net>
Cc:	Eric Dumazet <eric.dumazet@...il.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] tcp: md5: do not use stack storage in crypto operations

On Mon, Jun 27, 2016 at 7:51 AM, David Miller <davem@...emloft.net> wrote:
> From: Eric Dumazet <eric.dumazet@...il.com>
> Date: Sat, 25 Jun 2016 18:09:35 +0200
>
>> From: Eric Dumazet <edumazet@...gle.com>
>>
>> Some arches have virtually mapped kernel stacks, or will soon have.
>>
>> tcp_md5_hash_header() uses an automatic variable to copy tcp header
>> before mangling th->check and calling crypto function, which might
>> be problematic on such arches.
>>
>> So use percpu storage as we already do for the pseudo header,
>> and reduce number of crypto functions calls, as these headers
>> are ridiculously small.
>>
>> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
>> Reported-by: Andy Lutomirski <luto@...capital.net>
>> ---
>> I am not sure the md5 crypto functions have a problem with data backed
>> in virtually mapped stacks, but this patch seems to remove the doubt.
>
> In a non-SMP build this storage will be in the kernel image itself.
>
> And this violates the crypto layer's requirements.
>
> It has to be memory taken from the page allocator or kmalloc.  So
> before vmalloc'd stacks, the current code is perfectly fine.  And
> with vmalloc'd stacks this patch trades one bug for another.

After poking around a bit: could this use crypto_shash_init +
crypto_shash_finup?  Those functions seem like they'll be just fine
with any virtual address, and I bet they're considerably faster than
using the ahash API.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC