Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> writes: > On Fri, Jan 14, 2022 at 10:09:02AM +0100, Nicolai Stange wrote: >> >> I wonder whether this can be made more generic. I.e. would it be possible >> to make crypto_grab_spawn() to or in FIPS_INTERNAL into type (iff !(mask >> & FIPS_INTERNAL)) and to make crypto_register_instance() to propagate >> FIPS_INTERNAL from the template instance's spawns upwards into the >> instance's ->cra_flags? > > We could certainly do something like that in future. But it > isn't that easy because crypto_register_instance doesn't know > what the paramter algorithm(s) is/are. I was thinking of simply walking through the inst->spawns list for that. > >> On an unrelated note, this will break trusted_key_tpm_ops->init() in >> FIPS mode, because trusted_shash_alloc() would fail to get a hold of >> sha1. AFAICT, this could potentially make the init_trusted() module_init >> to fail, and, as encrypted-keys.ko imports key_type_trusted, prevent the >> loading of that one as well. Not sure that's desired... > > Well if sha1 is supposed to be forbidden in FIPS mode why should > TPM be allowed to use it? Yes, I only wanted to point out that doing that could potentially have unforseen consequences as it currently stands, like e.g. encrypted-keys.ko loading failures, even though the latter doesn't even seem to use sha1 by itself. However, this scenario would be possible only for CONFIG_TRUSTED_KEYS=m, CONFIG_TEE=n and tpm_default_chip() returning something. > If it must be allowed, then we could change TPM to set the > FIPS_INTERNAL flag. > > I think I'll simply leave out the line that actually disables sha1 > for now. > >> > diff --git a/crypto/api.c b/crypto/api.c >> > index cf0869dd130b..549f9aced1da 100644 >> > --- a/crypto/api.c >> > +++ b/crypto/api.c >> > @@ -223,6 +223,8 @@ static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg) >> > else if (crypto_is_test_larval(larval) && >> > !(alg->cra_flags & CRYPTO_ALG_TESTED)) >> > alg = ERR_PTR(-EAGAIN); >> > + else if (alg->cra_flags & CRYPTO_ALG_FIPS_INTERNAL) >> > + alg = ERR_PTR(-EAGAIN); >> >> I might be mistaken, but I think this would cause hmac(sha1) >> instantiation to fail if sha1 is not already there. I.e. if hmac(sha1) >> instantiation would load sha1, then it would invoke crypto_larval_wait() >> on the sha1 larval, see the FIPS_INTERNAL and fail? > > When EAGAIN is returned the lookup is automatically retried. Ah right, just found the loop in cryptomgr_probe(). > >> However, wouldn't it be possible to simply implement FIPS_INTERNAL >> lookups in analogy to the INTERNAL ones instead? That is, would adding >> >> if (!((type | mask) & CRYPTO_ALG_FIPS_INTERNAL)) >> mask |= CRYPTO_ALG_FIPS_INTERNAL; >> >> to the preamble of crypto_alg_mod_lookup() work instead? > > If you do that then you will end up creating an infinite number > of failed templates if you lookup something like hmac(md5). Once > the first hmac(md5) is created it must then match all subsequent > lookups of hmac(md5) in order to prevent any further creation. Thanks for the explanation, it makes sense now. > >> This looks all good to me, but as !->fips_allowed tests aren't skipped >> over anymore now, it would perhaps make sense to make their failure >> non-fatal in FIPS mode. Because in FIPS mode a failure could mean a >> panic and some of the existing TVs might not pass because of e.g. some >> key length checks or so active only for fips_enabled... > > You mean a buggy non-FIPS algorithm that fails when tested in > FIPS mode? Yes, I can't tell how realistic that is though. > I guess we could skip the panic in that case if everyone is happy with > that. Stephan? > Thanks, Nicolai -- SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg), GF: Ivo Totev
