On 6/3/2023 2:02 AM, Linus Torvalds wrote:
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.
Yes, we could not do it if the set_pub_key/set_priv_key methods use internally the passed pointer. I guess it depends on the methods, for RSA and ECDSA it seems fine (they copy to a different location).
The doubt comes because the buffer is freed after crypto_wait_req() and not after crypto_akcipher_set_*_key(), suggesting that it could be actually used during the crypto operation.
Rechecked the thread, and the suggestion to reuse the buffer and not append the signature and digest at the end was by Eric Biggers.
Eric, in light of this finding, should we still reuse the buffer?
Thanks
Roberto
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