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

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

On 2025-03-07 04:01, Herbert Xu wrote:
> 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?
> 

The calling function is on this return code triggering the request 
asynch.

>> +	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.
> 

Well, yes. I should really cover all the fields from the context
with the spinlock - will do with v2.

> 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.
> 

We had this discussion already. I am aware of the fact, that this
tfm context is used by multiple requests concurrently. However, I'll
describe why these updates in the tfm context are needed and are
conform to concurrent use of the ctx.

>> -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?

Ok, will change this for version 2

> 
> Cheers,

Thanks for you feedback. Have a nice day.