Re: [PATCH v2 03/18] crypto: dh - optimize domain parameter serialization for well-known groups

From: Stephan Mueller <smueller@chronox.de>
To: Nicolai Stange <nstange@suse.de>,
	Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>,
	Hannes Reinecke <hare@suse.de>, Torsten Duwe <duwe@suse.de>,
	Zaibo Xu <xuzaibo@huawei.com>,
	Giovanni Cabiddu <giovanni.cabiddu@intel.com>,
	David Howells <dhowells@redhat.com>,
	Jarkko Sakkinen <jarkko@kernel.org>,
	linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
	qat-linux@intel.com, keyrings@vger.kernel.org, simo@redhat.com
Subject: Re: [PATCH v2 03/18] crypto: dh - optimize domain parameter serialization for well-known groups
Date: Thu, 06 Jan 2022 15:30:04 +0100	[thread overview]
Message-ID: <2468270.qO8rWLYou6@tauon.chronox.de> (raw)
In-Reply-To: <YcvEkfS4cONDXXB9@gondor.apana.org.au>

Am Mittwoch, 29. Dezember 2021, 03:14:41 CET schrieb Herbert Xu:

Hi Herbert,

> On Mon, Dec 20, 2021 at 04:27:35PM +0100, Nicolai Stange wrote:
> > Just for my understanding: the problem here is having a (single) enum
> > for the representation of all the possible "known" groups in the first
> > place or more that the individual group id enum members have hard-coded
> > values assigned to them each?
> 
> Yes the fact that you need to have a list of all "known" groups is
> the issue.
> 
> > However, after some back and forth, I opted against doing something
> > similar for dh at the time, because there are quite some more possible
> > parameter sets than there are for ecdh, namely ten vs. three. If we were
> 
> I don't understand why we can't support ten or an even larger
> number of parameter sets.
> 
> If you are concerned about code duplication then there are ways
> around that.  Or do you have another specific concern in mind
> with respect to a large number of parameter sets under this scheme?
> 
> > Anyway, just to make sure I'm getting it right: when you're saying
> > "template", you mean to implement a crypto_template for instantiating
> > patterns like "dh(ffdhe2048)", "dh(ffdhe3072)" and so on? The dh(...)
> > template instantiations would keep a crypto_spawn for referring to the
> > underlying, non-template "dh" kpp_alg so that "dh" implementations of
> > higher priority (hpre + qat) would take over once they'd become
> > available, correct?
> 
> The template would work in the other dirirection.  It would look
> like ffdhe2048(dh) with dh being implemented in either software or
> hardware.
> 
> The template wrapper would simply supply the relevant parameters.

I fully agree with you. However, there is a small wrinkle we should consider. 
In FIPS mode, we must only allow DH together with the safe primes provided by 
the templates of ffdheXX and modpXX.

This means in FIPS mode, invoking the algo of "dh" should not be possible. 
Yet, on the other hand, we cannot mark "dh" as fips_allowed == 0 as the 
templates would not be able to instantiate them.

Therefore, I think we should mark "dh" as CRYPTO_ALG_INTERNAL if in FIPS mode. 
To do so, I would think that dh_init should contain something like:

if (fips_enabled)
	dh.base.cra_flags |= CRYPTO_ALG_INTERNAL;

When encapsulating this small flag setting code into a helper function (just 
like xts_check_key or xts_verify_key) this helper can be added to other / new 
DH implementations QAT to make them FIPS-compliant. This would be the same 
approach as we currently take for XTS where each XTS implementation must have 
a callback to xts_check_key.

Marking "dh" as INTERNAL implies it cannot be allocated in FIPS mode. Some may 
think this is a small inconsistency as this algo is marked fips_allowed == 1. 
I personally think there is no inconsistency because DH *is* allowed, however 
with a small policy caveat (the requirement to use it with safe-primes). So, 
having "dh" with fips_allowed == 1 but marking it as INTERNAL should be also 
consistent.

Yes, it is a small misuse of the INTERNAL flag. But the alternative would be 
to create a "__dh" implementation that is wrapped by "dh". In turn this 
implies a big churn as we would need to touch all drivers implementing "dh".

> 
> Cheers,

Ciao
Stephan