| Message ID | 20250323140911.226137-3-nstange@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: get rid of hard dependency on SHA-1 | expand |
On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote: > runtime_measurements_<hash-algo> sysfs files are getting created for > each PCR bank + for SHA-1. > > Now that runtime_measurements_<hash-algo> sysfs file creation is being > skipped for unsupported hash algorithms, it will become possible that no > such file would be provided at all once SHA-1 is made optional in a > later patch. > > Always create the file for the 'ima_hash' algorithm, even if it's not > associated with any of the PCR banks. As IMA initialization will > continue to fail if the ima_hash algorithm is not available to the > kernel, this guarantees that at least one such file will always be > there. > > Signed-off-by: Nicolai Stange <nstange@suse.de> > --- > security/integrity/ima/ima_fs.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c > index a8df2fe5f4cb..f030ff7f56da 100644 > --- a/security/integrity/ima/ima_fs.c > +++ b/security/integrity/ima/ima_fs.c > @@ -436,10 +436,8 @@ static int __init create_securityfs_measurement_lists(void) > u16 algo; > int i; > > - securityfs_measurement_list_count = NR_BANKS(ima_tpm_chip); > - > - if (ima_sha1_idx >= NR_BANKS(ima_tpm_chip)) > - securityfs_measurement_list_count++; > + securityfs_measurement_list_count = > + NR_BANKS(ima_tpm_chip) + ima_extra_slots; > > ascii_securityfs_measurement_lists = > kcalloc(securityfs_measurement_list_count, sizeof(struct dentry *), "ima_hash" is the default file hash algorithm. Re-using it as the default complete measurement list assumes that the subsequent kexec'ed kernels configure and define it as the default file hash algorithm as well, which might not be the case. Drop this patch. Defer allocating the "extra" non-sha1 bank. A subsequent patch will select SHA256. Based on the chosen algorithm, define the "extra" non-sha1 bank. thanks, Mimi
Mimi Zohar <zohar@linux.ibm.com> writes: > On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote: >> runtime_measurements_<hash-algo> sysfs files are getting created for >> each PCR bank + for SHA-1. >> >> Now that runtime_measurements_<hash-algo> sysfs file creation is being >> skipped for unsupported hash algorithms, it will become possible that no >> such file would be provided at all once SHA-1 is made optional in a >> later patch. >> >> Always create the file for the 'ima_hash' algorithm, even if it's not >> associated with any of the PCR banks. As IMA initialization will >> continue to fail if the ima_hash algorithm is not available to the >> kernel, this guarantees that at least one such file will always be >> there. >> >> Signed-off-by: Nicolai Stange <nstange@suse.de> >> --- >> security/integrity/ima/ima_fs.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c >> index a8df2fe5f4cb..f030ff7f56da 100644 >> --- a/security/integrity/ima/ima_fs.c >> +++ b/security/integrity/ima/ima_fs.c >> @@ -436,10 +436,8 @@ static int __init create_securityfs_measurement_lists(void) >> u16 algo; >> int i; >> >> - securityfs_measurement_list_count = NR_BANKS(ima_tpm_chip); >> - >> - if (ima_sha1_idx >= NR_BANKS(ima_tpm_chip)) >> - securityfs_measurement_list_count++; >> + securityfs_measurement_list_count = >> + NR_BANKS(ima_tpm_chip) + ima_extra_slots; >> >> ascii_securityfs_measurement_lists = >> kcalloc(securityfs_measurement_list_count, sizeof(struct dentry *), > > "ima_hash" is the default file hash algorithm. Re-using it as the default > complete measurement list assumes that the subsequent kexec'ed kernels configure > and define it as the default file hash algorithm as well, which might not be the > case. I don't really see why the ima_hashes would have to match between kexecs for this to work -- all events' template hashes are getting recreated from scratch anyway after kexec (ima_restore_measurement_list() -> ima_calc_field_array_hash()). That is, if ima_hash=sha256 first, and ima_hash=sha384 after kexec, one would have *runtime_measurements_sha256 first and *runtime_measurements_sha384 after kexec. And both had exclusively template hashes of their respective algo in them each. What am I missing? > Drop this patch. Fine by me, but just to confirm, in case there's no TPM attached and SHA1 was disabled, there would be no /sys/*/*runtime_measurement* at all then. Is that Ok? ima_hash was chosen here only, because after this series, it will be the only single algorithm guaranteed to be available. Thanks! Nicolai > Defer allocating the "extra" non-sha1 bank. A subsequent patch will select > SHA256. Based on the chosen algorithm, define the "extra" non-sha1 bank. >
On Wed, 2025-03-26 at 09:21 +0100, Nicolai Stange wrote: > Mimi Zohar <zohar@linux.ibm.com> writes: > > > On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote: > > > runtime_measurements_<hash-algo> sysfs files are getting created for > > > each PCR bank + for SHA-1. > > > > > > Now that runtime_measurements_<hash-algo> sysfs file creation is being > > > skipped for unsupported hash algorithms, it will become possible that no > > > such file would be provided at all once SHA-1 is made optional in a > > > later patch. > > > > > > Always create the file for the 'ima_hash' algorithm, even if it's not > > > associated with any of the PCR banks. As IMA initialization will > > > continue to fail if the ima_hash algorithm is not available to the > > > kernel, this guarantees that at least one such file will always be > > > there. > > > > > > Signed-off-by: Nicolai Stange <nstange@suse.de> > > > --- > > > security/integrity/ima/ima_fs.c | 6 ++---- > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c > > > index a8df2fe5f4cb..f030ff7f56da 100644 > > > --- a/security/integrity/ima/ima_fs.c > > > +++ b/security/integrity/ima/ima_fs.c > > > @@ -436,10 +436,8 @@ static int __init create_securityfs_measurement_lists(void) > > > u16 algo; > > > int i; > > > > > > - securityfs_measurement_list_count = NR_BANKS(ima_tpm_chip); > > > - > > > - if (ima_sha1_idx >= NR_BANKS(ima_tpm_chip)) > > > - securityfs_measurement_list_count++; > > > + securityfs_measurement_list_count = > > > + NR_BANKS(ima_tpm_chip) + ima_extra_slots; > > > > > > ascii_securityfs_measurement_lists = > > > kcalloc(securityfs_measurement_list_count, sizeof(struct dentry *), > > > > "ima_hash" is the default file hash algorithm. Re-using it as the default > > complete measurement list assumes that the subsequent kexec'ed kernels configure > > and define it as the default file hash algorithm as well, which might not be the > > case. > > I don't really see why the ima_hashes would have to match between kexecs > for this to work -- all events' template hashes are getting recreated > from scratch anyway after kexec (ima_restore_measurement_list() -> > ima_calc_field_array_hash()). > > That is, if ima_hash=sha256 first, and ima_hash=sha384 after kexec, one > would have *runtime_measurements_sha256 first and > *runtime_measurements_sha384 after kexec. And both had exclusively > template hashes of their respective algo in them each. > > What am I missing? Your solution would work nicely, if the "ima_hash" algorithm could be guaranteed to be built into the kernel. It's highly unlikely someone would choose a hash algorithm not built into kernel, but it is possible. hash_setup() only verifies that the hash algorithm is a valid name. Either fix hash_setup() to guarantee that the chosen hash algorithm is built into the kernel or use the CONFIG_IMA_DEFAULT_HASH and add a Kconfig to select the hash algorithm. This would be in lieu of v2 05/13. > > Drop this patch. > > Fine by me, but just to confirm, in case there's no TPM attached and > SHA1 was disabled, there would be no /sys/*/*runtime_measurement* at all > then. Is that Ok? Of course not. :) > ima_hash was chosen here only, because after this series, it will be the > only single algorithm guaranteed to be available. With the proposed changes to "[RFC PATCH v2 05/13] ima: select CRYPTO_SHA256 from Kconfig', SHA256 would be added to the "extra" measurements if the TPM SHA256 bank is disabled. Mimi
Mimi Zohar <zohar@linux.ibm.com> writes: > On Wed, 2025-03-26 at 09:21 +0100, Nicolai Stange wrote: >> Mimi Zohar <zohar@linux.ibm.com> writes: >> >> > On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote: >> > "ima_hash" is the default file hash algorithm. Re-using it as the default >> > complete measurement list assumes that the subsequent kexec'ed kernels configure >> > and define it as the default file hash algorithm as well, which might not be the >> > case. >> >> I don't really see why the ima_hashes would have to match between kexecs >> for this to work -- all events' template hashes are getting recreated >> from scratch anyway after kexec (ima_restore_measurement_list() -> >> ima_calc_field_array_hash()). >> >> That is, if ima_hash=sha256 first, and ima_hash=sha384 after kexec, one >> would have *runtime_measurements_sha256 first and >> *runtime_measurements_sha384 after kexec. And both had exclusively >> template hashes of their respective algo in them each. >> >> What am I missing? > > Your solution would work nicely, if the "ima_hash" algorithm could be guaranteed > to be built into the kernel. It's highly unlikely someone would choose a hash > algorithm not built into kernel, but it is possible. hash_setup() only verifies > that the hash algorithm is a valid name. But ima_init_ima_crypto(), hence the whole IMA __init, would fail if ima_hash was unavailable at __init time? Thanks, Nicolai > Either fix hash_setup() to guarantee that the chosen hash algorithm is built > into the kernel or use the CONFIG_IMA_DEFAULT_HASH and add a Kconfig to select > the hash algorithm. This would be in lieu of v2 05/13. > >> > Drop this patch. >> >> Fine by me, but just to confirm, in case there's no TPM attached and >> SHA1 was disabled, there would be no /sys/*/*runtime_measurement* at all >> then. Is that Ok? > > Of course not. :) > >> ima_hash was chosen here only, because after this series, it will be the >> only single algorithm guaranteed to be available. > > With the proposed changes to "[RFC PATCH v2 05/13] ima: select CRYPTO_SHA256 > from Kconfig', SHA256 would be added to the "extra" measurements if the TPM > SHA256 bank is disabled.
On Wed, 2025-03-26 at 14:46 +0100, Nicolai Stange wrote: > Mimi Zohar <zohar@linux.ibm.com> writes: > > > On Wed, 2025-03-26 at 09:21 +0100, Nicolai Stange wrote: > > > Mimi Zohar <zohar@linux.ibm.com> writes: > > > > > > > On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote: > > > > > "ima_hash" is the default file hash algorithm. Re-using it as the default > > > > complete measurement list assumes that the subsequent kexec'ed kernels configure > > > > and define it as the default file hash algorithm as well, which might not be the > > > > case. > > > > > > I don't really see why the ima_hashes would have to match between kexecs > > > for this to work -- all events' template hashes are getting recreated > > > from scratch anyway after kexec (ima_restore_measurement_list() -> > > > ima_calc_field_array_hash()). > > > > > > That is, if ima_hash=sha256 first, and ima_hash=sha384 after kexec, one > > > would have *runtime_measurements_sha256 first and > > > *runtime_measurements_sha384 after kexec. And both had exclusively > > > template hashes of their respective algo in them each. > > > > > > What am I missing? > > > > Your solution would work nicely, if the "ima_hash" algorithm could be guaranteed > > to be built into the kernel. It's highly unlikely someone would choose a hash > > algorithm not built into kernel, but it is possible. hash_setup() only verifies > > that the hash algorithm is a valid name. > > But ima_init_ima_crypto(), hence the whole IMA __init, would fail if > ima_hash was unavailable at __init time? Thanks for pointing that out! Now I understand why just selecting SHA256 is sufficient. Mimi > > > Either fix hash_setup() to guarantee that the chosen hash algorithm is built > > into the kernel or use the CONFIG_IMA_DEFAULT_HASH and add a Kconfig to select > > the hash algorithm. This would be in lieu of v2 05/13. > > > > > > Drop this patch. > > > > > > Fine by me, but just to confirm, in case there's no TPM attached and > > > SHA1 was disabled, there would be no /sys/*/*runtime_measurement* at all > > > then. Is that Ok? > > > > Of course not. :) > > > > > ima_hash was chosen here only, because after this series, it will be the > > > only single algorithm guaranteed to be available. > > > > With the proposed changes to "[RFC PATCH v2 05/13] ima: select CRYPTO_SHA256 > > from Kconfig', SHA256 would be added to the "extra" measurements if the TPM > > SHA256 bank is disabled. > >
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index a8df2fe5f4cb..f030ff7f56da 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -436,10 +436,8 @@ static int __init create_securityfs_measurement_lists(void) u16 algo; int i; - securityfs_measurement_list_count = NR_BANKS(ima_tpm_chip); - - if (ima_sha1_idx >= NR_BANKS(ima_tpm_chip)) - securityfs_measurement_list_count++; + securityfs_measurement_list_count = + NR_BANKS(ima_tpm_chip) + ima_extra_slots; ascii_securityfs_measurement_lists = kcalloc(securityfs_measurement_list_count, sizeof(struct dentry *),
runtime_measurements_<hash-algo> sysfs files are getting created for each PCR bank + for SHA-1. Now that runtime_measurements_<hash-algo> sysfs file creation is being skipped for unsupported hash algorithms, it will become possible that no such file would be provided at all once SHA-1 is made optional in a later patch. Always create the file for the 'ima_hash' algorithm, even if it's not associated with any of the PCR banks. As IMA initialization will continue to fail if the ima_hash algorithm is not available to the kernel, this guarantees that at least one such file will always be there. Signed-off-by: Nicolai Stange <nstange@suse.de> --- security/integrity/ima/ima_fs.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)