On Tue, May 06, 2025 at 04:02:41PM +0200, Harald Freudenberger wrote:
>
> > > + 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.
Perhaps add this as a comment in the code?
> 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.
I think the easiest would be to copy how simd + cryptd does it.
In simd we have the same problem of a fallback path through async
cryptd, and we need to keep using that fallback once we start until
it fully drains.
So right after entering encrypt/decrypt, you check whether the
fallback is in use, and if it is, just do that and that's it.
Otherwise continue as usual:
static int simd_skcipher_encrypt(struct skcipher_request *req)
{
...
if (!crypto_simd_usable() ||
(in_atomic() && cryptd_skcipher_queued(ctx->cryptd_tfm)))
Take fallback path
else
Continue on normal path
So for paes this would look like:
static int paes_skcipher_encrypt(struct skcipher_request *req)
{
struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
if (paes_skcipher_queued(tfm))
return paes_skcipher_fallback(req);
Continue on normal path
Where paes_skcipher_queued(tfm) is just a simple ref count of the
number of entries queued by that tfm onto the fallback path. IOW
every time you enqueue something you increase the refcount and
every time you dequeue something you decrease it too.
Cheers,
--
Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
