On Wed, 20 Jan 2021 at 20:00, Alessandrelli, Daniele
[off-list ref] wrote:

Hi Ard,

Thank you very much for your valuable feedback.

On Mon, 2021-01-18 at 13:09 +0100, Ard Biesheuvel wrote:

quoted

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.