linux-kernel - Re: [PATCH v2 13/13] RISC-V: crypto: add Zvkb accelerated ChaCha20 implementation

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]

Date:   Tue, 28 Nov 2023 16:57:38 +0800
From:   Jerry Shih <jerry.shih@...ive.com>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     Paul Walmsley <paul.walmsley@...ive.com>, palmer@...belt.com,
        Albert Ou <aou@...s.berkeley.edu>, herbert@...dor.apana.org.au,
        davem@...emloft.net, conor.dooley@...rochip.com, ardb@...nel.org,
        heiko@...ech.de, phoebe.chen@...ive.com, hongrong.hsu@...ive.com,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-crypto@...r.kernel.org
Subject: Re: [PATCH v2 13/13] RISC-V: crypto: add Zvkb accelerated ChaCha20
 implementation

On Nov 28, 2023, at 12:25, Eric Biggers <ebiggers@...nel.org> wrote:
> On Mon, Nov 27, 2023 at 03:07:03PM +0800, Jerry Shih wrote:
>> +config CRYPTO_CHACHA20_RISCV64
> 
> Can you call this kconfig option just CRYPTO_CHACHA_RISCV64?  I.e. drop the
> "20".  The ChaCha family of ciphers includes more than just ChaCha20.
> 
> The other architectures do use "CHACHA20" in their equivalent option, even when
> they implement XChaCha12 too.  But that's for historical reasons -- we didn't
> want to break anything by renaming the kconfig options.  For a new option we
> should use the more general name from the beginning, even if initially only
> ChaCha20 is implemented (which is fine).

I will use `CRYPTO_CHACHA_RISCV64` instead.

>> +static int chacha20_encrypt(struct skcipher_request *req)
> 
> riscv64_chacha_crypt(), please.  chacha20_encrypt() is dangerously close to
> being the same name as chacha20_crypt() which already exists in crypto/chacha.h.

The function will will have additional prefix/suffix.

>> +static inline bool check_chacha20_ext(void)
>> +{
>> +	return riscv_isa_extension_available(NULL, ZVKB) &&
>> +	       riscv_vector_vlen() >= 128;
>> +}
> 
> Just to double check: your intent is to simply require VLEN >= 128 for all the
> RISC-V vector crypto code, even when some might work with a shorter VLEN?  I
> don't see anything in chacha-riscv64-zvkb.pl that assumes VLEN >= 128, for
> example.  I think it would even work with VLEN == 32.

Yes, the chacha algorithm here only needs the VLEN>=32. But I think we will not get
benefits with that kind of hw.

> I think requiring VLEN >= 128 anyway makes sense so that we don't have to worry
> about validating the code with shorter VLEN.  And "application processors" are
> supposed to have VLEN >= 128.  But I just wanted to make sure this is what you
> intended too.

The standard "V" extension assumes VLEN>=128. I just follow that assumption.
https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#183-v-vector-extension-for-application-processors

-Jerry