Message ID | 4d7e38ff5bbc496cb794b50e1c5c83bcd2317e69.camel@huaweicloud.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp1083249vqr; Fri, 2 Jun 2023 07:51:05 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5zgIl+z9JHA2Tco5s+32KyNnVmDn9HDc5wrmTL+OAUjPi/X8eJ0B5Vy4sIL0fBPBiG3niE X-Received: by 2002:a17:902:d4c8:b0:1b1:8e8b:7f6a with SMTP id o8-20020a170902d4c800b001b18e8b7f6amr207317plg.27.1685717464778; Fri, 02 Jun 2023 07:51:04 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1685717464; cv=none; d=google.com; s=arc-20160816; b=qJNnAr4yG2ZAw9ipm4batYttUlZ3gXwVMKZgPbeYwILz1tZnAew9gAnhfg8xecQEOs gvp8vFofyFyBibGjWp7dcW2PKNRuaDszusVe0bboj0otaGCWPjTB4AJHMtBhsDaTWcZg yd3+4r2Shf3soaqYyRjp0XyqQBe6uVKekF6yuOLr5nGC7jwyDnviw7u2XAaHTzi7taNu 7lY8gEHeoWBDO+LYPG1eYi71NE2zxXEDtTSPb9M6w6uL4BTXF+EcwdJ172fe/8o6TC5b 2Z+WsfF/T+cqqO+Zs3UovnKDR251sZZWNU3VlHHFcY7CDTJxf4DaoYTqo5Wgdaf7zLaI jLMw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :user-agent:date:cc:to:from:subject:message-id; bh=YUASxAHiBXEBvDAsAsCHRSx3v3ypGC0vxL6geXN7w3I=; b=Uf/ghe4Dv6L+RDfhCU0UXCbcLtnNxU1bISMPh7NKBkAWx5WgSlGIP+OI2jw0cRYZ/B G7ehhGpP2+oeL8tKD+Z1G0BXf5Mnh+cRGLhyAMO+A4NLAWnlH6WDU3F8hjQLHWk6/Eeb erVefbmINqdmpkp4yCopv48FgF136FG75WYnIclj9VZhdrSrucAOXEMsMsT7tXND6gXj OuuUdqVDCWeUdZBWFEjG24OKf/VDr/g6d8GDn1ynWHz4wUSR6RjUhnSwC84gzdocayBb ip59z4HCsgxy9ZDPdeCEIDFlBEHUwU17Z9m0Ds9tArsenjLK31DkuhiJMO+vP0z8svP5 g91g== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id i10-20020a170902c94a00b001adc74c482asi1065950pla.151.2023.06.02.07.50.50; Fri, 02 Jun 2023 07:51:04 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235837AbjFBOlr (ORCPT <rfc822;limurcpp@gmail.com> + 99 others); Fri, 2 Jun 2023 10:41:47 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42782 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234629AbjFBOlq (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 2 Jun 2023 10:41:46 -0400 Received: from frasgout12.his.huawei.com (unknown [14.137.139.154]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4D502BC; Fri, 2 Jun 2023 07:41:44 -0700 (PDT) Received: from mail02.huawei.com (unknown [172.18.147.227]) by frasgout12.his.huawei.com (SkyGuard) with ESMTP id 4QXll85lk2z9xtRv; Fri, 2 Jun 2023 22:29:56 +0800 (CST) Received: from roberto-ThinkStation-P620 (unknown [10.204.63.22]) by APP2 (Coremail) with SMTP id GxC2BwCHUlmE_3lkXXL8Ag--.3617S2; Fri, 02 Jun 2023 15:41:19 +0100 (CET) Message-ID: <4d7e38ff5bbc496cb794b50e1c5c83bcd2317e69.camel@huaweicloud.com> Subject: [GIT PULL] Asymmetric keys fix for v6.4-rc5 From: Roberto Sassu <roberto.sassu@huaweicloud.com> To: Linus Torvalds <torvalds@linux-foundation.org> Cc: Andrew Morton <akpm@linux-foundation.org>, Eric Biggers <ebiggers@kernel.org>, Stefan Berger <stefanb@linux.ibm.com>, dhowells@redhat.com, herbert@gondor.apana.org.au, davem@davemloft.net, zohar@linux.ibm.com, dmitry.kasatkin@gmail.com, paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com, Jarkko Sakkinen <jarkko@kernel.org>, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, keyrings@vger.kernel.org, linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Date: Fri, 02 Jun 2023 16:41:04 +0200 Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.36.5-0ubuntu1 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-CM-TRANSID: GxC2BwCHUlmE_3lkXXL8Ag--.3617S2 X-Coremail-Antispam: 1UD129KBjvJXoW7tw4DCr1Dur1DGw4DGF17Jrb_yoW8XFWrp3 yfKr13Kr4Utr17tw13Jr47Cw15JrWvyr13Ja17Aw1rAF1DZr15tr4Igr4rWryrJr97Ww13 tr48Jr1UWr1DJw7anT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUk0b4IE77IF4wAFF20E14v26ryj6rWUM7CY07I20VC2zVCF04k2 6cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rwA2F7IY1VAKz4 vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Jr0_JF4l84ACjcxK6xIIjxv20xvEc7Cj xVAFwI0_Gr0_Cr1l84ACjcxK6I8E87Iv67AKxVW8JVWxJwA2z4x0Y4vEx4A2jsIEc7CjxV AFwI0_Gr1j6F4UJwAS0I0E0xvYzxvE52x082IY62kv0487Mc02F40EFcxC0VAKzVAqx4xG 6I80ewAv7VC0I7IYx2IY67AKxVWUJVWUGwAv7VC2z280aVAFwI0_Jr0_Gr1lOx8S6xCaFV Cjc4AY6r1j6r4UM4x0Y48IcVAKI48JM4IIrI8v6xkF7I0E8cxan2IY04v7MxAIw28IcxkI 7VAKI48JMxC20s026xCaFVCjc4AY6r1j6r4UMI8I3I0E5I8CrVAFwI0_Jr0_Jr4lx2IqxV Cjr7xvwVAFwI0_JrI_JrWlx4CE17CEb7AF67AKxVW8ZVWrXwCIc40Y0x0EwIxGrwCI42IY 6xIIjxv20xvE14v26r1j6r1xMIIF0xvE2Ix0cI8IcVCY1x0267AKxVW8JVWxJwCI42IY6x AIw20EY4v20xvaj40_WFyUJVCq3wCI42IY6I8E87Iv67AKxVWUJVW8JwCI42IY6I8E87Iv 6xkF7I0E14v26r4j6r4UJbIYCTnIWIevJa73UjIFyTuYvjxUFDGOUUUUU X-CM-SenderInfo: purev21wro2thvvxqx5xdzvxpfor3voofrz/1tbiAgAQBF1jj4oNfAABsd X-CFilter-Loop: Reflected X-Spam-Status: No, score=-0.9 required=5.0 tests=BAYES_00,PDS_RDNS_DYNAMIC_FP, RCVD_IN_MSPIKE_BL,RCVD_IN_MSPIKE_L3,RDNS_DYNAMIC,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1767602876161878927?= X-GMAIL-MSGID: =?utf-8?q?1767602876161878927?= |
Series |
[GIT,PULL] Asymmetric keys fix for v6.4-rc5
|
|
Pull-request
https://github.com/robertosassu/linux.git tags/asym-keys-fix-for-linus-v6.4-rc5Message
Roberto Sassu
June 2, 2023, 2:41 p.m. UTC
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(-)
Comments
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.