Re: [PATCH 5/5] tcp/md5: Make more generic tcp_sig_pool

[Dmitry Safonov <0x7f454c46@gmail.com>]

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.

> 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.

Thanks,
          Dmitry