Message ID | 20240429202811.13643-19-James.Bottomley@HansenPartnership.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | add integrity and security to TPM2 transactions | expand |
On Mon, Apr 29, 2024 at 04:28:07PM -0400, James Bottomley wrote: > If some entity is snooping the TPM bus, they can see the random > numbers we're extracting from the TPM and do prediction attacks > against their consumers. Foil this attack by using response > encryption to prevent the attacker from seeing the random sequence. > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> > > --- > v7: add review > --- > drivers/char/tpm/tpm2-cmd.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index a53a843294ed..0cdf892ec2a7 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -292,25 +292,35 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max) > if (!num_bytes || max > TPM_MAX_RNG_DATA) > return -EINVAL; > > - err = tpm_buf_init(&buf, 0, 0); > + err = tpm2_start_auth_session(chip); > if (err) > return err; > > + err = tpm_buf_init(&buf, 0, 0); > + if (err) { > + tpm2_end_auth_session(chip); > + return err; > + } > + > do { > - tpm_buf_reset(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_RANDOM); > + tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM); > + tpm_buf_append_hmac_session_opt(chip, &buf, TPM2_SA_ENCRYPT > + | TPM2_SA_CONTINUE_SESSION, > + NULL, 0); > tpm_buf_append_u16(&buf, num_bytes); > + tpm_buf_fill_hmac_session(chip, &buf); > err = tpm_transmit_cmd(chip, &buf, > offsetof(struct tpm2_get_random_out, > buffer), > "attempting get random"); > + err = tpm_buf_check_hmac_response(chip, &buf, err); > if (err) { > if (err > 0) > err = -EIO; > goto out; > } > > - out = (struct tpm2_get_random_out *) > - &buf.data[TPM_HEADER_SIZE]; > + out = (struct tpm2_get_random_out *)tpm_buf_parameters(&buf); > recd = min_t(u32, be16_to_cpu(out->size), num_bytes); > if (tpm_buf_length(&buf) < > TPM_HEADER_SIZE + > @@ -327,9 +337,12 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max) > } while (retries-- && total < max); > > tpm_buf_destroy(&buf); > + tpm2_end_auth_session(chip); > + > return total ? total : -EIO; > out: > tpm_buf_destroy(&buf); > + tpm2_end_auth_session(chip); > return err; > } > > -- > 2.35.3 > Hi, KernelCI has identified a new warning and I tracked it down to this commit. It was observed on the following platforms: * mt8183-kukui-jacuzzi-juniper-sku16 * sc7180-trogdor-kingoftown (but probably affects all platforms that have a tpm driver with async probe) The warning is the following: [ 2.017338] ------------[ cut here ]------------ [ 2.025521] WARNING: CPU: 0 PID: 72 at kernel/module/kmod.c:144 __request_module+0x188/0x1f4 [ 2.039508] Modules linked in: [ 2.046447] CPU: 0 PID: 72 Comm: kworker/u34:3 Not tainted 6.9.0 #1 [ 2.046455] Hardware name: Google juniper sku16 board (DT) [ 2.046460] Workqueue: async async_run_entry_fn [ 2.060094] [ 2.060097] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 2.091758] pc : __request_module+0x188/0x1f4 [ 2.096114] lr : __request_module+0x180/0x1f4 [ 2.100468] sp : ffff80008088b400 [ 2.103777] x29: ffff80008088b400 x28: 0000000000281ae0 x27: ffffa13fd366e0d2 [ 2.110915] x26: 0000000000000000 x25: ffff2387008f33c0 x24: 00000000ffffffff [ 2.118053] x23: 000000000000200f x22: ffffa13fd0ed49de x21: 0000000000000001 [ 2.125190] x20: 0000000000000000 x19: ffffa13fd23f0be0 x18: 0000000000000014 [ 2.132327] x17: 00000000fbdae5e3 x16: 000000005bcbb9f8 x15: 000000008700f694 [ 2.139463] x14: 0000000000000001 x13: ffff80008088b850 x12: 0000000000000000 [ 2.146600] x11: 00000000f8f4a4bb x10: fffffffffdd186ae x9 : 0000000000000004 [ 2.153736] x8 : ffff2387008f33c0 x7 : 3135616873286361 x6 : 0c0406065b07370f [ 2.160873] x5 : 0f37075b0606040c x4 : 0000000000000000 x3 : 0000000000000000 [ 2.168009] x2 : ffffa13fd0ed49de x1 : ffffa13fcfcc4768 x0 : 0000000000000001 [ 2.175146] Call trace: [ 2.177587] __request_module+0x188/0x1f4 [ 2.181596] crypto_alg_mod_lookup+0x178/0x21c [ 2.186042] crypto_alloc_tfm_node+0x58/0x114 [ 2.190396] crypto_alloc_shash+0x24/0x30 [ 2.194404] drbg_init_hash_kernel+0x28/0xdc [ 2.198673] drbg_kcapi_seed+0x21c/0x420 [ 2.202593] crypto_rng_reset+0x84/0xb4 [ 2.206425] crypto_get_default_rng+0xa4/0xd8 [ 2.210779] ecc_gen_privkey+0x58/0xd0 [ 2.214526] ecdh_set_secret+0x90/0x198 [ 2.218360] tpm_buf_append_salt+0x164/0x2dc [ 2.222632] tpm2_start_auth_session+0xc8/0x29c [ 2.227162] tpm2_get_random+0x44/0x204 [ 2.230996] tpm_get_random+0x74/0x90 [ 2.234655] tpm_hwrng_read+0x24/0x30 [ 2.238314] add_early_randomness+0x68/0x118 [ 2.242584] hwrng_register+0x16c/0x218 [ 2.246418] tpm_chip_register+0xf0/0x2cc [ 2.248143] cros-ec-spi spi2.0: SPI transfer timed out [ 2.250419] tpm_tis_core_init+0x494/0x7e0 [ 2.255552] spi_master spi2: failed to transfer one message from queue [ 2.259623] tpm_tis_spi_init+0x54/0x70 [ 2.259629] cr50_spi_probe+0xf4/0x27c [ 2.266145] spi_master spi2: noqueue transfer failed [ 2.269961] tpm_tis_spi_driver_probe+0x34/0x64 [ 2.273704] cros-ec-spi spi2.0: spi transfer failed: -110 [ 2.278647] spi_probe+0x84/0xe4 [ 2.291799] really_probe+0xbc/0x2a0 [ 2.295373] __driver_probe_device+0x78/0x12c [ 2.299725] driver_probe_device+0xdc/0x160 [ 2.303903] __device_attach_driver+0xb8/0x134 [ 2.308342] bus_for_each_drv+0x84/0xe0 [ 2.312174] __device_attach_async_helper+0xac/0xd0 [ 2.317051] async_run_entry_fn+0x34/0xe0 [ 2.321058] process_one_work+0x150/0x294 [ 2.325068] worker_thread+0x304/0x408 [ 2.328816] kthread+0x118/0x11c [ 2.332045] ret_from_fork+0x10/0x20 [ 2.335621] ---[ end trace 0000000000000000 ]--- Which is generated in __request_module() here: /* * We don't allow synchronous module loading from async. Module * init may invoke async_synchronize_full() which will end up * waiting for this task which already is waiting for the module * loading to complete, leading to a deadlock. */ WARN_ON_ONCE(wait && current_is_async()); As the comment says this could lead to a deadlock so it seemed worthwhile to report and get fixed. The tpm_tis_spi driver probes asynchronously, .probe_type = PROBE_PREFER_ASYNCHRONOUS, and as part of its probe registers the tpm device which ultimately leads to a module being requested synchronously in crypto_larval_lookup(): request_module("crypto-%s-all", name); and that triggers the warning. #regzbot introduced: 1b6d7f9eb150 #regzbot title: __request_module warning: sync module loading from async tpm driver probe Thanks, Nícolas
On Thu, 2024-05-16 at 20:25 -0400, Nícolas F. R. A. Prado wrote: > On Mon, Apr 29, 2024 at 04:28:07PM -0400, James Bottomley wrote: > > If some entity is snooping the TPM bus, they can see the random > > numbers we're extracting from the TPM and do prediction attacks > > against their consumers. Foil this attack by using response > > encryption to prevent the attacker from seeing the random sequence. > > > > Signed-off-by: James Bottomley > > <James.Bottomley@HansenPartnership.com> > > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > --- > > v7: add review > > --- > > drivers/char/tpm/tpm2-cmd.c | 21 +++++++++++++++++---- > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2- > > cmd.c > > index a53a843294ed..0cdf892ec2a7 100644 > > --- a/drivers/char/tpm/tpm2-cmd.c > > +++ b/drivers/char/tpm/tpm2-cmd.c > > @@ -292,25 +292,35 @@ int tpm2_get_random(struct tpm_chip *chip, u8 > > *dest, size_t max) > > if (!num_bytes || max > TPM_MAX_RNG_DATA) > > return -EINVAL; > > > > - err = tpm_buf_init(&buf, 0, 0); > > + err = tpm2_start_auth_session(chip); > > if (err) > > return err; > > > > + err = tpm_buf_init(&buf, 0, 0); > > + if (err) { > > + tpm2_end_auth_session(chip); > > + return err; > > + } > > + > > do { > > - tpm_buf_reset(&buf, TPM2_ST_NO_SESSIONS, > > TPM2_CC_GET_RANDOM); > > + tpm_buf_reset(&buf, TPM2_ST_SESSIONS, > > TPM2_CC_GET_RANDOM); > > + tpm_buf_append_hmac_session_opt(chip, &buf, > > TPM2_SA_ENCRYPT > > + | > > TPM2_SA_CONTINUE_SESSION, > > + NULL, 0); > > tpm_buf_append_u16(&buf, num_bytes); > > + tpm_buf_fill_hmac_session(chip, &buf); > > err = tpm_transmit_cmd(chip, &buf, > > offsetof(struct > > tpm2_get_random_out, > > buffer), > > "attempting get random"); > > + err = tpm_buf_check_hmac_response(chip, &buf, err); > > if (err) { > > if (err > 0) > > err = -EIO; > > goto out; > > } > > > > - out = (struct tpm2_get_random_out *) > > - &buf.data[TPM_HEADER_SIZE]; > > + out = (struct tpm2_get_random_out > > *)tpm_buf_parameters(&buf); > > recd = min_t(u32, be16_to_cpu(out->size), > > num_bytes); > > if (tpm_buf_length(&buf) < > > TPM_HEADER_SIZE + > > @@ -327,9 +337,12 @@ int tpm2_get_random(struct tpm_chip *chip, u8 > > *dest, size_t max) > > } while (retries-- && total < max); > > > > tpm_buf_destroy(&buf); > > + tpm2_end_auth_session(chip); > > + > > return total ? total : -EIO; > > out: > > tpm_buf_destroy(&buf); > > + tpm2_end_auth_session(chip); > > return err; > > } > > > > -- > > 2.35.3 > > > > Hi, > > KernelCI has identified a new warning and I tracked it down to this > commit. It > was observed on the following platforms: > * mt8183-kukui-jacuzzi-juniper-sku16 > * sc7180-trogdor-kingoftown > (but probably affects all platforms that have a tpm driver with async > probe) > > The warning is the following: > > [ 2.017338] ------------[ cut here ]------------ > [ 2.025521] WARNING: CPU: 0 PID: 72 at kernel/module/kmod.c:144 > __request_module+0x188/0x1f4 > [ 2.039508] Modules linked in: > [ 2.046447] CPU: 0 PID: 72 Comm: kworker/u34:3 Not tainted 6.9.0 > #1 > [ 2.046455] Hardware name: Google juniper sku16 board (DT) > [ 2.046460] Workqueue: async async_run_entry_fn > [ 2.060094] > [ 2.060097] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS > BTYPE=--) > [ 2.091758] pc : __request_module+0x188/0x1f4 > [ 2.096114] lr : __request_module+0x180/0x1f4 > [ 2.100468] sp : ffff80008088b400 > [ 2.103777] x29: ffff80008088b400 x28: 0000000000281ae0 x27: > ffffa13fd366e0d2 > [ 2.110915] x26: 0000000000000000 x25: ffff2387008f33c0 x24: > 00000000ffffffff > [ 2.118053] x23: 000000000000200f x22: ffffa13fd0ed49de x21: > 0000000000000001 > [ 2.125190] x20: 0000000000000000 x19: ffffa13fd23f0be0 x18: > 0000000000000014 > [ 2.132327] x17: 00000000fbdae5e3 x16: 000000005bcbb9f8 x15: > 000000008700f694 > [ 2.139463] x14: 0000000000000001 x13: ffff80008088b850 x12: > 0000000000000000 > [ 2.146600] x11: 00000000f8f4a4bb x10: fffffffffdd186ae x9 : > 0000000000000004 > [ 2.153736] x8 : ffff2387008f33c0 x7 : 3135616873286361 x6 : > 0c0406065b07370f > [ 2.160873] x5 : 0f37075b0606040c x4 : 0000000000000000 x3 : > 0000000000000000 > [ 2.168009] x2 : ffffa13fd0ed49de x1 : ffffa13fcfcc4768 x0 : > 0000000000000001 > [ 2.175146] Call trace: > [ 2.177587] __request_module+0x188/0x1f4 > [ 2.181596] crypto_alg_mod_lookup+0x178/0x21c > [ 2.186042] crypto_alloc_tfm_node+0x58/0x114 > [ 2.190396] crypto_alloc_shash+0x24/0x30 > [ 2.194404] drbg_init_hash_kernel+0x28/0xdc > [ 2.198673] drbg_kcapi_seed+0x21c/0x420 > [ 2.202593] crypto_rng_reset+0x84/0xb4 > [ 2.206425] crypto_get_default_rng+0xa4/0xd8 > [ 2.210779] ecc_gen_privkey+0x58/0xd0 > [ 2.214526] ecdh_set_secret+0x90/0x198 > [ 2.218360] tpm_buf_append_salt+0x164/0x2dc This looks like a misconfiguration. The kernel is trying to load the ecdh module, but it should have been selected as built in by this in drivers/char/tpm/Kconfig: config TCG_TPM2_HMAC bool "Use HMAC and encrypted transactions on the TPM bus" default y select CRYPTO_ECDH select CRYPTO_LIB_AESCFB select CRYPTO_LIB_SHA256 Can you check what the status of CONFIG_CRYPTO_ECHD is for your build? Regards, James
On Fri, 17 May 2024 at 03:59, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > > On Thu, 2024-05-16 at 20:25 -0400, Nícolas F. R. A. Prado wrote: ... > > KernelCI has identified a new warning and I tracked it down to this > > commit. It > > was observed on the following platforms: > > * mt8183-kukui-jacuzzi-juniper-sku16 > > * sc7180-trogdor-kingoftown > > (but probably affects all platforms that have a tpm driver with async > > probe) > > > > [ 2.175146] Call trace: > > [ 2.177587] __request_module+0x188/0x1f4 > > [ 2.181596] crypto_alg_mod_lookup+0x178/0x21c > > [ 2.186042] crypto_alloc_tfm_node+0x58/0x114 > > [ 2.190396] crypto_alloc_shash+0x24/0x30 > > [ 2.194404] drbg_init_hash_kernel+0x28/0xdc > > [ 2.198673] drbg_kcapi_seed+0x21c/0x420 > > [ 2.202593] crypto_rng_reset+0x84/0xb4 > > [ 2.206425] crypto_get_default_rng+0xa4/0xd8 > > [ 2.210779] ecc_gen_privkey+0x58/0xd0 > > [ 2.214526] ecdh_set_secret+0x90/0x198 > > [ 2.218360] tpm_buf_append_salt+0x164/0x2dc > > This looks like a misconfiguration. The kernel is trying to load the > ecdh module, but it should have been selected as built in by this in > drivers/char/tpm/Kconfig: > > config TCG_TPM2_HMAC > bool "Use HMAC and encrypted transactions on the TPM bus" > default y > select CRYPTO_ECDH > select CRYPTO_LIB_AESCFB > select CRYPTO_LIB_SHA256 > The module request is not for ECDH itself but for the DRBG it attempts to use to generate the secret. Given that CRYPTO_ECDH does not strictly require a DRBG in principle, but does in this particular case, I think it makes sense to select CRYPTO_DRBG here (or depend on it being builtin), rather than updating the Kconfig rules for CRYPTO_ECDH itself.
On Fri May 17, 2024 at 10:20 AM EEST, Ard Biesheuvel wrote: > On Fri, 17 May 2024 at 03:59, James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > > > On Thu, 2024-05-16 at 20:25 -0400, Nícolas F. R. A. Prado wrote: > ... > > > KernelCI has identified a new warning and I tracked it down to this > > > commit. It > > > was observed on the following platforms: > > > * mt8183-kukui-jacuzzi-juniper-sku16 > > > * sc7180-trogdor-kingoftown > > > (but probably affects all platforms that have a tpm driver with async > > > probe) > > > > > > [ 2.175146] Call trace: > > > [ 2.177587] __request_module+0x188/0x1f4 > > > [ 2.181596] crypto_alg_mod_lookup+0x178/0x21c > > > [ 2.186042] crypto_alloc_tfm_node+0x58/0x114 > > > [ 2.190396] crypto_alloc_shash+0x24/0x30 > > > [ 2.194404] drbg_init_hash_kernel+0x28/0xdc > > > [ 2.198673] drbg_kcapi_seed+0x21c/0x420 > > > [ 2.202593] crypto_rng_reset+0x84/0xb4 > > > [ 2.206425] crypto_get_default_rng+0xa4/0xd8 > > > [ 2.210779] ecc_gen_privkey+0x58/0xd0 > > > [ 2.214526] ecdh_set_secret+0x90/0x198 > > > [ 2.218360] tpm_buf_append_salt+0x164/0x2dc > > > > This looks like a misconfiguration. The kernel is trying to load the > > ecdh module, but it should have been selected as built in by this in > > drivers/char/tpm/Kconfig: > > > > config TCG_TPM2_HMAC > > bool "Use HMAC and encrypted transactions on the TPM bus" > > default y > > select CRYPTO_ECDH > > select CRYPTO_LIB_AESCFB > > select CRYPTO_LIB_SHA256 > > > > The module request is not for ECDH itself but for the DRBG it attempts > to use to generate the secret. > > Given that CRYPTO_ECDH does not strictly require a DRBG in principle, > but does in this particular case, I think it makes sense to select > CRYPTO_DRBG here (or depend on it being builtin), rather than updating > the Kconfig rules for CRYPTO_ECDH itself. I can spin a new PR if James can make a fix. All previous 4 PR's for 6.10 were applied to Linus' tree so my queue is empty. Need to have both fixes and stable-tags to save my bandwidth. Maybe for transcript just two first lines denoting that it was __request_module() will do. That and adding CONFIG_DRBG will take it away should be enough for the full disclosure, right? BR, Jarkko
On Fri, 2024-05-17 at 09:20 +0200, Ard Biesheuvel wrote: > On Fri, 17 May 2024 at 03:59, James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > > > On Thu, 2024-05-16 at 20:25 -0400, Nícolas F. R. A. Prado wrote: > ... > > > KernelCI has identified a new warning and I tracked it down to > > > this > > > commit. It > > > was observed on the following platforms: > > > * mt8183-kukui-jacuzzi-juniper-sku16 > > > * sc7180-trogdor-kingoftown > > > (but probably affects all platforms that have a tpm driver with > > > async > > > probe) > > > > > > [ 2.175146] Call trace: > > > [ 2.177587] __request_module+0x188/0x1f4 > > > [ 2.181596] crypto_alg_mod_lookup+0x178/0x21c > > > [ 2.186042] crypto_alloc_tfm_node+0x58/0x114 > > > [ 2.190396] crypto_alloc_shash+0x24/0x30 > > > [ 2.194404] drbg_init_hash_kernel+0x28/0xdc > > > [ 2.198673] drbg_kcapi_seed+0x21c/0x420 > > > [ 2.202593] crypto_rng_reset+0x84/0xb4 > > > [ 2.206425] crypto_get_default_rng+0xa4/0xd8 > > > [ 2.210779] ecc_gen_privkey+0x58/0xd0 > > > [ 2.214526] ecdh_set_secret+0x90/0x198 > > > [ 2.218360] tpm_buf_append_salt+0x164/0x2dc > > > > This looks like a misconfiguration. The kernel is trying to load > > the > > ecdh module, but it should have been selected as built in by this > > in > > drivers/char/tpm/Kconfig: > > > > config TCG_TPM2_HMAC > > bool "Use HMAC and encrypted transactions on the TPM bus" > > default y > > select CRYPTO_ECDH > > select CRYPTO_LIB_AESCFB > > select CRYPTO_LIB_SHA256 > > > > The module request is not for ECDH itself but for the DRBG it > attempts > to use to generate the secret. > > Given that CRYPTO_ECDH does not strictly require a DRBG in principle, > but does in this particular case, I think it makes sense to select > CRYPTO_DRBG here (or depend on it being builtin), rather than > updating the Kconfig rules for CRYPTO_ECDH itself. Thanks for the analysis. If I look at how CRYPTO_ECC does it, that selects CRYPTO_RNG_DEFAULT which pulls in CRYPTO_DRBG, so the fix would be the attached. Does that look right to you Ard? And does it work Nicolas? James ---8>8>8><8<8<8--- From 8c60ffd959eaa65627aca6596c35bb9cbfd9bee6 Mon Sep 17 00:00:00 2001 From: James Bottomley <James.Bottomley@HansenPartnership.com> Date: Fri, 17 May 2024 06:29:31 -0700 Subject: [PATCH] tpm: Fix sessions cryptography requirement for Random Numbers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ECDH code in tpm2-sessions.c requires an initial random number generator to generate the key pair. If the configuration doesn't have CONFIG_RNG_DEFAULT, it will try to pull this in as a module (which is impossible for the early kernel boot where the TPM starts). Fix this by selecting the required RNG. Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> Fixes: 1b6d7f9eb150 ("tpm: add session encryption protection to tpm2_get_random()") Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> --- drivers/char/tpm/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig index 4f83ee7021d0..12065eddb922 100644 --- a/drivers/char/tpm/Kconfig +++ b/drivers/char/tpm/Kconfig @@ -31,6 +31,7 @@ config TCG_TPM2_HMAC bool "Use HMAC and encrypted transactions on the TPM bus" default y select CRYPTO_ECDH + select CRYTPO_RNG_DEFAULT select CRYPTO_LIB_AESCFB select CRYPTO_LIB_SHA256 help
On Fri, 17 May 2024 at 15:35, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > > On Fri, 2024-05-17 at 09:20 +0200, Ard Biesheuvel wrote: > > On Fri, 17 May 2024 at 03:59, James Bottomley > > <James.Bottomley@hansenpartnership.com> wrote: > > > > > > On Thu, 2024-05-16 at 20:25 -0400, Nícolas F. R. A. Prado wrote: > > ... > > > > KernelCI has identified a new warning and I tracked it down to > > > > this > > > > commit. It > > > > was observed on the following platforms: > > > > * mt8183-kukui-jacuzzi-juniper-sku16 > > > > * sc7180-trogdor-kingoftown > > > > (but probably affects all platforms that have a tpm driver with > > > > async > > > > probe) > > > > > > > > [ 2.175146] Call trace: > > > > [ 2.177587] __request_module+0x188/0x1f4 > > > > [ 2.181596] crypto_alg_mod_lookup+0x178/0x21c > > > > [ 2.186042] crypto_alloc_tfm_node+0x58/0x114 > > > > [ 2.190396] crypto_alloc_shash+0x24/0x30 > > > > [ 2.194404] drbg_init_hash_kernel+0x28/0xdc > > > > [ 2.198673] drbg_kcapi_seed+0x21c/0x420 > > > > [ 2.202593] crypto_rng_reset+0x84/0xb4 > > > > [ 2.206425] crypto_get_default_rng+0xa4/0xd8 > > > > [ 2.210779] ecc_gen_privkey+0x58/0xd0 > > > > [ 2.214526] ecdh_set_secret+0x90/0x198 > > > > [ 2.218360] tpm_buf_append_salt+0x164/0x2dc > > > > > > This looks like a misconfiguration. The kernel is trying to load > > > the > > > ecdh module, but it should have been selected as built in by this > > > in > > > drivers/char/tpm/Kconfig: > > > > > > config TCG_TPM2_HMAC > > > bool "Use HMAC and encrypted transactions on the TPM bus" > > > default y > > > select CRYPTO_ECDH > > > select CRYPTO_LIB_AESCFB > > > select CRYPTO_LIB_SHA256 > > > > > > > The module request is not for ECDH itself but for the DRBG it > > attempts > > to use to generate the secret. > > > > Given that CRYPTO_ECDH does not strictly require a DRBG in principle, > > but does in this particular case, I think it makes sense to select > > CRYPTO_DRBG here (or depend on it being builtin), rather than > > updating the Kconfig rules for CRYPTO_ECDH itself. > > Thanks for the analysis. If I look at how CRYPTO_ECC does it, that > selects CRYPTO_RNG_DEFAULT which pulls in CRYPTO_DRBG, so the fix would > be the attached. Does that look right to you Ard? No it doesn't - it's CRYPTO_RNG_DEFAULT not CRYTPO_RNG_DEFAULT :-) With that fixed, Acked-by: Ard Biesheuvel <ardb@kernel.org> > And does it work > Nicolas? > > James > > ---8>8>8><8<8<8--- > > From 8c60ffd959eaa65627aca6596c35bb9cbfd9bee6 Mon Sep 17 00:00:00 2001 > From: James Bottomley <James.Bottomley@HansenPartnership.com> > Date: Fri, 17 May 2024 06:29:31 -0700 > Subject: [PATCH] tpm: Fix sessions cryptography requirement for Random Numbers > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > The ECDH code in tpm2-sessions.c requires an initial random number > generator to generate the key pair. If the configuration doesn't have > CONFIG_RNG_DEFAULT, it will try to pull this in as a module (which is > impossible for the early kernel boot where the TPM starts). Fix this > by selecting the required RNG. > > Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > Fixes: 1b6d7f9eb150 ("tpm: add session encryption protection to tpm2_get_random()") > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > --- > drivers/char/tpm/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig > index 4f83ee7021d0..12065eddb922 100644 > --- a/drivers/char/tpm/Kconfig > +++ b/drivers/char/tpm/Kconfig > @@ -31,6 +31,7 @@ config TCG_TPM2_HMAC > bool "Use HMAC and encrypted transactions on the TPM bus" > default y > select CRYPTO_ECDH > + select CRYTPO_RNG_DEFAULT > select CRYPTO_LIB_AESCFB > select CRYPTO_LIB_SHA256 > help > -- > 2.35.3 > >
On Fri, 2024-05-17 at 15:43 +0200, Ard Biesheuvel wrote: > On Fri, 17 May 2024 at 15:35, James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: [...] > > Thanks for the analysis. If I look at how CRYPTO_ECC does it, that > > selects CRYPTO_RNG_DEFAULT which pulls in CRYPTO_DRBG, so the fix > > would be the attached. Does that look right to you Ard? > > No it doesn't - it's CRYPTO_RNG_DEFAULT not CRYTPO_RNG_DEFAULT :-) > > With that fixed, > > Acked-by: Ard Biesheuvel <ardb@kernel.org> Erm, oops, sorry about that; so attached is the update. James ---8>8>8><8<8<8--- From 2ac337a33e6416ef806e2c692b9239d193e8468f Mon Sep 17 00:00:00 2001 From: James Bottomley <James.Bottomley@HansenPartnership.com> Date: Fri, 17 May 2024 06:29:31 -0700 Subject: [PATCH] tpm: Fix sessions cryptography requirement for Random Numbers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ECDH code in tpm2-sessions.c requires an initial random number generator to generate the key pair. If the configuration doesn't have CONFIG_RNG_DEFAULT, it will try to pull this in as a module (which is impossible for the early kernel boot where the TPM starts). Fix this by selecting the required RNG. Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> Fixes: 1b6d7f9eb150 ("tpm: add session encryption protection to tpm2_get_random()") Acked-by: Ard Biesheuvel <ardb@kernel.org> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> --- drivers/char/tpm/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig index 4f83ee7021d0..ecdd3db4be2b 100644 --- a/drivers/char/tpm/Kconfig +++ b/drivers/char/tpm/Kconfig @@ -31,6 +31,7 @@ config TCG_TPM2_HMAC bool "Use HMAC and encrypted transactions on the TPM bus" default y select CRYPTO_ECDH + select CRYPTO_RNG_DEFAULT select CRYPTO_LIB_AESCFB select CRYPTO_LIB_SHA256 help
On Fri, May 17, 2024 at 07:25:40AM -0700, James Bottomley wrote: > On Fri, 2024-05-17 at 15:43 +0200, Ard Biesheuvel wrote: > > On Fri, 17 May 2024 at 15:35, James Bottomley > > <James.Bottomley@hansenpartnership.com> wrote: > [...] > > > Thanks for the analysis. If I look at how CRYPTO_ECC does it, that > > > selects CRYPTO_RNG_DEFAULT which pulls in CRYPTO_DRBG, so the fix > > > would be the attached. Does that look right to you Ard? > > > > No it doesn't - it's CRYPTO_RNG_DEFAULT not CRYTPO_RNG_DEFAULT :-) > > > > With that fixed, > > > > Acked-by: Ard Biesheuvel <ardb@kernel.org> > > Erm, oops, sorry about that; so attached is the update. > > James > > ---8>8>8><8<8<8--- > > From 2ac337a33e6416ef806e2c692b9239d193e8468f Mon Sep 17 00:00:00 2001 > From: James Bottomley <James.Bottomley@HansenPartnership.com> > Date: Fri, 17 May 2024 06:29:31 -0700 > Subject: [PATCH] tpm: Fix sessions cryptography requirement for Random Numbers > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > The ECDH code in tpm2-sessions.c requires an initial random number > generator to generate the key pair. If the configuration doesn't have > CONFIG_RNG_DEFAULT, it will try to pull this in as a module (which is > impossible for the early kernel boot where the TPM starts). Fix this > by selecting the required RNG. > > Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > Fixes: 1b6d7f9eb150 ("tpm: add session encryption protection to tpm2_get_random()") > Acked-by: Ard Biesheuvel <ardb@kernel.org> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > --- > drivers/char/tpm/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig > index 4f83ee7021d0..ecdd3db4be2b 100644 > --- a/drivers/char/tpm/Kconfig > +++ b/drivers/char/tpm/Kconfig > @@ -31,6 +31,7 @@ config TCG_TPM2_HMAC > bool "Use HMAC and encrypted transactions on the TPM bus" > default y > select CRYPTO_ECDH > + select CRYPTO_RNG_DEFAULT > select CRYPTO_LIB_AESCFB > select CRYPTO_LIB_SHA256 > help > -- > 2.35.3 > > Hi James, thanks for the patch. But I actually already had that config enabled builtin. I also had ECDH and DRBG which have been suggested previously: CONFIG_CRYPTO_RNG_DEFAULT=y CONFIG_CRYPTO_DRBG_MENU=y CONFIG_CRYPTO_DRBG_HMAC=y # CONFIG_CRYPTO_DRBG_HASH is not set # CONFIG_CRYPTO_DRBG_CTR is not set CONFIG_CRYPTO_DRBG=y CONFIG_CRYPTO_ECDH=y I've pasted my full config here: http://0x0.st/XPN_.txt Adding a debug print I see that the module that the code tries to load is "crypto-hmac(sha512)". I would have expected to see MODULE_ALIAS_CRYPTO("hmac(sha512)"); in crypto/drbg.c, but I don't see it anywhere in the tree. Maybe it is missing? Thanks, Nícolas
On Fri May 17, 2024 at 7:22 PM EEST, Nícolas F. R. A. Prado wrote: > On Fri, May 17, 2024 at 07:25:40AM -0700, James Bottomley wrote: > > On Fri, 2024-05-17 at 15:43 +0200, Ard Biesheuvel wrote: > > > On Fri, 17 May 2024 at 15:35, James Bottomley > > > <James.Bottomley@hansenpartnership.com> wrote: > > [...] > > > > Thanks for the analysis. If I look at how CRYPTO_ECC does it, that > > > > selects CRYPTO_RNG_DEFAULT which pulls in CRYPTO_DRBG, so the fix > > > > would be the attached. Does that look right to you Ard? > > > > > > No it doesn't - it's CRYPTO_RNG_DEFAULT not CRYTPO_RNG_DEFAULT :-) > > > > > > With that fixed, > > > > > > Acked-by: Ard Biesheuvel <ardb@kernel.org> > > > > Erm, oops, sorry about that; so attached is the update. > > > > James > > > > ---8>8>8><8<8<8--- > > > > From 2ac337a33e6416ef806e2c692b9239d193e8468f Mon Sep 17 00:00:00 2001 > > From: James Bottomley <James.Bottomley@HansenPartnership.com> > > Date: Fri, 17 May 2024 06:29:31 -0700 > > Subject: [PATCH] tpm: Fix sessions cryptography requirement for Random Numbers > > MIME-Version: 1.0 > > Content-Type: text/plain; charset=UTF-8 > > Content-Transfer-Encoding: 8bit > > > > The ECDH code in tpm2-sessions.c requires an initial random number > > generator to generate the key pair. If the configuration doesn't have > > CONFIG_RNG_DEFAULT, it will try to pull this in as a module (which is > > impossible for the early kernel boot where the TPM starts). Fix this > > by selecting the required RNG. > > > > Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > Fixes: 1b6d7f9eb150 ("tpm: add session encryption protection to tpm2_get_random()") > > Acked-by: Ard Biesheuvel <ardb@kernel.org> > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > --- > > drivers/char/tpm/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig > > index 4f83ee7021d0..ecdd3db4be2b 100644 > > --- a/drivers/char/tpm/Kconfig > > +++ b/drivers/char/tpm/Kconfig > > @@ -31,6 +31,7 @@ config TCG_TPM2_HMAC > > bool "Use HMAC and encrypted transactions on the TPM bus" > > default y > > select CRYPTO_ECDH > > + select CRYPTO_RNG_DEFAULT > > select CRYPTO_LIB_AESCFB > > select CRYPTO_LIB_SHA256 > > help > > -- > > 2.35.3 > > > > > > Hi James, > > thanks for the patch. But I actually already had that config enabled builtin. I > also had ECDH and DRBG which have been suggested previously: > > CONFIG_CRYPTO_RNG_DEFAULT=y > > CONFIG_CRYPTO_DRBG_MENU=y > CONFIG_CRYPTO_DRBG_HMAC=y > # CONFIG_CRYPTO_DRBG_HASH is not set > # CONFIG_CRYPTO_DRBG_CTR is not set > CONFIG_CRYPTO_DRBG=y > > CONFIG_CRYPTO_ECDH=y > > I've pasted my full config here: http://0x0.st/XPN_.txt > > Adding a debug print I see that the module that the code tries to load is > "crypto-hmac(sha512)". I would have expected to see > > MODULE_ALIAS_CRYPTO("hmac(sha512)"); > > in crypto/drbg.c, but I don't see it anywhere in the tree. Maybe it is missing? 1. Bug fixes need to be submitted as described in https://www.kernel.org/doc/html/latest/process/submitting-patches.html 2. The patch is missing the transcript: https://lore.kernel.org/linux-integrity/D1BRZ60B9O5S.3NAT20QPQE6KH@kernel.org/ There's nothing to review at this point. > > Thanks, > Nícolas BR, Jarkko
On Fri, May 17, 2024 at 07:48:48PM +0300, Jarkko Sakkinen wrote: > On Fri May 17, 2024 at 7:22 PM EEST, Nícolas F. R. A. Prado wrote: > > On Fri, May 17, 2024 at 07:25:40AM -0700, James Bottomley wrote: > > > On Fri, 2024-05-17 at 15:43 +0200, Ard Biesheuvel wrote: > > > > On Fri, 17 May 2024 at 15:35, James Bottomley > > > > <James.Bottomley@hansenpartnership.com> wrote: > > > [...] > > > > > Thanks for the analysis. If I look at how CRYPTO_ECC does it, that > > > > > selects CRYPTO_RNG_DEFAULT which pulls in CRYPTO_DRBG, so the fix > > > > > would be the attached. Does that look right to you Ard? > > > > > > > > No it doesn't - it's CRYPTO_RNG_DEFAULT not CRYTPO_RNG_DEFAULT :-) > > > > > > > > With that fixed, > > > > > > > > Acked-by: Ard Biesheuvel <ardb@kernel.org> > > > > > > Erm, oops, sorry about that; so attached is the update. > > > > > > James > > > > > > ---8>8>8><8<8<8--- > > > > > > From 2ac337a33e6416ef806e2c692b9239d193e8468f Mon Sep 17 00:00:00 2001 > > > From: James Bottomley <James.Bottomley@HansenPartnership.com> > > > Date: Fri, 17 May 2024 06:29:31 -0700 > > > Subject: [PATCH] tpm: Fix sessions cryptography requirement for Random Numbers > > > MIME-Version: 1.0 > > > Content-Type: text/plain; charset=UTF-8 > > > Content-Transfer-Encoding: 8bit > > > > > > The ECDH code in tpm2-sessions.c requires an initial random number > > > generator to generate the key pair. If the configuration doesn't have > > > CONFIG_RNG_DEFAULT, it will try to pull this in as a module (which is > > > impossible for the early kernel boot where the TPM starts). Fix this > > > by selecting the required RNG. > > > > > > Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > Fixes: 1b6d7f9eb150 ("tpm: add session encryption protection to tpm2_get_random()") > > > Acked-by: Ard Biesheuvel <ardb@kernel.org> > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > --- > > > drivers/char/tpm/Kconfig | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig > > > index 4f83ee7021d0..ecdd3db4be2b 100644 > > > --- a/drivers/char/tpm/Kconfig > > > +++ b/drivers/char/tpm/Kconfig > > > @@ -31,6 +31,7 @@ config TCG_TPM2_HMAC > > > bool "Use HMAC and encrypted transactions on the TPM bus" > > > default y > > > select CRYPTO_ECDH > > > + select CRYPTO_RNG_DEFAULT > > > select CRYPTO_LIB_AESCFB > > > select CRYPTO_LIB_SHA256 > > > help > > > -- > > > 2.35.3 > > > > > > > > > > Hi James, > > > > thanks for the patch. But I actually already had that config enabled builtin. I > > also had ECDH and DRBG which have been suggested previously: > > > > CONFIG_CRYPTO_RNG_DEFAULT=y > > > > CONFIG_CRYPTO_DRBG_MENU=y > > CONFIG_CRYPTO_DRBG_HMAC=y > > # CONFIG_CRYPTO_DRBG_HASH is not set > > # CONFIG_CRYPTO_DRBG_CTR is not set > > CONFIG_CRYPTO_DRBG=y > > > > CONFIG_CRYPTO_ECDH=y > > > > I've pasted my full config here: http://0x0.st/XPN_.txt > > > > Adding a debug print I see that the module that the code tries to load is > > "crypto-hmac(sha512)". I would have expected to see > > > > MODULE_ALIAS_CRYPTO("hmac(sha512)"); > > > > in crypto/drbg.c, but I don't see it anywhere in the tree. Maybe it is missing? > This is "normal" behavior when the crypto API instantiates a template: 1. drbg.c asks for "hmac(sha512)" 2. The crypto API looks for a direct implementation of "hmac(sha512)". This includes requesting a module with alias "crypto-hmac(sha512)". 3. If none is found, the "hmac" template is instantiated instead. There are two possible fixes for the bug. Either fix ecc_gen_privkey() to just use get_random_bytes() instead of the weird crypto API RNG, or make drbg_init_hash_kernel() pass the CRYPTO_NOLOAD flag to crypto_alloc_shash(). Or if the TPM driver could be changed to not need to generate an ECC private key at probe time, that would also avoid this problem. - Eric
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index a53a843294ed..0cdf892ec2a7 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -292,25 +292,35 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max) if (!num_bytes || max > TPM_MAX_RNG_DATA) return -EINVAL; - err = tpm_buf_init(&buf, 0, 0); + err = tpm2_start_auth_session(chip); if (err) return err; + err = tpm_buf_init(&buf, 0, 0); + if (err) { + tpm2_end_auth_session(chip); + return err; + } + do { - tpm_buf_reset(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_RANDOM); + tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM); + tpm_buf_append_hmac_session_opt(chip, &buf, TPM2_SA_ENCRYPT + | TPM2_SA_CONTINUE_SESSION, + NULL, 0); tpm_buf_append_u16(&buf, num_bytes); + tpm_buf_fill_hmac_session(chip, &buf); err = tpm_transmit_cmd(chip, &buf, offsetof(struct tpm2_get_random_out, buffer), "attempting get random"); + err = tpm_buf_check_hmac_response(chip, &buf, err); if (err) { if (err > 0) err = -EIO; goto out; } - out = (struct tpm2_get_random_out *) - &buf.data[TPM_HEADER_SIZE]; + out = (struct tpm2_get_random_out *)tpm_buf_parameters(&buf); recd = min_t(u32, be16_to_cpu(out->size), num_bytes); if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + @@ -327,9 +337,12 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max) } while (retries-- && total < max); tpm_buf_destroy(&buf); + tpm2_end_auth_session(chip); + return total ? total : -EIO; out: tpm_buf_destroy(&buf); + tpm2_end_auth_session(chip); return err; }