| Message ID | 20240701133814.641662-1-romain.naour@smile.fr (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [RFC] integrity: wait for completion of i2c initialization using late_initcall_sync() | expand |
Dear Romain, Thank you for your patch. Am 01.07.24 um 15:38 schrieb Romain Naour: > From: Romain Naour <romain.naour@skf.com> > > It has been reported that on some plateforms the ima and evm platforms > initialization were performed too early during initcall initialization > process and misses TPM chip detection [1] [2]. > > Indeed, ima may uses a TPM chip but requires to wait for bus > interface (spi or i2c) and TPM driver initialization. > > [ 0.166261] ima: No TPM chip found, activating TPM-bypass! > ... > [ 0.166322] evm: Initialising EVM extended attributes: > ... > [ 0.182571] ti-sci 44083000.system-controller: ABI: 3.1 (firmware rev 0x0009 '9.2.4--v09.02.04 (Kool Koala)') > [ 0.281540] omap_i2c 42120000.i2c: bus 0 rev0.12 at 400 kHz > [ 0.282314] omap_i2c 2000000.i2c: bus 4 rev0.12 at 400 kHz > [ 0.282972] omap_i2c 2010000.i2c: bus 5 rev0.12 at 400 kHz > [ 0.335177] tpm_tis_i2c 2-002e: 2.0 TPM (device-id 0x1C, rev-id 22) > [ 0.471596] omap_i2c 2020000.i2c: bus 2 rev0.12 at 100 kHz > [ 0.472310] omap_i2c 2030000.i2c: bus 6 rev0.12 at 400 kHz > [ 0.472951] omap_i2c 2040000.i2c: bus 3 rev0.12 at 100 kHz > [ 0.473596] omap_i2c 2050000.i2c: bus 7 rev0.12 at 400 kHz > [ 0.474274] omap_i2c 2060000.i2c: bus 1 rev0.12 at 100 kHz > > The ima stack was expecting to start after the TPM device (hence the > comment) using late_initcall() but fail to do so on such plateforms: platforms > > late_initcall(init_ima); /* Start IMA after the TPM is available */ > > Using late_initcall_sync() variant allows to really wait for i2c > initialization completion. > > [ 0.285986] omap_i2c 42120000.i2c: bus 0 rev0.12 at 400 kHz > [ 0.286706] omap_i2c 2000000.i2c: bus 4 rev0.12 at 400 kHz > [ 0.287382] omap_i2c 2010000.i2c: bus 5 rev0.12 at 400 kHz > [ 0.331503] tpm_tis_i2c 2-002e: 2.0 TPM (device-id 0x1C, rev-id 22) > [ 0.677185] omap_i2c 2020000.i2c: bus 2 rev0.12 at 100 kHz > [ 0.677904] omap_i2c 2030000.i2c: bus 6 rev0.12 at 400 kHz > [ 0.678557] omap_i2c 2040000.i2c: bus 3 rev0.12 at 100 kHz > [ 0.679167] omap_i2c 2050000.i2c: bus 7 rev0.12 at 400 kHz > [ 0.679792] omap_i2c 2060000.i2c: bus 1 rev0.12 at 100 kHz > ... > [ 3.062788] ima: Allocated hash algorithm: sha256 > ... > [ 3.318975] ima: No architecture policies found > [ 3.323536] evm: Initialising EVM extended attributes: > [ 3.328662] evm: security.selinux (disabled) > [ 3.332919] evm: security.SMACK64 (disabled) > [ 3.337177] evm: security.SMACK64EXEC (disabled) > [ 3.341781] evm: security.SMACK64TRANSMUTE (disabled) > [ 3.346819] evm: security.SMACK64MMAP (disabled) > [ 3.351422] evm: security.apparmor (disabled) > [ 3.355764] evm: security.ima > [ 3.358721] evm: security.capability > [ 3.362285] evm: HMAC attrs: 0x1 > > [1] https://lore.kernel.org/linux-integrity/9b98d912-ba78-402c-a5c8-154bef8794f7@smile.fr/ > [2] https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1375425/tda4vm-ima-vs-tpm-builtin-driver-boot-order > > Signed-off-by: Romain Naour <romain.naour@skf.com> Should this get a Fixes: tag and be also applied to the stable series? > --- > security/integrity/evm/evm_main.c | 2 +- > security/integrity/ima/ima_main.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c > index 62fe66dd53ce..316f8d140825 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -1180,4 +1180,4 @@ DEFINE_LSM(evm) = { > .blobs = &evm_blob_sizes, > }; > > -late_initcall(init_evm); > +late_initcall_sync(init_evm); /* Start EVM after IMA */ > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index f04f43af651c..0aa7cd9aabfa 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -1220,4 +1220,4 @@ DEFINE_LSM(ima) = { > .blobs = &ima_blob_sizes, > }; > > -late_initcall(init_ima); /* Start IMA after the TPM is available */ > +late_initcall_sync(init_ima); /* Start IMA after the TPM is available */ Looks good to me. Kind regards, Paul
Hello Paul, Le 01/07/2024 à 15:53, Paul Menzel a écrit : > Dear Romain, > > > Thank you for your patch. > > Am 01.07.24 um 15:38 schrieb Romain Naour: >> From: Romain Naour <romain.naour@skf.com> >> >> It has been reported that on some plateforms the ima and evm > > platforms > >> initialization were performed too early during initcall initialization >> process and misses TPM chip detection [1] [2]. >> >> Indeed, ima may uses a TPM chip but requires to wait for bus >> interface (spi or i2c) and TPM driver initialization. >> >> [ 0.166261] ima: No TPM chip found, activating TPM-bypass! >> ... >> [ 0.166322] evm: Initialising EVM extended attributes: >> ... >> [ 0.182571] ti-sci 44083000.system-controller: ABI: 3.1 (firmware rev >> 0x0009 '9.2.4--v09.02.04 (Kool Koala)') >> [ 0.281540] omap_i2c 42120000.i2c: bus 0 rev0.12 at 400 kHz >> [ 0.282314] omap_i2c 2000000.i2c: bus 4 rev0.12 at 400 kHz >> [ 0.282972] omap_i2c 2010000.i2c: bus 5 rev0.12 at 400 kHz >> [ 0.335177] tpm_tis_i2c 2-002e: 2.0 TPM (device-id 0x1C, rev-id 22) >> [ 0.471596] omap_i2c 2020000.i2c: bus 2 rev0.12 at 100 kHz >> [ 0.472310] omap_i2c 2030000.i2c: bus 6 rev0.12 at 400 kHz >> [ 0.472951] omap_i2c 2040000.i2c: bus 3 rev0.12 at 100 kHz >> [ 0.473596] omap_i2c 2050000.i2c: bus 7 rev0.12 at 400 kHz >> [ 0.474274] omap_i2c 2060000.i2c: bus 1 rev0.12 at 100 kHz >> >> The ima stack was expecting to start after the TPM device (hence the >> comment) using late_initcall() but fail to do so on such plateforms: > > platforms I'll fix, thanks! > >> >> late_initcall(init_ima); /* Start IMA after the TPM is available */ >> >> Using late_initcall_sync() variant allows to really wait for i2c >> initialization completion. >> >> [ 0.285986] omap_i2c 42120000.i2c: bus 0 rev0.12 at 400 kHz >> [ 0.286706] omap_i2c 2000000.i2c: bus 4 rev0.12 at 400 kHz >> [ 0.287382] omap_i2c 2010000.i2c: bus 5 rev0.12 at 400 kHz >> [ 0.331503] tpm_tis_i2c 2-002e: 2.0 TPM (device-id 0x1C, rev-id 22) >> [ 0.677185] omap_i2c 2020000.i2c: bus 2 rev0.12 at 100 kHz >> [ 0.677904] omap_i2c 2030000.i2c: bus 6 rev0.12 at 400 kHz >> [ 0.678557] omap_i2c 2040000.i2c: bus 3 rev0.12 at 100 kHz >> [ 0.679167] omap_i2c 2050000.i2c: bus 7 rev0.12 at 400 kHz >> [ 0.679792] omap_i2c 2060000.i2c: bus 1 rev0.12 at 100 kHz >> ... >> [ 3.062788] ima: Allocated hash algorithm: sha256 >> ... >> [ 3.318975] ima: No architecture policies found >> [ 3.323536] evm: Initialising EVM extended attributes: >> [ 3.328662] evm: security.selinux (disabled) >> [ 3.332919] evm: security.SMACK64 (disabled) >> [ 3.337177] evm: security.SMACK64EXEC (disabled) >> [ 3.341781] evm: security.SMACK64TRANSMUTE (disabled) >> [ 3.346819] evm: security.SMACK64MMAP (disabled) >> [ 3.351422] evm: security.apparmor (disabled) >> [ 3.355764] evm: security.ima >> [ 3.358721] evm: security.capability >> [ 3.362285] evm: HMAC attrs: 0x1 >> >> [1] >> https://lore.kernel.org/linux-integrity/9b98d912-ba78-402c-a5c8-154bef8794f7@smile.fr/ >> [2] >> https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1375425/tda4vm-ima-vs-tpm-builtin-driver-boot-order >> >> Signed-off-by: Romain Naour <romain.naour@skf.com> > > Should this get a Fixes: tag and be also applied to the stable series? The current behavior can be reproduced on any released kernel (at least since 6.1). But I'm not sure if it should be backported to stable kernels since it delays the ima/evm initialization at runtime. > >> --- >> security/integrity/evm/evm_main.c | 2 +- >> security/integrity/ima/ima_main.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/security/integrity/evm/evm_main.c >> b/security/integrity/evm/evm_main.c >> index 62fe66dd53ce..316f8d140825 100644 >> --- a/security/integrity/evm/evm_main.c >> +++ b/security/integrity/evm/evm_main.c >> @@ -1180,4 +1180,4 @@ DEFINE_LSM(evm) = { >> .blobs = &evm_blob_sizes, >> }; >> -late_initcall(init_evm); >> +late_initcall_sync(init_evm); /* Start EVM after IMA */ >> diff --git a/security/integrity/ima/ima_main.c >> b/security/integrity/ima/ima_main.c >> index f04f43af651c..0aa7cd9aabfa 100644 >> --- a/security/integrity/ima/ima_main.c >> +++ b/security/integrity/ima/ima_main.c >> @@ -1220,4 +1220,4 @@ DEFINE_LSM(ima) = { >> .blobs = &ima_blob_sizes, >> }; >> -late_initcall(init_ima); /* Start IMA after the TPM is available */ >> +late_initcall_sync(init_ima); /* Start IMA after the TPM is available */ > > Looks good to me. Thanks for the review! Best regards, Romain > > > Kind regards, > > Paul
Hi Romain, Please limit the subject line to 70 - 75 characters. On Mon, 2024-07-01 at 16:58 +0200, Romain Naour wrote: > > > [1] > > > https://lore.kernel.org/linux-integrity/9b98d912-ba78-402c-a5c8-154bef8794f7@smile.fr/ > > > [2] > > > https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1375425/tda4vm-ima-vs-tpm-builtin-driver-boot-order > > > > > > Signed-off-by: Romain Naour <romain.naour@skf.com> > > > > Should this get a Fixes: tag and be also applied to the stable series? > > The current behavior can be reproduced on any released kernel (at least since > 6.1). But I'm not sure if it should be backported to stable kernels since it > delays the ima/evm initialization at runtime. With the IMA builtin measurement policy specified on the boot command line ("ima_policy=tcb"), moving init_ima from the late_initcall() to late_initcall_sync() affects the measurement list order. It's unlikely, but possible, that someone is sealing the TPM to PCR-10. It's probably not a good idea to backport the change. An alternative would be to continue using the late_initcall(), but retry on failure, instead of going directly into TPM-bypass mode. As far as I can tell, everything is still being measured and verified, but more testing is required. Mimi
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 62fe66dd53ce..316f8d140825 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -1180,4 +1180,4 @@ DEFINE_LSM(evm) = { .blobs = &evm_blob_sizes, }; -late_initcall(init_evm); +late_initcall_sync(init_evm); /* Start EVM after IMA */ diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index f04f43af651c..0aa7cd9aabfa 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -1220,4 +1220,4 @@ DEFINE_LSM(ima) = { .blobs = &ima_blob_sizes, }; -late_initcall(init_ima); /* Start IMA after the TPM is available */ +late_initcall_sync(init_ima); /* Start IMA after the TPM is available */