Re: [PATCH v3 2/3] s390/crypto: Rework protected key AES for true asynch support

[Harald Freudenberger <freude@linux.ibm.com>]

On 2025-04-25 16:56, Holger Dengler wrote:
> On 01/04/2025 16:50, Harald Freudenberger wrote:
>> This is a complete rework of the protected key AES (PAES) 
>> implementation.
>> The goal of this rework is to implement the 4 modes (ecb, cbc, ctr, 
>> xts)
>> in a real asynchronous fashion:
>> - init(), exit() and setkey() are synchronous and don't allocate any 
>> memory.
>> - the encrypt/decrypt functions first try to do the job in a 
>> synchronous
>>   manner. If this fails, for example the protected key got invalid 
>> caused
>>   by for example a guest suspend/resume or guest migration action, the
> 
> reword: please drop one of the "for example".
> 

done

>>   encrypt/decrypt is transfered to an instance of the crypto engine 
>> (see
> 
> typo: transferred
> 

done

>>   below) for asynchronous processing.
>>   These via crypto engine postponed requests are then handled via the
>>   do_one_request() callback but may of course again run into a still
> 
> reword: please drop at least one "via". Proposal (if I got it
> correctly): "These postponed requests are then handled by the crypto
> engine by calling the do_one_request() callback ..."
> 

done

>>   not converted key or the key is getting invalid. If the key is
>>   still not converted, the first thread does the conversion and 
>> updates
>>   the key status in the transformation context. The conversion is
>>   invoked via pkey API with a new flag PKEY_XFLAG_NOMEMALLOC.
>> 
>> The pkey API used here - the function pkey_key2protkey() - uses
>> a new version of this in-kernel-API. A new flag PKEY_XFLAG_NOMEMALLOC
>> tells the PKEY layer (and subsidiary layers) that it must not allocate
>> any memory causing IO operations. Note that the patches for this
>> pkey/zcrypt/AP extensions are currently under review and yet not
>> upstream available. SO THIS PATCH DOES NOT COMPILE YET.
> 
> As the ap-rework series is now on its way, you can remove parts of
> this paragraph.
> 

done

>> This patch together with the pkey/zcrypt/AP extensions should
>> toughen the paes crypto algorithms to truly meet the requirements
>> for in-kernel skcipher implementations and the usage patterns for
>> the dm-crypt and dm-integrity layers.
>> 
>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> 
> It is very hard to review this patch. If there is any chance to split
> this up into smaller pieces, please do it.
> This is the first part of the review, covering mainly common parts and
> ecb. The other modes will follow later.
> See my comments below.
> 

Sorry but I don't see how I could split this rework into digestible 
units.

>> ---
>>  arch/s390/crypto/paes_s390.c | 1725 
>> +++++++++++++++++++++++-----------
>>  1 file changed, 1183 insertions(+), 542 deletions(-)
>> 
>> diff --git a/arch/s390/crypto/paes_s390.c 
>> b/arch/s390/crypto/paes_s390.c
>> index 646cbbf0678d..1d1f1a98ec4d 100644
>> --- a/arch/s390/crypto/paes_s390.c
>> +++ b/arch/s390/crypto/paes_s390.c
...
>> +#define PK_STATE_NO_KEY		     0
>> +#define PK_STATE_CONVERT_IN_PROGRESS 1
>> +#define PK_STATE_VALID		     2
> 
> Please use an enum here.
> 

I'd like to keep the one variable pk_state with either a negative errno
value if the key conversion failed, or a >= value with one of the listed
states.

