Re: [PATCH v1 1/1] s390/crypto: Rework protected key AES for true asynch support

[Herbert Xu <herbert@gondor.apana.org.au>]

On Thu, Mar 06, 2025 at 06:12:01PM +0100, Harald Freudenberger wrote:
>
> +	/* fetch and check protected key state */
>  	spin_lock_bh(&ctx->pk_lock);
> +	pk_state = ctx->pk_state;
>  	memcpy(param.key, ctx->pk.protkey, PAES_256_PROTKEY_SIZE);
>  	spin_unlock_bh(&ctx->pk_lock);
> +	switch (pk_state) {
> +	case PK_STATE_NO_KEY:
> +		rc = -ENOKEY;
> +		goto out;
> +	case PK_STATE_CONVERT_IN_PROGRESS:
> +		rc = -EKEYEXPIRED;
> +		goto out;

Shouldn't this go async rather than failing?

> +	case PK_STATE_VALID:
> +		break;
> +	default:
> +		rc = pk_state < 0 ? pk_state : -EIO;
> +		goto out;
> +	}
> +
> +	/* setkey() should have updated the function code */
> +	if (!ctx->fc) {

The locking is wrong for this field.  It gets written to without
any locks in cbc_paes_wq_setkey_fn, and here you're reading it
without any locking.

In fact the whole switch statement smells fishy.  One tfm could
be used by any number of encryption requests in parallel.  So
your pk_state could change from right under your nose as soon as
you let go of the pk_lock.

Please describe the high-level picture of how pk_lock and its
protected fields are meant to work in the face of requests being
issued in parallel on one tfm.
  
> -static int cbc_paes_set_key(struct crypto_skcipher *tfm, const u8 *in_key,
> -			    unsigned int key_len)
> +static int cbc_paes_setkey(struct crypto_skcipher *tfm, const u8 *in_key,
> +			   unsigned int key_len)
>  {
>  	struct s390_paes_ctx *ctx = crypto_skcipher_ctx(tfm);
>  	int rc;
>  
> -	_free_kb_keybuf(&ctx->kb);
> -	rc = _key_to_kb(&ctx->kb, in_key, key_len);
> +	rc = key_to_ctx(ctx, in_key, key_len);
>  	if (rc)
>  		return rc;
>  
> -	return __cbc_paes_set_key(ctx);
> +	/* Always trigger an asynch key convert */
> +	spin_lock_bh(&ctx->pk_lock);
> +	ctx->pk_state = PK_STATE_CONVERT_IN_PROGRESS;
> +	spin_unlock_bh(&ctx->pk_lock);
> +	queue_work(paes_wq, &ctx->work);

Why does this need to be async? The setkey function is the one
part of the API that is competely synchronous.  It is not allowed
to occur while any encryption is still incomplete.

By making it asynchronous, you risk creating new issues.  For example,
what is supposed to happen when a second setkey occurs while the first
setkey's scheduled work is yet to complete?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt