| Message ID | 20240701133814.641662-1-romain.naour@smile.fr (mailing list archive) |
|---|---|
| State | New |
| 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
On Mon, 2024-07-01 at 22:37 -0400, Mimi Zohar wrote: > 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. Romain, Paul, another report of IMA going into TPM-bypass mode is here: https://github.com/raspberrypi/linux/issues/6217. Deferring IMA initialization to late_initcall_sync() did not resolve the problem for them. Please take a look at the report. thanks, Mimi
Hi Mimi, Le 11/07/2024 à 16:06, Mimi Zohar a écrit : > On Mon, 2024-07-01 at 22:37 -0400, Mimi Zohar wrote: >> 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. Indeed, it would be better if the IMA (and EVM) can use something like EPROBE_DEFER. >> >> As far as I can tell, everything is still being measured and verified, but more >> testing is required. Agree, I'm still evaluating the TPM stack when time allows. > > Romain, Paul, another report of IMA going into TPM-bypass mode is here: > https://github.com/raspberrypi/linux/issues/6217. Deferring IMA initialization > to late_initcall_sync() did not resolve the problem for them. Please take a > look at the report. I don't think that the "mdelay(200)" is really needed when late_initcall_sync() is used. I guess the issue was the spi driver was not builtin, fixed by: CONFIG_SPI_DESIGNWARE=y CONFIG_SPI_DW_MMIO=y Best regards, Romain > > thanks, > > Mimi >
On Thu, 2024-08-01 at 12:12 +0200, Romain Naour wrote: > Hi Mimi, > > Le 11/07/2024 à 16:06, Mimi Zohar a écrit : > > On Mon, 2024-07-01 at 22:37 -0400, Mimi Zohar wrote: > > > 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. > > Indeed, it would be better if the IMA (and EVM) can use something like EPROBE_DEFER. Yes, "something like EPROBE_DEFER" sounds like the right direction. Depending on the environment, the TPM initialization delay might be acceptable and not introduce an integrity gap. For now let's start with just late_initcall() and late_initcall_sync(). If the TPM hasn't been initialized, not all of ima_init() needs to be deferred to late_initcall_sync(). > > > > > > > As far as I can tell, everything is still being measured and verified, but more > > > testing is required. > > Agree, I'm still evaluating the TPM stack when time allows. > > > > > Romain, Paul, another report of IMA going into TPM-bypass mode is here: > > https://github.com/raspberrypi/linux/issues/6217. Deferring IMA initialization > > to late_initcall_sync() did not resolve the problem for them. Please take a > > look at the report. > > I don't think that the "mdelay(200)" is really needed when late_initcall_sync() > is used. I guess the issue was the spi driver was not builtin, fixed by: > > CONFIG_SPI_DESIGNWARE=y > CONFIG_SPI_DW_MMIO=y Good to know. thanks, Mimi
On Tue, 2024-08-06 at 20:41 -0400, Mimi Zohar wrote: > On Thu, 2024-08-01 at 12:12 +0200, Romain Naour wrote: > > Hi Mimi, > > > > Le 11/07/2024 à 16:06, Mimi Zohar a écrit : > > > On Mon, 2024-07-01 at 22:37 -0400, Mimi Zohar wrote: > > > > 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. > > > > Indeed, it would be better if the IMA (and EVM) can use something like EPROBE_DEFER. > > Yes, "something like EPROBE_DEFER" sounds like the right direction. Depending > on the environment, the TPM initialization delay might be acceptable and not > introduce an integrity gap. > > For now let's start with just late_initcall() and late_initcall_sync(). If the > TPM hasn't been initialized, not all of ima_init() needs to be deferred to > late_initcall_sync(). Just a personal opinion. I like the idea of initializing everything as soon as possible, even if there is no TPM. However, in the case where only the kernel image has been measured by the boot loader and one wants measurements with file granularity for the initial ram disk, deferring the TPM PCR extend I think it is not acceptable from the trusted computing point of view. One example would be loading a kernel module while the TPM is still not available, where the kernel module would be able to escape the subsequent measurement later when the TPM becomes available. In this particular case, if there is a measurement policy in place, I would deny any operation matching the policy, until the TPM is available (no defer). If there is really need for allowing something before the TPM is available, I would pass the digest of what it should be allowed by the kernel somehow (e.g. from the boot loader, kernel command line?). In this way, the allowed operations are measured before in the earlier stages of the boot. Roberto > > > > > > > > > > As far as I can tell, everything is still being measured and verified, but more > > > > testing is required. > > > > Agree, I'm still evaluating the TPM stack when time allows. > > > > > > > > Romain, Paul, another report of IMA going into TPM-bypass mode is here: > > > https://github.com/raspberrypi/linux/issues/6217. Deferring IMA initialization > > > to late_initcall_sync() did not resolve the problem for them. Please take a > > > look at the report. > > > > I don't think that the "mdelay(200)" is really needed when late_initcall_sync() > > is used. I guess the issue was the spi driver was not builtin, fixed by: > > > > CONFIG_SPI_DESIGNWARE=y > > CONFIG_SPI_DW_MMIO=y > > Good to know. > > thanks, > > 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 */