>> +
>> +struct s390_paes_ctx {
>> +	/* source key material used to derive a protected key from */
>> +	u8 keybuf[PAES_MAX_KEYSIZE];
>> +	unsigned int keylen;
>> +
>> +	/* cpacf function code to use with this protected key type */
>> +	long fc;
>> +
>> +	/* spinlock to atomic read/update all the following fields */
>> +	spinlock_t pk_lock;
>> +
>> +	/* see PK_STATE* defines above, < 0 holds convert failure rc  */
>> +	int pk_state;
> 
> I see no advantage to split the value range. On the contrary, it makes
> the status handling more complex.
> I would prefer to use an enum for pk_state and use another element for
> the conversion rc.
> 
>> +	/* if state is valid, pk holds the protected key */
>> +	struct paes_protkey pk;
>> +};
>> +
>> +struct s390_pxts_ctx {
>> +	/* source key material used to derive a protected key from */
>> +	u8 keybuf[2 * PAES_MAX_KEYSIZE];
>>  	unsigned int keylen;
>> +
>> +	/* cpacf function code to use with this protected key type */
>> +	long fc;
>> +
>> +	/* spinlock to atomic read/update all the following fields */
>> +	spinlock_t pk_lock;
>> +
>> +	/* see PK_STATE* defines above, < 0 holds convert failure rc  */
>> +	int pk_state;
> 
> Same here.
> 
>> +	/* if state is valid, pk[] hold(s) the protected key(s) */
>> +	struct paes_protkey pk[2];
>>  };
>> 
>>  /*
>> @@ -89,214 +122,344 @@ static inline u32 make_clrkey_token(const u8 
>> *ck, size_t cklen, u8 *dest)
>>  	return sizeof(*token) + cklen;
>>  }
>> 
>> -static inline int _key_to_kb(struct key_blob *kb,
>> -			     const u8 *key,
>> -			     unsigned int keylen)
>> +/*
>> + * key_to_ctx() - Set key value into context, maybe construct
>> + * a clear key token digestable by pkey from a clear key value.
>> + */
>> +static inline int key_to_ctx(struct s390_paes_ctx *ctx,
>> +			     const u8 *key, unsigned int keylen)
> 
> The function name implies a transformation of a key into a context,
> not just a set of a context element. What about paes_ctx_setkey()?
> 

done, this is now paes_ctx_setkey()

>>  {
>> +	if (keylen > sizeof(ctx->keybuf))
>> +		return -EINVAL;
>> +
>>  	switch (keylen) {
>>  	case 16:
>>  	case 24:
>>  	case 32:
>>  		/* clear key value, prepare pkey clear key token in keybuf */
>> -		memset(kb->keybuf, 0, sizeof(kb->keybuf));
>> -		kb->keylen = make_clrkey_token(key, keylen, kb->keybuf);
>> -		kb->key = kb->keybuf;
>> +		memset(ctx->keybuf, 0, sizeof(ctx->keybuf));
>> +		ctx->keylen = make_clrkey_token(key, keylen, ctx->keybuf);
>>  		break;
>>  	default:
>>  		/* other key material, let pkey handle this */
>> -		if (keylen <= sizeof(kb->keybuf))
>> -			kb->key = kb->keybuf;
>> -		else {
>> -			kb->key = kmalloc(keylen, GFP_KERNEL);
>> -			if (!kb->key)
>> -				return -ENOMEM;
>> -		}
>> -		memcpy(kb->key, key, keylen);
>> -		kb->keylen = keylen;
>> +		memcpy(ctx->keybuf, key, keylen);
>> +		ctx->keylen = keylen;
>>  		break;
>>  	}
>> 
>>  	return 0;
>>  }
>> 
>> -static inline int _xts_key_to_kb(struct key_blob *kb,
>> -				 const u8 *key,
>> -				 unsigned int keylen)
>> +/*
>> + * xts_key_to_ctx() - Set key value into context, maybe construct
>> + * a clear key token digestable by pkey from a clear key value.
>> + */
>> +static inline int xts_key_to_ctx(struct s390_pxts_ctx *ctx,
>> +				 const u8 *key, unsigned int keylen)
> 
> Same here, the function name implies a transformation of a key into a
> context, not just a set of a context element. What about
> pxts_ctx_setkey()?

done

