lkml.org 
[lkml]   [2019]   [Oct]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v7 4/8] powerpc/ima: add measurement rules to ima arch specific policy
From
Date
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

\
 
 \ /
  Last update: 2019-10-19 20:28    [W:0.117 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site