| Message ID | 20210623135600.n343aglmvu272fsg@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] TPM DEVICE DRIVER changes for v5.14 | expand |
On Wed, Jun 23, 2021 at 6:56 AM Jarkko Sakkinen <jarkko@kernel.org> wrote: > > Contains bug fixes for TPM, and support for signing modules using elliptic > curve keys, which I promised to pick up to my tree. I pulled this, but then I looked at the key type changes, and that made me so scared that I unpulled it again. In particular, that code will do shell rm -f $(CONFIG_MODULE_SIG_KEY) from the Makefile if some config options have changed. That just seems too broken for words. Maybe I misunderstand this, but this really seems like an easy mistake might cause the kernel build to actively start removing some random user key files that the user pointed at previously. Linus
On 6/28/21 1:34 PM, Linus Torvalds wrote: > On Wed, Jun 23, 2021 at 6:56 AM Jarkko Sakkinen <jarkko@kernel.org> wrote: >> Contains bug fixes for TPM, and support for signing modules using elliptic >> curve keys, which I promised to pick up to my tree. > I pulled this, but then I looked at the key type changes, and that > made me so scared that I unpulled it again. > > In particular, that code will do > > shell rm -f $(CONFIG_MODULE_SIG_KEY) > > from the Makefile if some config options have changed. I suppose it is from this part here. +# Support user changing key type +ifdef CONFIG_MODULE_SIG_KEY_TYPE_ECDSA +keytype_openssl = -newkey ec -pkeyopt ec_paramgen_curve:secp384r1 +ifeq ($(openssl_available),yes) +$(if $(findstring id-ecPublicKey,$(X509TEXT)),,$(shell rm -f $(CONFIG_MODULE_SIG_KEY))) +endif +endif # CONFIG_MODULE_SIG_KEY_TYPE_ECDSA + +ifdef CONFIG_MODULE_SIG_KEY_TYPE_RSA +ifeq ($(openssl_available),yes) $(if $(findstring rsaEncryption,$(X509TEXT)),,$(shell rm -f $(CONFIG_MODULE_SIG_KEY))) endif +endif # CONFIG_MODULE_SIG_KEY_TYPE_RSA If the user changed the build option from an ECDSA module signing key to an RSA signing key or vice versa then this code deletes the current signing key and subsequent code in the Makefile will create an RSA or ECDSA signing key following the user's choice. I suppose this is expected behavior that when a user chooses an RSA signing key it will use an RSA signing key. Maybe we should make a backup copy of the previous key, if that helps. > > That just seems too broken for words. Maybe I misunderstand this, but > this really seems like an easy mistake might cause the kernel build to > actively start removing some random user key files that the user > pointed at previously. The removal is triggered by the user changing the type of key from what is in the keyfile. Stefan > > Linus
On Mon, Jun 28, 2021 at 11:33 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > > The removal is triggered by the user changing the type of key from what > is in the keyfile. I understand. But if I earlier pointed the kernel config to one my RSA keys, and then I change some key type config option to something else, I sure as hell don't want to perhaps lose my key as a result. Yes, one common situation is that the key is some automatically generated one. That's what I use personally - I want a temporary key that is thrown away and never exists except for validating that "yup, I built these modules for this kernel". Removing that temporary key is fine. But if I pointed MODULE_SIG_KEY to something outside the kernel build, I sure as hell don't want the kernel build deleting it. Ever. In fact, it should never write to it. It should extract the key information from it, and nothing else. So no. No backups either. Because there is not a single valid situation where you'd want a backup - because the kernel build should never EVER modify the original. Maybe I misunderstand what is going on, but I think the whole thing is completely wrongly designed. The _only_ key that the kernel build should touchn is the auto-generated throw-away one (ie "certs/signing_key.pem"), not CONFIG_MODULE_SIG_KEY in general. Linus
On 6/28/21 3:11 PM, Linus Torvalds wrote: > On Mon, Jun 28, 2021 at 11:33 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >> The removal is triggered by the user changing the type of key from what >> is in the keyfile. > > > So no. No backups either. Because there is not a single valid > situation where you'd want a backup - because the kernel build should > never EVER modify the original. > > Maybe I misunderstand what is going on, but I think the whole thing is > completely wrongly designed. The _only_ key that the kernel build > should touchn is the auto-generated throw-away one (ie > "certs/signing_key.pem"), not CONFIG_MODULE_SIG_KEY in general. Correct, and the code (certs/Makefile) is surrounded by the check for this particular file here, so it won't touch anything else: [...] ifeq ($(CONFIG_MODULE_SIG_KEY),"certs/signing_key.pem") ifeq ($(openssl_available),yes) X509TEXT=$(shell openssl x509 -in $(CONFIG_MODULE_SIG_KEY) -text) endif # Support user changing key type ifdef CONFIG_MODULE_SIG_KEY_TYPE_ECDSA keytype_openssl = -newkey ec -pkeyopt ec_paramgen_curve:secp384r1 ifeq ($(openssl_available),yes) $(if $(findstring id-ecPublicKey,$(X509TEXT)),,$(shell rm -f $(CONFIG_MODULE_SIG_KEY))) endif endif # CONFIG_MODULE_SIG_KEY_TYPE_ECDSA ifdef CONFIG_MODULE_SIG_KEY_TYPE_RSA ifeq ($(openssl_available),yes) $(if $(findstring rsaEncryption,$(X509TEXT)),,$(shell rm -f $(CONFIG_MODULE_SIG_KEY))) endif endif # CONFIG_MODULE_SIG_KEY_TYPE_RSA [...] There's one dent in this patch series that requires suppressing an error output: https://lkml.org/lkml/2021/6/25/452 Stefan > > Linus
On Mon, Jun 28, 2021 at 12:21 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > > Correct, and the code (certs/Makefile) is surrounded by the check for > this particular file here, so it won't touch anything else: Ahh, I missed that part. Can we just make it really really obvious, and not use CONFIG_MODULE_SIG_KEY at all, then? IOW, make these literally be about "certs/signing_key.pem" and nothing else, so that when people grep for this, or look at the Makefile, they don't fall into that trap I fell into? That also would make it obvious that there are no pathname quoting issues etc. Linus
On 6/28/21 3:27 PM, Linus Torvalds wrote: > On Mon, Jun 28, 2021 at 12:21 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >> Correct, and the code (certs/Makefile) is surrounded by the check for >> this particular file here, so it won't touch anything else: > Ahh, I missed that part. > > Can we just make it really really obvious, and not use > CONFIG_MODULE_SIG_KEY at all, then? > > IOW, make these literally be about "certs/signing_key.pem" and nothing > else, so that when people grep for this, or look at the Makefile, they > don't fall into that trap I fell into? Yes, sir. Stefan > > That also would make it obvious that there are no pathname quoting issues etc. > > Linus
On Mon, Jun 28, 2021 at 10:34:26AM -0700, Linus Torvalds wrote: > On Wed, Jun 23, 2021 at 6:56 AM Jarkko Sakkinen <jarkko@kernel.org> wrote: > > > > Contains bug fixes for TPM, and support for signing modules using elliptic > > curve keys, which I promised to pick up to my tree. > > I pulled this, but then I looked at the key type changes, and that > made me so scared that I unpulled it again. > > In particular, that code will do > > shell rm -f $(CONFIG_MODULE_SIG_KEY) > > from the Makefile if some config options have changed. > > That just seems too broken for words. Maybe I misunderstand this, but > this really seems like an easy mistake might cause the kernel build to > actively start removing some random user key files that the user > pointed at previously. > > Linus Since there was still a new fix for the series [*], I'd rather refine the pull request without these patches, and not risk them being blocker for the rest of the commits. [*] https://lore.kernel.org/linux-integrity/20210629201257.dr77kemy66mxpox5@kernel.org/ /Jarkko
On Tue, Jun 29, 2021 at 1:20 PM Jarkko Sakkinen <jarkko@kernel.org> wrote: > > Since there was still a new fix for the series [*], I'd rather refine > the pull request without these patches, and not risk them being blocker > for the rest of the commits. They seemed to be just the last two commits at the end of the series, so I could take everything up to 0178f9d0f60b ("tpm: Replace WARN_ONCE() with dev_err_once() in tpm_tis_status()") perhaps? I can do that even without a new pull request (I've done that kind of thing before where I decide to pull everything but the last few commits). But admittedly I'd prefer to see a new pull request just so that I get a signed tag (which I wouldn't get if I just merged that top commit). Linus
On Tue, Jun 29, 2021 at 02:08:53PM -0700, Linus Torvalds wrote: > On Tue, Jun 29, 2021 at 1:20 PM Jarkko Sakkinen <jarkko@kernel.org> wrote: > > > > Since there was still a new fix for the series [*], I'd rather refine > > the pull request without these patches, and not risk them being blocker > > for the rest of the commits. > > They seemed to be just the last two commits at the end of the series, > so I could take everything up to 0178f9d0f60b ("tpm: Replace > WARN_ONCE() with dev_err_once() in tpm_tis_status()") perhaps? > > I can do that even without a new pull request (I've done that kind of > thing before where I decide to pull everything but the last few > commits). But admittedly I'd prefer to see a new pull request just so > that I get a signed tag (which I wouldn't get if I just merged that > top commit). > > Linus OK, that would be great! Please, do that. Thank you. /Jarkko
Hi, Contains bug fixes for TPM, and support for signing modules using elliptic curve keys, which I promised to pick up to my tree. /Jarkko The following changes since commit 0c18f29aae7ce3dadd26d8ee3505d07cc982df75: module: limit enabling module.sig_enforce (2021-06-22 11:13:19 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/ tags/tpmdd-next-v5.14-rc1 for you to fetch changes up to 87e9688481163dee836c7f86e02f9aaf3240af2e: certs: Add support for using elliptic curve keys for signing modules (2021-06-23 16:51:04 +0300) ---------------------------------------------------------------- tpmdd updates for Linux v5.14-rc1 ---------------------------------------------------------------- Amir Mizinski (1): tpm: add longer timeout for TPM2_CC_VERIFY_SIGNATURE Jarkko Sakkinen (1): tpm: Replace WARN_ONCE() with dev_err_once() in tpm_tis_status() Javier Martinez Canillas (1): tpm_tis_spi: add missing SPI device ID entries Liguang Zhang (1): tpm_tis_spi: set default probe function if device id not match Stefan Berger (2): certs: Trigger creation of RSA module signing key if it's not an RSA key certs: Add support for using elliptic curve keys for signing modules Tian Tao (2): tpm_crb: Use IOMEM_ERR_PTR when function returns iomem char: tpm: move to use request_irq by IRQF_NO_AUTOEN flag Yang Yingliang (1): tpm: fix some doc warnings in tpm1-cmd.c Zhen Lei (1): tpm_tis: Use DEFINE_RES_MEM() to simplify code certs/Kconfig | 26 ++++++++++++++++++++++++++ certs/Makefile | 21 +++++++++++++++++++++ crypto/asymmetric_keys/pkcs7_parser.c | 8 ++++++++ drivers/char/tpm/tpm1-cmd.c | 4 ++-- drivers/char/tpm/tpm2-cmd.c | 2 +- drivers/char/tpm/tpm_crb.c | 2 +- drivers/char/tpm/tpm_tis.c | 6 +----- drivers/char/tpm/tpm_tis_core.c | 25 ++++++++++++++++++------- drivers/char/tpm/tpm_tis_core.h | 3 ++- drivers/char/tpm/tpm_tis_i2c_cr50.c | 4 ++-- drivers/char/tpm/tpm_tis_spi_main.c | 14 ++++++++++---- 11 files changed, 92 insertions(+), 23 deletions(-)