diff mbox series

[v8,18/22] tpm: add session encryption protection to tpm2_get_random()

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

Commit Message

James Bottomley April 29, 2024, 8:28 p.m. UTC 
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(-)

Comments

Nícolas F. R. A. Prado May 17, 2024, 12:25 a.m. UTC | #1 
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
James Bottomley May 17, 2024, 1:59 a.m. UTC | #2 
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
Ard Biesheuvel May 17, 2024, 7:20 a.m. UTC | #3 
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.
Jarkko Sakkinen May 17, 2024, 8:26 a.m. UTC | #4 
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
James Bottomley May 17, 2024, 1:35 p.m. UTC | #5 
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
Ard Biesheuvel May 17, 2024, 1:43 p.m. UTC | #6 
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
>
>
James Bottomley May 17, 2024, 2:25 p.m. UTC | #7 
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
diff mbox series

Patch

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;
 }