Re: [RFC PATCH 0/6] Keem Bay OCS ECC crypto driver

["Alessandrelli, Daniele" <daniele.alessandrelli@intel.com>]

On Thu, 2021-01-21 at 10:52 +0100, Ard Biesheuvel wrote:
> On Wed, 20 Jan 2021 at 20:00, Alessandrelli, Daniele
> <daniele.alessandrelli@intel.com> wrote:
> > Hi Ard,
> > 
> > Thank you very much for your valuable feedback.
> > 
> > On Mon, 2021-01-18 at 13:09 +0100, Ard Biesheuvel wrote:
> > > This is rather unusual compared with how the crypto API is
> > > typically
> > > used, but if this is really what you want to implement, you can
> > > do so
> > > by:
> > > - having a "ecdh" implementation that implements the entire
> > > range, and
> > > uses a fallback for curves that it does not implement
> > > - export the same implementation again as "ecdh" and with a known
> > > driver name "ecdh-keembay-ocs", but with a slightly lower
> > > priority,
> > > and in this case, return an error when the unimplemented curve is
> > > requested.
> > > 
> > > That way, you fully adhere to the API, by providing
> > > implementations of
> > > all curves by default. And if a user requests "ecdh-keembay-ocs"
> > > explicitly, it will not be able to use the P192 curve
> > > inadvertently.
> > 
> > I tried to implement this, but it looks like the driver name is
> > mandatory, so I specified one also for the first implementation.
> > 
> > Basically I defined two 'struct kpp_alg' variables; both with
> > cra_name
> > = "ecdh", but with different 'cra_driver_name' (one with
> > cra_driver_name = "ecdh-keembay-ocs-fallback" and the other one
> > with
> > cra_driver_name = "ecdh-keembay-ocs").
> > 
> > Is this what you were suggesting?
> > 
> > Anyway, that works (i.e., 'ecdh-keembay-ocs' returns an error when
> > the
> > unimplemented curve is requested; while 'ecdh-keembay-ocs' and
> > 'ecdh'
> > work fine with any curve), but I have to set the priority of 'ecdh-
> > keembay-ocs' to something lower than the 'ecdh_generic' priority.
> > Otherwise the implementation with fallback ends up using my "ecdh-
> > keembay-ocs" as fallback (so it ends up using a fallback that still
> > does not support the P-192 curve).
> > 
> > Also, the implementation without fallback is still failing crypto
> > self-
> > tests (as expected I guess).
> > 
> > Therefore, I tried with a slightly different solution. Still two
> > implementations, but with different cra_names (one with cra_name =
> > "ecdh" and the other one with cra_name = "ecdh-keembay"). This
> > solution
> > seems to be working, since, the "ecdh-keembay" is not tested by the
> > self tests and is not picked up as fallback for "ecdh" (since, if I
> > understand it correctly, it's like if I'm defining a new kind of
> > kpp
> > algorithm), but it's still picked when calling
> > crypto_alloc_kpp("ecdh-
> > keembay").
> > 
> > Does this second solution looks okay to you? Or does it have some
> > pitfall that I'm missing?
> > 
> 
> You should set the CRYPTO_ALG_NEED_FALLBACK flag on both
> implementations, to ensure that neither of them are considered as
> fallbacks themselves.

Thanks again!

I was setting that flag only for the first implementation (the one with
fallback), but I see now how it's needed for the second one as well.

With that, the second implementation (i.e., the one without fallback)
is not used anymore as a fallback for the first one.

As expected, the second implementation does not pass self-tests and
crypto_alloc_kpp() returns -ELIBBAD when trying to allocate it, but
I've seen that I can avoid the error (and have it allocated properly)
by passing the CRYPTO_ALG_TESTED flag in the 'type' argument, like
below:

   crypto_alloc_kpp("ecdh-keembay-ocs", CRYPTO_ALG_TESTED, 0);

Is that the right way to tell crypto_alloc_kpp() that we are fine using
an implementation that fails self-tests?