netdev - Re: [PATCH v4 01/21] net/tcp: Prepare tcp_md5sig

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: <bd40ff2f-b015-4ed4-7755-f9d547c8b868@arista.com>
Date:   Mon, 20 Feb 2023 16:57:20 +0000
From:   Dmitry Safonov <dima@...sta.com>
To:     Herbert Xu <herbert@...dor.apana.org.au>
Cc:     linux-kernel@...r.kernel.org, David Ahern <dsahern@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Andy Lutomirski <luto@...capital.net>,
        Ard Biesheuvel <ardb@...nel.org>,
        Bob Gilligan <gilligan@...sta.com>,
        David Laight <David.Laight@...lab.com>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Eric Biggers <ebiggers@...nel.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Francesco Ruggeri <fruggeri05@...il.com>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Ivan Delalande <colona@...sta.com>,
        Leonard Crestez <cdleonard@...il.com>,
        Salam Noureddine <noureddine@...sta.com>,
        netdev@...r.kernel.org
Subject: Re: [PATCH v4 01/21] net/tcp: Prepare tcp_md5sig_pool for TCP-AO

Hi Herbert,

On 2/20/23 09:41, Herbert Xu wrote:
> On Wed, Feb 15, 2023 at 06:33:15PM +0000, Dmitry Safonov wrote:
>> TCP-AO similarly to TCP-MD5 needs to allocate tfms on a slow-path, which
>> is setsockopt() and use crypto ahash requests on fast paths, which are
>> RX/TX softirqs. It as well needs a temporary/scratch buffer for
>> preparing the hashing request.
>>
>> Extend tcp_md5sig_pool to support other hashing algorithms than MD5.
>> Move it in a separate file.
>>
>> This patch was previously submitted as more generic crypto_pool [1],
>> but Herbert nacked making it generic crypto API. His view is that crypto
>> requests should be atomically allocated on fast-paths. So, in this
>> version I don't move this pool anywhere outside TCP, only extending it
>> for TCP-AO use-case. It can be converted once there will be per-request
>> hashing crypto keys.
>>
>> [1]: https://lore.kernel.org/all/20230118214111.394416-1-dima@arista.com/T/#u
>> Signed-off-by: Dmitry Safonov <dima@...sta.com>
>> ---
>>  include/net/tcp.h        |  48 ++++--
>>  net/ipv4/Kconfig         |   4 +
>>  net/ipv4/Makefile        |   1 +
>>  net/ipv4/tcp.c           | 103 +++---------
>>  net/ipv4/tcp_ipv4.c      |  97 +++++++-----
>>  net/ipv4/tcp_minisocks.c |  21 ++-
>>  net/ipv4/tcp_sigpool.c   | 333 +++++++++++++++++++++++++++++++++++++++
>>  net/ipv6/tcp_ipv6.c      |  58 +++----
>>  8 files changed, 493 insertions(+), 172 deletions(-)
>>  create mode 100644 net/ipv4/tcp_sigpool.c
> 
> Please wait for my per-request hash work before you resubmit this.

Do you have a timeline for that work?
And if you don't mind I keep re-iterating, as I'm trying to address TCP
reviews and missed functionality/selftests.

> Once that's in place all you need is a single tfm for the whole
> system.

Unfortunately, not really: RFC5926 prescribes the mandatory-to-implement
MAC algorithms for TCP-AO: HMAC-SHA-1-96 and AES-128-CMAC-96. But since
the RFC was written sha1 is now more eligible for attacks as well as
RFC5925 has:
> The option should support algorithms other than the default, to
> allow agility over time.
> TCP-AO allows any desired algorithm, subject to TCP option
> space limitations, as noted in Section 2.2. The use of a set
> of MKTs allows separate connections to use different
> algorithms, both for the MAC and the KDF.

As well as from a customer's request we need to support more than two
required algorithms. So, this implementation let the user choose the
algorithm that is supported by crypto/ layer (more or less like xfrm does).

Which means, that it still has to support multiple tfms. I guess that
pool of tfms can be converted to use per-request keys quite easily.

> As to request pools what exactly is the point of that? Just kmalloc
> them on demand.

1) before your per-request key patches - it's not possible.
2) after your patches - my question would be: "is it better to
kmalloc(GFP_ATOMIC) in RX/TX for every signed TCP segment, rather than
pre-allocate it?"

The price of (2) may just well be negligible, but worth measuring before
switching.

Thanks,
          Dmitry