| Message ID | 20221229211710.14912-4-vdronov@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Trivial set of FIPS 140-3 related changes | expand |
Hi Vladis,
the patch subject prefix is a bit misleading IMO, it kind of suggests
that this patch would apply to the generic crypto/xts.c. How about using
a format similar to e.g. the one from commit 7988fb2c03c8 ("crypto:
s390/aes - convert to skcipher API"), i.e.
"crypto: s390/aes - drop redundant xts key check"
?
Vladis Dronov <vdronov@redhat.com> writes:
> xts_fallback_setkey() in xts_aes_set_key() will now enforce key size
> rule in FIPS mode when setting up the fallback algorithm keys,
I think it would be nice to make it more explicit why/how
xts_fallback_setkey() happens to enforce the key size rules now.
Perhaps amend the above sentence by something like
"xts_fallback_setkey() in xts_aes_set_key() will now implictly enforce
the key size rule in FIPS mode by means of invoking the generic xts
implementation with its key checks for setting up the fallback
algorithm,"
?
> which makes the check in xts_aes_set_key() redundant or
> unreachable. So just drop this check.
>
> xts_fallback_setkey() now makes a key size check in xts_verify_key():
>
> xts_fallback_setkey()
> crypto_skcipher_setkey() [ skcipher_setkey_unaligned() ]
> cipher->setkey() { .setkey = xts_setkey }
> xts_setkey()
> xts_verify_key()
>
> Signed-off-by: Vladis Dronov <vdronov@redhat.com>
> ---
> arch/s390/crypto/aes_s390.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
> index 526c3f40f6a2..c773820e4af9 100644
> --- a/arch/s390/crypto/aes_s390.c
> +++ b/arch/s390/crypto/aes_s390.c
> @@ -398,10 +398,6 @@ static int xts_aes_set_key(struct crypto_skcipher *tfm, const u8 *in_key,
> if (err)
> return err;
>
> - /* In fips mode only 128 bit or 256 bit keys are valid */
> - if (fips_enabled && key_len != 32 && key_len != 64)
> - return -EINVAL;
> -
The change itself looks good, but it might be worth adding a comment
right at the invocation of xts_fallback_setkey() that this includes an
implicit xts_verify_key() check? So that if anybody ever was about to
remove the xts_fallback_setkey() for some reason in the future, it would
give a clear indication that xts_verify_key() needs to get called
directly instead?
Thanks!
Nicolai
> /* Pick the correct function code based on the key length */
> fc = (key_len == 32) ? CPACF_KM_XTS_128 :
> (key_len == 64) ? CPACF_KM_XTS_256 : 0;
diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c index 526c3f40f6a2..c773820e4af9 100644 --- a/arch/s390/crypto/aes_s390.c +++ b/arch/s390/crypto/aes_s390.c @@ -398,10 +398,6 @@ static int xts_aes_set_key(struct crypto_skcipher *tfm, const u8 *in_key, if (err) return err; - /* In fips mode only 128 bit or 256 bit keys are valid */ - if (fips_enabled && key_len != 32 && key_len != 64) - return -EINVAL; - /* Pick the correct function code based on the key length */ fc = (key_len == 32) ? CPACF_KM_XTS_128 : (key_len == 64) ? CPACF_KM_XTS_256 : 0;
xts_fallback_setkey() in xts_aes_set_key() will now enforce key size rule in FIPS mode when setting up the fallback algorithm keys, which makes the check in xts_aes_set_key() redundant or unreachable. So just drop this check. xts_fallback_setkey() now makes a key size check in xts_verify_key(): xts_fallback_setkey() crypto_skcipher_setkey() [ skcipher_setkey_unaligned() ] cipher->setkey() { .setkey = xts_setkey } xts_setkey() xts_verify_key() Signed-off-by: Vladis Dronov <vdronov@redhat.com> --- arch/s390/crypto/aes_s390.c | 4 ---- 1 file changed, 4 deletions(-)