Messages in this thread | | | Subject | Re: [PATCH v7 4/8] powerpc/ima: add measurement rules to ima arch specific policy | From | Nayna <> | Date | Sat, 19 Oct 2019 14:27:07 -0400 |
| |
Hi Michael,
On 10/15/2019 07:29 AM, Michael Ellerman wrote: > Nayna Jain <nayna@linux.ibm.com> writes: >> This patch adds the measurement rules to the arch specific policies on >> trusted boot enabled systems. >> >> Signed-off-by: Nayna Jain <nayna@linux.ibm.com> >> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> >> --- >> arch/powerpc/kernel/ima_arch.c | 45 +++++++++++++++++++++++++++++++--- >> 1 file changed, 42 insertions(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c >> index c22d82965eb4..88bfe4a1a9a5 100644 >> --- a/arch/powerpc/kernel/ima_arch.c >> +++ b/arch/powerpc/kernel/ima_arch.c >> @@ -12,8 +12,19 @@ bool arch_ima_get_secureboot(void) >> return is_powerpc_os_secureboot_enabled(); >> } >> >> -/* Defines IMA appraise rules for secureboot */ >> +/* >> + * The "arch_rules" contains both the securebot and trustedboot rules for adding >> + * the kexec kernel image and kernel modules file hashes to the IMA measurement >> + * list and verifying the file signatures against known good values. >> + * >> + * The "appraise_type=imasig|modsig" option allows the good signature to be >> + * stored as an xattr or as an appended signature. The "template=ima-modsig" >> + * option includes the appended signature, when available, in the IMA >> + * measurement list. >> + */ >> static const char *const arch_rules[] = { >> + "measure func=KEXEC_KERNEL_CHECK template=ima-modsig", >> + "measure func=MODULE_CHECK template=ima-modsig", >> "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", >> #if !IS_ENABLED(CONFIG_MODULE_SIG_FORCE) >> "appraise func=MODULE_CHECK appraise_type=imasig|modsig", >> @@ -22,12 +33,40 @@ static const char *const arch_rules[] = { >> }; >> >> /* >> - * Returns the relevant IMA arch policies based on the system secureboot state. >> + * The "measure_rules" are enabled only on "trustedboot" enabled systems. >> + * These rules add the kexec kernel image and kernel modules file hashes to >> + * the IMA measurement list. >> + */ >> +static const char *const measure_rules[] = { >> + "measure func=KEXEC_KERNEL_CHECK", >> + "measure func=MODULE_CHECK", > Why do these ones not have "template=ima-modsig" on the end?
ima-modsig template is applicable only when IMA "collects" the appended signatures. IMA can then include it in the measurement list.
> >> + NULL >> +}; >> + >> +/* >> + * Returns the relevant IMA arch policies based on the system secureboot >> + * and trustedboot state. >> */ >> const char *const *arch_get_ima_policy(void) >> { >> - if (is_powerpc_os_secureboot_enabled()) >> + const char *const *rules; >> + int offset = 0; >> + >> + for (rules = arch_rules; *rules != NULL; rules++) { >> + if (strncmp(*rules, "appraise", 8) == 0) >> + break; >> + offset++; >> + } > This seems like kind of a hack, doesn't it? :) > > What we really want is three sets of rules isn't it? But some of them > are shared between the different sets. But they just have to be flat > arrays of strings. > > I think it would probably be cleaner to just use a #define for the > shared part of the rules, eg something like: > > #ifdef CONFIG_MODULE_SIG_FORCE > #define APPRAISE_MODULE > #else > #define APPRAISE_MODULE \ > "appraise func=MODULE_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig", > #endif > > #define APPRAISE_KERNEL \ > "appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig" > > #define MEASURE_KERNEL \ > "measure func=KEXEC_KERNEL_CHECK" > > #define MEASURE_MODULE \ > "measure func=MODULE_CHECK" > > #define APPEND_TEMPLATE_IMA_MODSIG \ > " template=ima-modsig" > > static const char *const secure_and_trusted_rules[] = { > MEASURE_KERNEL APPEND_TEMPLATE_IMA_MODSIG, > MEASURE_MODULE APPEND_TEMPLATE_IMA_MODSIG, > APPRAISE_KERNEL, > APPRAISE_MODULE > NULL > }; > > static const char *const secure_rules[] = { > APPRAISE_KERNEL, > APPRAISE_MODULE > NULL > }; > > static const char *const trusted_rules[] = { > MEASURE_KERNEL, > MEASURE_MODULE, > NULL > };
Yes, I agree it is sort of a hack to walk through the rules to find the start of the appraise policy. While trying your suggestion, I realized that defining three arrays, with some rule duplication, can fix the hack without #defines. This also improves readability of the rules. I have just now posted the new version with the changes. Please let me know if this looks ok.
Thanks & Regards, - Nayna
| |