Re: [PATCH] crypto: qat - Don't use -EFAULT as error code during setkey()

[Thomas Huth <thuth@redhat.com>]

Am 24.08.26 um 14:24 schrieb Thomas Huth:
> EFAULT means "Bad Address", it's the error code that is meant to
> be returned when userspace provided the kerknel with a pointer to
> memory that is not part of its address space. However, the QAT driver
> abuses this error code during its setkey() function to signal that
> there was something wrong with the selected algorithm, which is just
> confusing.
> 
> The best choice for an error code would likely have been EINVAL here,
> but qat_alg_aead_init_sessions() seems to make an effort to use this
> for another error case already, so there is likely the desire to
> distinguish the different errors here. Thus use EOPNOTSUPP as a
> replacement now, so qat_alg_aead_init_sessions() continues to return
> two different error codes depending on what went wrong.
> 
> Anyway, instead of hard-coding the error codes in the function
> qat_alg_aead_init_sessions() again, let's also simply rather pass
> the error code from the called functions up to the caller instead,
> this way we can also get rid of the need to call memzero_explicit()
> here three times.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   .../crypto/intel/qat/qat_common/qat_algs.c    | 31 +++++++++----------
>   1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/crypto/intel/qat/qat_common/qat_algs.c b/drivers/crypto/intel/qat/qat_common/qat_algs.c
> index 91663805d9e60..91da9cad1d89a 100644
> --- a/drivers/crypto/intel/qat/qat_common/qat_algs.c
> +++ b/drivers/crypto/intel/qat/qat_common/qat_algs.c
> @@ -135,7 +135,7 @@ static int qat_alg_do_precomputes(struct icp_qat_hw_auth_algo_blk *hash,
>   		return 0;
>   	}
>   	default:
> -		return -EFAULT;
> +		return -EOPNOTSUPP;
>   	}
>   }
>   
> @@ -186,7 +186,7 @@ static int qat_alg_aead_init_enc_session(struct crypto_aead *aead_tfm,
>   		cpu_to_be32(ctx->hash_blocksize);
>   
>   	if (qat_alg_do_precomputes(hash, ctx, keys->authkey, keys->authkeylen))
> -		return -EFAULT;
> +		return -EOPNOTSUPP;
>   
>   	/* Request setup */
>   	qat_alg_init_common_hdr(header);
> @@ -273,7 +273,7 @@ static int qat_alg_aead_init_dec_session(struct crypto_aead *aead_tfm,
>   		cpu_to_be32(ctx->hash_blocksize);
>   
>   	if (qat_alg_do_precomputes(hash, ctx, keys->authkey, keys->authkeylen))
> -		return -EFAULT;
> +		return -EOPNOTSUPP;
>   
>   	/* Request setup */
>   	qat_alg_init_common_hdr(header);
> @@ -472,28 +472,25 @@ static int qat_alg_aead_init_sessions(struct crypto_aead *tfm, const u8 *key,
>   				      unsigned int keylen,  int mode)
>   {
>   	struct crypto_authenc_keys keys;
> -	int alg;
> -
> -	if (crypto_authenc_extractkeys(&keys, key, keylen))
> -		goto bad_key;
> +	int alg, ret;
>   
> -	if (qat_alg_validate_key(keys.enckeylen, &alg, mode))
> -		goto bad_key;
> +	ret = crypto_authenc_extractkeys(&keys, key, keylen);
> +	if (ret)
> +		goto error;
>   
> -	if (qat_alg_aead_init_enc_session(tfm, alg, &keys, mode))
> +	ret = qat_alg_validate_key(keys.enckeylen, &alg, mode);
> +	if (ret)
>   		goto error;
>   
> -	if (qat_alg_aead_init_dec_session(tfm, alg, &keys, mode))
> +	ret = qat_alg_aead_init_enc_session(tfm, alg, &keys, mode);
> +	if (ret)
>   		goto error;
>   
> -	memzero_explicit(&keys, sizeof(keys));
> -	return 0;
> -bad_key:
> -	memzero_explicit(&keys, sizeof(keys));
> -	return -EINVAL;
> +	ret = qat_alg_aead_init_dec_session(tfm, alg, &keys, mode);
> +
>   error:
>   	memzero_explicit(&keys, sizeof(keys));
> -	return -EFAULT;
> +	return ret;
>   }
>   
>   static int qat_alg_skcipher_init_sessions(struct qat_alg_skcipher_ctx *ctx,

Friendly ping! Anybody got a comment on this patch?

  Thomas