> 
>>  {
>>  	size_t cklen = keylen / 2;
>> 
> [...]
>> +static int ecb_paes_do_crypt(struct s390_paes_ctx *ctx,
>> +			     struct s390_pecb_req_ctx *req_ctx,
>> +			     bool maysleep)
>>  {
>> -	struct s390_paes_ctx *ctx = crypto_skcipher_ctx(tfm);
>> -	int rc;
>> +	struct ecb_param *param = &req_ctx->param;
>> +	struct skcipher_walk *walk = &req_ctx->walk;
>> +	unsigned int nbytes, n, k;
>> +	int pk_state, rc;
>> +
>> +	if (!req_ctx->param_init_done) {
>> +		/* 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);
> 
> I would prefer to use the size of param->key instead of a constant
> value as length.
> 

I checked all these occurrences and rewrote where possible to use 
sizeof().

>> +		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;
>> +		case PK_STATE_VALID:
>> +			req_ctx->param_init_done = true;
>> +			break;
>> +		default:
>> +			rc = pk_state < 0 ? pk_state : -EIO;
>> +			goto out;
>> +		}
>> +	}
>> 
>> -	_free_kb_keybuf(&ctx->kb);
>> -	rc = _key_to_kb(&ctx->kb, in_key, key_len);
>> -	if (rc)
>> -		return rc;
>> +	rc = 0;
> 
> Modify the param block in req_ctx only if the protected key is valid.
> 
> int rc = 0;
> 
> if (!req_ctx->param_init_done) {
> 	/* fetch and check protected key state */
> 	spin_lock_bh(&ctx->pk_lock);
> 	switch (ctx->pk_state) {
> 	case PK_STATE_NO_KEY:
> 		rc = -ENOKEY;
> 		break;
> 	case PK_STATE_CONVERT_IN_PROGRESS:
> 		rc = -EKEYEXPIRED;
> 		break;
> 	case PK_STATE_VALID:
> 		memcpy(param->key, ctx->pk.protkey, sizeof(param->key));
> 		req_ctx->param_init_done = true;
> 		break;
> 	default:
> 		rc = pk_state < 0 ? pk_state : -EIO;
> 		break;
> 	}
> 	spin_unlock_bh(&ctx->pk_lock);
> 	if (rc)
> 		goto out;
> }
> 

done (for all 4 algs)

>> +
>> +	/* always walk on the ... */
> 
> What does this comment mean? I'm afraid, I don't get it.
> 

removed, but added a comment about the walk instead as suggested by 
Herbert Xu

>> +	while ((nbytes = walk->nbytes) != 0) {
>> +		/* only use complete blocks */
>> +		n = nbytes & ~(AES_BLOCK_SIZE - 1);
>> +		k = cpacf_km(ctx->fc | req_ctx->modifier, param,
>> +			     walk->dst.virt.addr, walk->src.virt.addr, n);
>> +		if (k)
>> +			rc = skcipher_walk_done(walk, nbytes - k);
>> +		if (k < n) {
>> +			if (!maysleep) {
>> +				rc = -EKEYEXPIRED;
>> +				goto out;
>> +			}
>> +			rc = paes_convert_key(ctx);
>> +			if (rc)
>> +				goto out;
>> +			spin_lock_bh(&ctx->pk_lock);
>> +			memcpy(param->key, ctx->pk.protkey, PAES_256_PROTKEY_SIZE);
>> +			spin_unlock_bh(&ctx->pk_lock);
>> +		}
>> +	}
>> 
>> -	return __ecb_paes_set_key(ctx);
>> +out:
>> +	pr_debug("rc=%d\n", rc);
>> +	return rc;
>>  }
>> 
>>  static int ecb_paes_crypt(struct skcipher_request *req, unsigned long 
>> modifier)
>>  {
>> +	struct s390_pecb_req_ctx *req_ctx = skcipher_request_ctx(req);
>>  	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
>>  	struct s390_paes_ctx *ctx = crypto_skcipher_ctx(tfm);
>> -	struct {
>> -		u8 key[PAES_256_PROTKEY_SIZE];
>> -	} param;
>> -	struct skcipher_walk walk;
>> -	unsigned int nbytes, n, k;
>> +	struct skcipher_walk *walk = &req_ctx->walk;
>>  	int rc;
>> 
>> -	rc = skcipher_walk_virt(&walk, req, false);
>> +	/*
>> +	 * First try synchronous. If this fails for any reason
>> +	 * schedule this request asynchronous via crypto engine.
>> +	 */
>> +
>> +	rc = skcipher_walk_virt(walk, req, false);
>>  	if (rc)
>> -		return rc;
>> +		goto out;
>> 
>> -	spin_lock_bh(&ctx->pk_lock);
>> -	memcpy(param.key, ctx->pk.protkey, PAES_256_PROTKEY_SIZE);
>> -	spin_unlock_bh(&ctx->pk_lock);
>> +	req_ctx->modifier = modifier;
>> +	req_ctx->param_init_done = false;
>> 
>> -	while ((nbytes = walk.nbytes) != 0) {
>> -		/* only use complete blocks */
>> -		n = nbytes & ~(AES_BLOCK_SIZE - 1);
>> -		k = cpacf_km(ctx->fc | modifier, &param,
>> -			     walk.dst.virt.addr, walk.src.virt.addr, n);
>> -		if (k)
>> -			rc = skcipher_walk_done(&walk, nbytes - k);
>> -		if (k < n) {
>> -			if (__paes_convert_key(ctx))
>> -				return skcipher_walk_done(&walk, -EIO);
>> -			spin_lock_bh(&ctx->pk_lock);
>> -			memcpy(param.key, ctx->pk.protkey, PAES_256_PROTKEY_SIZE);
>> -			spin_unlock_bh(&ctx->pk_lock);
>> -		}
>> +	rc = ecb_paes_do_crypt(ctx, req_ctx, false);
>> +	if (rc != -EKEYEXPIRED) {
>> +		if (rc)
>> +			skcipher_walk_done(walk, rc);
>> +		goto out;
>>  	}
>> +
>> +	rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, 
>> req);
>> +	if (rc)
>> +		goto out;
>> +
>> +	rc = -EINPROGRESS;
>> +
>> +out:
>> +	if (rc != -EINPROGRESS)
>> +		memzero_explicit(&req_ctx->param, sizeof(req_ctx->param));
>> +	pr_debug("rc=%d\n", rc);
>>  	return rc;
> 
> If took me a while to find the synchronous good case code path. I
> would prefer to handle the various cases separately, either with a
> switch/case or by explicit checks in the main path.
> 
> rc = ecb_paes_do_crypt(ctx, req_ctx, false);
> if (rc == -EKEYEXPIRED) {
> 	rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, 
> req);
> 	rc = rc ?: -EINPROGRESS;
> } else if (rc) {
> 	skcipher_walk_done(walk, rc);
> }
> 
> if (rc != -EINPROGRESS)
> 	memzero_explicit(&req_ctx->param, sizeof(req_ctx->param));
> pr_debug("rc=%d\n", rc);
> return rc;
> 

reworked this and 7 other places.

>>  }
>> 
>> @@ -310,112 +473,242 @@ static int ecb_paes_decrypt(struct 
>> skcipher_request *req)
>>  	return ecb_paes_crypt(req, CPACF_DECRYPT);
>>  }
>> 
>> -static struct skcipher_alg ecb_paes_alg = {
>> -	.base.cra_name		=	"ecb(paes)",
>> -	.base.cra_driver_name	=	"ecb-paes-s390",
>> -	.base.cra_priority	=	401,	/* combo: aes + ecb + 1 */
>> -	.base.cra_blocksize	=	AES_BLOCK_SIZE,
>> -	.base.cra_ctxsize	=	sizeof(struct s390_paes_ctx),
>> -	.base.cra_module	=	THIS_MODULE,
>> -	.base.cra_list		=	LIST_HEAD_INIT(ecb_paes_alg.base.cra_list),
>> -	.init			=	ecb_paes_init,
>> -	.exit			=	ecb_paes_exit,
>> -	.min_keysize		=	PAES_MIN_KEYSIZE,
>> -	.max_keysize		=	PAES_MAX_KEYSIZE,
>> -	.setkey			=	ecb_paes_set_key,
>> -	.encrypt		=	ecb_paes_encrypt,
>> -	.decrypt		=	ecb_paes_decrypt,
>> -};
>> -
>> -static int cbc_paes_init(struct crypto_skcipher *tfm)
>> +static int ecb_paes_init(struct crypto_skcipher *tfm)
>>  {
>>  	struct s390_paes_ctx *ctx = crypto_skcipher_ctx(tfm);
>> 
>> -	ctx->kb.key = NULL;
>> +	memset(ctx, 0, sizeof(*ctx));
>>  	spin_lock_init(&ctx->pk_lock);
>> 
>> +	crypto_skcipher_set_reqsize(tfm, sizeof(struct s390_pecb_req_ctx));
>> +
>>  	return 0;
>>  }
>> 
>> -static void cbc_paes_exit(struct crypto_skcipher *tfm)
>> +static void ecb_paes_exit(struct crypto_skcipher *tfm)
>>  {
>>  	struct s390_paes_ctx *ctx = crypto_skcipher_ctx(tfm);
>> 
>> -	_free_kb_keybuf(&ctx->kb);
>> +	memzero_explicit(ctx, sizeof(*ctx));
>>  }
>> 
>> -static inline int __cbc_paes_set_key(struct s390_paes_ctx *ctx)
>> +static int ecb_paes_do_one_request(struct crypto_engine *engine, void 
>> *areq)
>>  {
>> -	unsigned long fc;
>> +	struct skcipher_request *req = skcipher_request_cast(areq);
>> +	struct s390_pecb_req_ctx *req_ctx = skcipher_request_ctx(req);
>> +	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
>> +	struct s390_paes_ctx *ctx = crypto_skcipher_ctx(tfm);
>> +	struct skcipher_walk *walk = &req_ctx->walk;
>>  	int rc;
>> 
>> -	rc = __paes_convert_key(ctx);
>> -	if (rc)
>> -		return rc;
>> -
>> -	/* Pick the correct function code based on the protected key type */
>> -	fc = (ctx->pk.type == PKEY_KEYTYPE_AES_128) ? CPACF_KMC_PAES_128 :
>> -		(ctx->pk.type == PKEY_KEYTYPE_AES_192) ? CPACF_KMC_PAES_192 :
>> -		(ctx->pk.type == PKEY_KEYTYPE_AES_256) ? CPACF_KMC_PAES_256 : 0;
>> +	/* walk has already been prepared */
>> 
>> -	/* Check if the function code is available */
>> -	ctx->fc = (fc && cpacf_test_func(&kmc_functions, fc)) ? fc : 0;
>> +	rc = ecb_paes_do_crypt(ctx, req_ctx, true);
>> +	if (rc != -EKEYEXPIRED) {
>> +		if (rc)
>> +			skcipher_walk_done(walk, rc);
>> +		goto complete;
>> +	}
> 
> Same here, I would prefer to reverse the logic of the error handling.
> 
>> 
>> -	return ctx->fc ? 0 : -EINVAL;
>> +	/*
>> +	 * Protected key expired, conversion is in process.
>> +	 * Trigger a re-schedule of this request by returning
>> +	 * -ENOSPC ("hardware queue is full") to the crypto engine.
>> +	 * To avoid immediately re-invocation of this callback,
>> +	 * tell the scheduler to voluntarily give up the CPU here.
>> +	 */
>> +	yield();
> 
> As mentioned by checkpatch.pl, the use of yield() should be avoided.
> Please use alternatives (e.g. cond_reschedule()).
> 

done - replaced with cond_resched()

>> +	pr_debug("rescheduling request\n");
>> +	return -ENOSPC;
>> +
>> +complete:
>> +	memzero_explicit(&req_ctx->param, sizeof(req_ctx->param));
>> +	pr_debug("request complete with rc=%d\n", rc);
>> +	local_bh_disable();
>> +	crypto_finalize_skcipher_request(engine, req, rc);
>> +	local_bh_enable();
>> +	return rc;
>>  }
> 
> [...]

Thanks, see v4 for the changes.