| Message ID | 4d7e38ff5bbc496cb794b50e1c5c83bcd2317e69.camel@huaweicloud.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [GIT,PULL] Asymmetric keys fix for v6.4-rc5 | expand |
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. > 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... 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 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@huaweicloud.com/ 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 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
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 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. - Eric
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 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. In the mean time feel free to take this patch as it appears to be correct and should keep things chugging along while we work on the API. Acked-by: Herbert Xu <herbert@gondor.apana.org.au> Cheers,
Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > Here is a small fix to make an unconditional copy of the buffer passed > to crypto operations, to take into account the case of the stack not in > the linear mapping area. I wonder if evm_verify_hmac() and other such callers of the signature verification service should be placing the data and crypto material in slab memory rather than it being on the stack. But, for the moment: Acked-by: David Howells <dhowells@redhat.com>
The pull request you sent on Fri, 02 Jun 2023 16:41:04 +0200:
> https://github.com/robertosassu/linux.git tags/asym-keys-fix-for-linus-v6.4-rc5
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/f8dba31b0a826e691949cd4fdfa5c30defaac8c5
Thank you!
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.
Hi Linus 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. I signed the tag, but probably it would not matter, since my key is not among your trusted keys. The following changes since commit 921bdc72a0d68977092d6a64855a1b8967acc1d9: Merge tag 'mmc-v6.4-rc1-2' of git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc (2023-06-02 08:35:13 -0400) are available in the Git repository at: https://github.com/robertosassu/linux.git tags/asym-keys-fix-for-linus-v6.4-rc5 for you to fetch changes up to c3d03e8e35e005e1a614e51bb59053eeb5857f76: KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() (2023-06-02 15:36:23 +0200) ---------------------------------------------------------------- Asymmetric keys fix for v6.4-rc5 Here is a small fix to make an unconditional copy of the buffer passed to crypto operations, to take into account the case of the stack not in the linear mapping area. It has been tested and verified to fix the bug. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> ---------------------------------------------------------------- Roberto Sassu (1): KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() crypto/asymmetric_keys/public_key.c | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-)