From:   Stephan Mueller <smueller@chronox.de>
To:     Nicolai Stange <nstange@suse.de>
Cc:     Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        Nicolai Stange <nstange@suse.de>,
        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
Subject: Re: [PATCH 09/18] crypto: dh - implement private key generation primitive
Date:   Wed, 08 Dec 2021 08:16:18 +0100
Message-ID: <4767831.y2tiDqZFiq@tauon.chronox.de>
In-Reply-To: <87pmq7ehxg.fsf@suse.de>
References: <20211201004858.19831-1-nstange@suse.de> <25213093.1r3eYUQgxm@positron.chronox.de> <87pmq7ehxg.fsf@suse.de>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

Am Mittwoch, 8. Dezember 2021, 07:20:43 CET schrieb Nicolai Stange:

Hi Nicolai,

> >> +		return -EINVAL;
> >> +
> >> +	/*
> >> +	 * 5.6.1.1.1: choose key length N such that
> >> +	 * 2 * ->max_strength <= N <= log2(q) + 1 = ->p_size * 8 - 1
> >> +	 * with q = (p - 1) / 2 for the safe-prime groups.
> >> +	 * Choose the lower bound's next power of two for N in order to
> >> +	 * avoid excessively large private keys while still
> >> +	 * maintaining some extra reserve beyond the bare minimum in
> >> +	 * most cases. Note that for each entry in safe_prime_groups[],
> >> +	 * the following holds for such N:
> >> +	 * - N >= 256, in particular it is a multiple of 2^6 = 64
> >> +	 *   bits and
> >> +	 * - N < log2(q) + 1, i.e. N respects the upper bound.
> >> +	 */
> >> +	n = roundup_pow_of_two(2 * g->max_strength);
> >> +	WARN_ON_ONCE(n & ((1u << 6) - 1));
> >> +	n >>= 6; /* Convert N into units of u64. */
> > 
> > Couldn't we pre-compute that value for each of the safeprime groups? This
> > value should be static for each of them.
> 
> Can you elaborate why this would be better? As long as the value
> calculated above is considered reasonable for every usecase, I don't see
> the advantage of storing it somewhere.

Well, I usually try to avoid using CPU resources if I have information a-
priori. And as we have only known domain parameters in this code path, I 
thought we can spare a few CPU cycles.

> 
> OTOH, calculating the value on the fly
> - enforces conformance to 5.6.1.1.1 (>= twice the sec strength)
> - and guarantees that it is a multiple of 64 bits, as required
>   by the implementation,
> whereas you'd had to examine each and every individual group's setting
> for correctness when storing precomputed values alongside the other,
> "primary" group parameters.

You are right, but when we reach this code path we only have well-known 
parameters. Hence my suggestion.

Ciao
Stephan