On Fri, Jun 2, 2023 at 1:38 PM Linus Torvalds torvalds@linux-foundation.org wrote:
The patch re-uses the allocation it already does for the key data, and it seems sane.
Ugh. I had to check that it was ok to re-use the key buffer, but it does seem to be the case that you can just re-use the buffer after you've done that crypto_akcipher_set_priv/pub_key() call, and the crypto layer has to copy it into its own data structures.
I absolutely abhor the crypto interfaces. They all seem designed for that "external DMA engine" case that seems so horrendously pointless and slow. In practice so few of them are that, and we have all those optimized routines for doing it all on the CPU - but have in the meantime wasted all that time and effort into copying everything, turning simple buffers into sg-bufs etc etc. The amount of indirection and "set this state in the state machine" is just nasty, and this seems to all be a prime example of it all. With some of it then randomly going through some kthread too.
I still think that patch is probably fine, but was also going "maybe the real problem is in that library helper function (asymmetric_verify(), in this case), which takes those (sig, siglen, digest, digestlen) arguments and turns it into a 'struct public_key_signature' without marshalling them.
Just looking at this mess of indirection and different "helper" functions makes me second-guess myself about where the actual conversion should be - while also feeling like it should never have been done as a scatter-gather entry in the first place.
Anyway, I don't feel competent to decide if that pull request is the right fix or not.
But it clearly is *a* fix.
Linus