Re: [PATCH v3 4/4] fscrypt: Add LEA-256-XTS, LEA-256-CTS support

[Eric Biggers]

On Thu, Jun 29, 2023 at 07:01:11PM +0900, Dongsoo Lee wrote:
> On Tue, Jun 27, 2023 at 23:38:30 -0700, Eric Biggers wrote:
> >On Mon, Jun 26, 2023 at 05:47:03PM +0900, Dongsoo Lee wrote:
> >> when SIMD instructions are available, it performs even faster.
> >
> >This will only be true once there is actually an applicable implementation
> of
> >LEA-XTS and LEA-CTS using SIMD instructions included in the kernel.
> >
> >Perhaps it is your plan to go through and accelerate LEA-XTS and LEA-CTS
> for the
> >common CPU architectures.  However, it is not included in this patchset
> yet, so
> >it should not be claimed in the documentation yet.
> >
> >> Particularly, it outperforms AES when the dedicated crypto
> >> +instructions for AES are unavailable, regardless of the presence of SIMD
> >> +instructions. However, it is not recommended to use LEA unless there is
> >> +a clear reason (such as the absence of dedicated crypto instructions for
> >> +AES or a mandatory requirement) to do so. Also, to enable LEA support,
> >> +it needs to be enabled in the kernel crypto API.
> >
> >I think I'd prefer that you omit the mention of the "absence of dedicated
> crypto
> >instructions" use case for now.  fscrypt already supports another algorithm
> that
> >fulfills exactly that use case (Adiantum), and that algorithm already has
> >optimized implementations for arm32, arm64, and x86_64.  LEA does not have
> that
> >yet.  So it does not really bring anything new to the table.  I'm also
> unsure it
> >would be appropriate to recommend a "lightweight" cipher at this point...
> >
> >That would leave "mandatory requirement" as the rationale, at least for
> now,
> >similar to SM4.
> >
> >- Eric
> 
> As you might expect, we are working on a SIMD implementation of LEA in a
> general-purpose CPU environment. However, since no such implementation has
> been submitted yet, we agree that it's right to leave it out for now.
> 
> In the next version, we would like to change the description to the
> following:
> 
> LEA is a South Korean 128-bit block cipher (with 128/192/256-bit keys)
> included in the ISO/IEC 29192-2:2019 standard (Information security -
> Lightweight cryptography - Part 2: Block ciphers). If dedicated cipher
> instructions are available, or other options with performance benefits
> are available, using LEA is likely not a suitable choice. Therefore,
> it is not recommended to use LEA-256-XTS unless there is a clear reason
> to do so, such as if there is a mandate. Also, to enable LEA support,
> it needs to be enabled in the kernel crypto API.

I don't think that really addresses my comment, due to the second sentence.  I
understand that you would like to advertise the performance of LEA.  But as I
mentioned, it's not yet realized in the kernel crypto API, and in the context of
fscrypt it won't really bring anything new to the table anyway.  For now I think
LEA is best described as a "national pride cipher" alongside SM4...  Keep in
mind, it can always be changed later if new use cases come up.

Could you just omit the documentation update from your patch?  I actually need
to rework the whole "Encryption modes and usage" section anyway since it's
growing a bit unwieldy, with 6 different combinations of encryption modes now
supported.  The information needs to be organized better.  It currently reads
like a list, and it might be hard for users to understand which setting to use.

I'll add on a patch that does that and adds the mention of LEA support.

- Eric