netdev - Re: [PATCH v2 06/25] tcp: authopt: Compute packet signatures

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: <6beb9ff4-34ec-5685-ab9d-decd382ab7cc@gmail.com>
Date:   Fri, 5 Nov 2021 08:39:12 +0200
From:   Leonard Crestez <cdleonard@...il.com>
To:     Dmitry Safonov <0x7f454c46@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Herbert Xu <herbert@...dor.apana.org.au>
Cc:     Kuniyuki Iwashima <kuniyu@...zon.co.jp>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Yuchung Cheng <ycheng@...gle.com>,
        Francesco Ruggeri <fruggeri@...sta.com>,
        Mat Martineau <mathew.j.martineau@...ux.intel.com>,
        Christoph Paasch <cpaasch@...le.com>,
        Ivan Delalande <colona@...sta.com>,
        Priyaranjan Jha <priyarjha@...gle.com>, netdev@...r.kernel.org,
        linux-crypto@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org, Shuah Khan <shuah@...nel.org>,
        David Ahern <dsahern@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH v2 06/25] tcp: authopt: Compute packet signatures

On 11/5/21 3:53 AM, Dmitry Safonov wrote:
> On 11/1/21 16:34, Leonard Crestez wrote:
> [..]
>> +static int skb_shash_frags(struct shash_desc *desc,
>> +			   struct sk_buff *skb)
>> +{
>> +	struct sk_buff *frag_iter;
>> +	int err, i;
>> +
>> +	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
>> +		skb_frag_t *f = &skb_shinfo(skb)->frags[i];
>> +		u32 p_off, p_len, copied;
>> +		struct page *p;
>> +		u8 *vaddr;
>> +
>> +		skb_frag_foreach_page(f, skb_frag_off(f), skb_frag_size(f),
>> +				      p, p_off, p_len, copied) {
>> +			vaddr = kmap_atomic(p);
>> +			err = crypto_shash_update(desc, vaddr + p_off, p_len);
>> +			kunmap_atomic(vaddr);
>> +			if (err)
>> +				return err;
>> +		}
>> +	}
>> +
>> +	skb_walk_frags(skb, frag_iter) {
>> +		err = skb_shash_frags(desc, frag_iter);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> This seems quite sub-optimal: IIUC, shash should only be used for small
> amount of hashing. That's why tcp-md5 uses ahash with scatterlists.

There is indeed no good reason to prefer shash over ahash. Despite the 
"async" in the name it's possible to use it in atomic context.

> Which drives me to the question: why not reuse tcp_md5sig_pool code?
> 
> And it seems that you can avoid TCP_AUTHOPT_ALG_* enum and just supply
> to crypto the string from socket option (like xfrm does).
> 
> Here is my idea:
> https://lore.kernel.org/all/20211105014953.972946-6-dima@arista.com/T/#u

Making the md5 pool more generic and reusing it can work.

This "pool" mechanism is really just a workaround for the crypto API not 
supporting the allocation of a hash in softirq context. It would make a 
lot sense for this functionality to be part of the crypto layer itself.

Looking at your generic tcp_sig_crypto there is nothing actually 
specific to TCP in there: it's just an ahash and a scratch buffer per-cpu.

I don't understand the interest in using arbitrary crypto algorithms 
beyond RFC5926, this series is already complex enough. Other than 
increasing the complexity of crypto allocation there are various stack 
allocations which would need to be up to the maximum size of a TCP options.

--
Regards,
Leonard