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

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

On 2025-04-02 06:04, Herbert Xu wrote:
> On Tue, Apr 01, 2025 at 04:50:47PM +0200, Harald Freudenberger wrote:
>> 
>> +static int ecb_paes_do_crypt(struct s390_paes_ctx *ctx,
>> +			     struct s390_pecb_req_ctx *req_ctx,
>> +			     bool maysleep)
> 
> ...
> 
>> +	/* always walk on the ... */
>> +	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;
>> +			}
> 
> So this leaves the skcipher walk in a mapped state, to be resumed in
> a work queue later.  Now I don't believe you guys have the horror of
> HIGHMEM so it's not fatal, but it's still a bit of a hack and worthy
> of a comment to at least stop people from other architectures copying
> this.
> 

v4 will have this clearly documented.

>> +			rc = paes_convert_key(ctx);
> 
> At first I thought this was racy, but then I realised that it is not
> because only the crypto_engine thread gets called with maysleep ==
> true.  Since there is only one crypto_engine thread this is safe.
> 
> I think this is not really obvious though and worthy of a comment to
> explain the reliance on the single crypto engine thread.
> 

This is racy but the code can handle that. The cpacf instruction
refuses to do any operations if the converted key material (the 
"protected" key)
is invalid. So it is in fact thinkable and possible to replace an fresh
protected key with an older (in the meantime invalid) protected key. As 
the
cpacf instruction detects this, refuses to operate with an invalid key 
and
the calling code triggers a (re-)conversion this does no harm. So it
is racy but may only lead to additional conversions but never to invalid
data on en- or decrypted.

> There is one more subtle issue to do with request ordering.  Because
> networking requires packets to not be reordered, we enforce this in
> the Crypto API.  An algorithm must not reorder the requests sent to
> the same tfm.
> 
> To do that here, once a ctx goes into the crypto_engine, all future
> requests to the same ctx must also go through the crypto_engine, as
> long as at the time of the request being submitted prior work is still
> outstanding.
> 
> The easiest way would be to have a counter in the ctx that keeps
> track of how many requests are currently outstanding in the engine.
> Then in paes_do_crypt you'd simply check the counter, and if it is
> non-zero you always put the request into the engine.
> 

I am struggling with that. The thing is how to keep this information.
I extended the request context with a bool field telling me that there
is/was a request pushed to the engine and thus all following crypto
operations on this request need to go via engine.
BUT ... the request context is not initial zeroized and there is no
init() for a request and thus one does not know on invocation of the
skcipher encrypt or decrypt function if the value of the bool field
is taken for serious or needs initialization. Same would happen if
there is a counter instead - how to initially set the counter value
to 0? Any hints on this are welcome.

> Cheers,