On Thu, 2021-01-21 at 10:52 +0100, Ard Biesheuvel wrote: > On Wed, 20 Jan 2021 at 20:00, Alessandrelli, Daniele > <daniele.alessandre...@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?