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
On Sat May 18, 2024 at 7:31 AM EEST, Eric Biggers wrote: > 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. Issues: - IMA extends PCR's. This requires encrypted communications path. - HWRNG uses auth session (see tpm2_get_radom()). - TPM trusted keys Null key is required before any other legit use in initialization. Even something like --- a/drivers/char/tpm/Kconfig +++ b/drivers/char/tpm/Kconfig @@ -36,6 +36,8 @@ config TCG_TPM2_HMAC bool "Use HMAC and encrypted transactions on the TPM bus" default y + select CRYPTO_DRBG select CRYPTO_ECDH + select CRYPTO_HMAC + select CRYPTO_SHA512 select CRYPTO_LIB_AESCFB select CRYPTO_LIB_SHA256 help would be more decent. > > - Eric BR, Jarkko
On Sat, May 18, 2024 at 01:56:44PM +0300, Jarkko Sakkinen wrote: > > --- a/drivers/char/tpm/Kconfig > +++ b/drivers/char/tpm/Kconfig > @@ -36,6 +36,8 @@ config TCG_TPM2_HMAC > bool "Use HMAC and encrypted transactions on the TPM bus" > default y > + select CRYPTO_DRBG > select CRYPTO_ECDH > + select CRYPTO_HMAC > + select CRYPTO_SHA512 > select CRYPTO_LIB_AESCFB > select CRYPTO_LIB_SHA256 > help This isn't necessary because ECDH selects ECC which already selects the DRBG and all the required algorithms. You can verify this with the failing .config file as it has everything needed built into the kernel (which is in fact where the problem is). Cheers,
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; }