Subject: Re: [PATCH 5/5] tcp/md5: Make more generic tcp_sig_pool
To:     Dmitry Safonov <0x7f454c46@gmail.com>,
        Eric Dumazet <edumazet@google.com>,
        David Ahern <dsahern@kernel.org>
Cc:     Andy Lutomirski <luto@amacapital.net>,
        "David S. Miller" <davem@davemloft.net>,
        Francesco Ruggeri <fruggeri@arista.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        linux-crypto@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Herbert Xu <herbert@gondor.apana.org.au>
References: <20211105014953.972946-1-dima@arista.com>
 <20211105014953.972946-6-dima@arista.com>
 <88edb8ff-532e-5662-cda7-c00904c612b4@gmail.com>
 <11215b43-cd3f-6cdc-36da-44636ca11f51@gmail.com>
From:   Leonard Crestez <cdleonard@gmail.com>
Message-ID: <4de0111a-af4e-779e-eda9-f0cda433dde5@gmail.com>
Date:   Fri, 5 Nov 2021 18:53:11 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <11215b43-cd3f-6cdc-36da-44636ca11f51@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 11/5/21 3:59 PM, Dmitry Safonov wrote:
> On 11/5/21 09:54, Leonard Crestez wrote:
>> On 11/5/21 3:49 AM, Dmitry Safonov wrote:
>>> Convert tcp_md5sig_pool to more generic tcp_sig_pool.
>>> Now tcp_sig_pool_alloc(const char *alg) can be used to allocate per-cpu
>>> ahash request for different hashing algorithms besides md5.
>>> tcp_sig_pool_get() and tcp_sig_pool_put() should be used to get
>>> ahash_request and scratch area.
>>
>> This pool pattern is a workaround for crypto-api only being able to
>> allocate transforms from user context.
>>> It would be useful for this "one-transform-per-cpu" object to be part of
>> crypto api itself, there is nothing TCP-specific here other than the
>> size of scratch buffer.
> 
> Agree, it would be nice to have something like this as a part of crypto.
> The intention here is to reuse md5 sig pool, rather than introduce
> another similar one.
> 
>>> Make tcp_sig_pool reusable for TCP Authentication Option support
>>> (TCP-AO, RFC5925), where RFC5926[1] requires HMAC-SHA1 and AES-128_CMAC
>>> hashing at least.
>> Additional work would be required to support options of arbitrary size
>> and I don't think anyone would use non-standard crypto algorithms.
>>
>> Is RFC5926 conformance really insufficient?
> 
> For the resulting hash, the scratch buffer can be used.
> 
> Honestly, I just don't see much benefit in introducing more code and
> structures in order to limit hash algorithms. If anything,
> 
> :if (strcmp("hmac(sha1)", opts.algo) && strcmp("cmac(aes)", opts.algo))
> :       return -EPROTONOSUPPORT;
> 
> and passing the string straight to crypto seems to be better than adding
> new structures.
> 
> On the other side, those two hashes MUST be supported to comply with
> RFC, other may. As user can already configure conflicting receive/send
> ids for MKTs, I don't see a point not allowing any hash algorithm
> supported by crypto.

The algorithm enum controls not just the algorithm but the length in 
bytes of the traffic key and signatures. Supporting arbitrary algorithms 
requires supporting arbitrary byte lengths everywhere and increasing a 
couple of stack allocations (including some in the TCP core).

An earlier version actually had u8 fields for the length but those were 
later removed.

Adding this is not difficult but requires careful testing of the new 
corner cases. All this will be for functionality is never going to be used.

>> My knowledge of cryptography doesn't go much beyond "data goes in
>> signature goes out" but there are many recent arguments from that cipher
>> agility is outright harmful and recent protocols like WireGuard don't
>> support any algorithm choices.
> 
> You already limit usage when root-enabled sysctl is triggered, I don't
> see big concerns here.
> 
>>
>>> +#define TCP_SIG_POOL_MAX        8
>>> +static struct tcp_sig_pool_priv_t {
>>> +    struct tcp_sig_crypto        cryptos[TCP_SIG_POOL_MAX];
>>> +    unsigned int            cryptos_nr;
>>> +} tcp_sig_pool_priv = {
>>> +    .cryptos_nr = 1,
>>> +    .cryptos[TCP_MD5_SIG_ID].alg = "md5",
>>> +};
>>
>> Why an array of 8? Better to use an arbitrary list.
> 
> Some reasonable limit, may be 16 or whatever in order to avoid
> dynamically (re-)allocating the array and keeping O(1) lookups.

Defining an arbitrary array length limit is an underhanded way of making 
lookup O(1).