On Fri, 2023-06-02 at 13:38 -0400, Linus Torvalds wrote:

On Fri, Jun 2, 2023 at 10:41 AM Roberto Sassu roberto.sassu@huaweicloud.com wrote:

...

sorry for this unusual procedure of me requesting a patch to be pulled. I asked for several months the maintainers (David: asymmetric keys, Jarkko: key subsystem) to pick my patch but without any luck.

Hmm.

The patch behind that tag looks sane to me, but this is not code I am hugely familiar with.

Who is the caller that passes in the public_key_signature data on the stack to public_key_verify_signature()? This may well be the right point to move it away from the stack in order to have a valid sg-list, but even if this patch is all good, it would be nice to have the call chain documented as part of the commit message.

Oh, it seems it was only in the first version of the patch:

https://lore.kernel.org/linux-kernel/20221104122023.1750333-1-roberto.sassu@...

Originally, the kernel panic was due to EVM, but I later found that IMA Appraisal could have caused the same.

...

I signed the tag, but probably it would not matter, since my key is not among your trusted keys.

It does matter - I do pull from people even without full chains, I just end up being a lot more careful, and I still want to see the signature for any future reference...

Ok, then it makes sense to push my key to a key server.

Thanks

Roberto

DavidH, Herbert, please comment:

...

https://github.com/robertosassu/linux.git tags/asym-keys-fix-for-linus-v6.4-rc5

basically public_key_verify_signature() is passed that

 const struct public_key_signature *sig

as an argument, and currently does

    sg_init_table(src_sg, 2);
    sg_set_buf(&src_sg[0], sig->s, sig->s_size);
    sg_set_buf(&src_sg[1], sig->digest, sig->digest_size);

on it which is *not* ok if the s->s and s->digest points to stack data that ends up not dma'able because of a virtually mapped stack.

The patch re-uses the allocation it already does for the key data, and it seems sane.

But again, this is not code I look at normally, so...

           Linus

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

On 6/3/2023 6:02 PM, Eric Biggers wrote:

On Sat, Jun 03, 2023 at 12:41:00PM +0200, Roberto Sassu wrote:

...

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?

I don't think there was any "finding" here. The setkey methods in the crypto API aren't allowed to reuse the buffer they are passed, so the patch is fine.

That was the information I was missing.

Thanks!

Roberto

On Mon, 5 Jun 2023 at 10:49, Herbert Xu herbert@gondor.apana.org.au wrote:

On Fri, Jun 02, 2023 at 08:02:23PM -0400, Linus Torvalds wrote:

...

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.

You're right. Originally SG lists were used as the majority of our input came from network packets, in the form of skb's. They are easily translated into SG lists. This is still somewhat the case for parts of the Crypto API (e.g., skcipher and ahash).

However, for akcipher the only user of the underlying API is the file in question so I absolutely agree that forcing it to go through an SG list is just wrong.

I'll change the underlying akcipher interface to take pointers instead and hide the SG list stuff (along with the copying) inside API.

Could we do the same for the compression API? This is a major pain as well, and results (on my 128-core workstation) in 32 MiB permanently tied up in scratch buffers in the scomp-to-acomp adaptation layer because most of the underlying implementations are compression libraries operating on plain virtual addresses, and so the scatterlists needs to be copied into a buffer and back to perform the actual transformation.

The only user user of the async compression interface is zswap, but it blocks on the completion so it is actually synchronous as well.