| Message ID | 20191024034717.70552-3-nayna@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | powerpc: Enabling IMA arch specific secure boot policies | expand |
On 10/23/2019 8:47 PM, Nayna Jain wrote: > +/* > + * The "secure_rules" are enabled only on "secureboot" enabled systems. > + * These rules verify the file signatures against known good values. > + * The "appraise_type=imasig|modsig" option allows the known good signature > + * to be stored as an xattr or as an appended signature. > + * > + * To avoid duplicate signature verification as much as possible, the IMA > + * policy rule for module appraisal is added only if CONFIG_MODULE_SIG_FORCE > + * is not enabled. > + */ > +static const char *const secure_rules[] = { > + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", > +#ifndef CONFIG_MODULE_SIG_FORCE > + "appraise func=MODULE_CHECK appraise_type=imasig|modsig", > +#endif > + NULL > +}; Is there any way to not use conditional compilation in the above array definition? Maybe define different functions to get "secure_rules" for when CONFIG_MODULE_SIG_FORCE is defined and when it is not defined. Just a suggestion. -lakshmi
On 10/24/19 12:35 PM, Lakshmi Ramasubramanian wrote: > On 10/23/2019 8:47 PM, Nayna Jain wrote: > >> +/* >> + * The "secure_rules" are enabled only on "secureboot" enabled systems. >> + * These rules verify the file signatures against known good values. >> + * The "appraise_type=imasig|modsig" option allows the known good >> signature >> + * to be stored as an xattr or as an appended signature. >> + * >> + * To avoid duplicate signature verification as much as possible, >> the IMA >> + * policy rule for module appraisal is added only if >> CONFIG_MODULE_SIG_FORCE >> + * is not enabled. >> + */ >> +static const char *const secure_rules[] = { >> + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", >> +#ifndef CONFIG_MODULE_SIG_FORCE >> + "appraise func=MODULE_CHECK appraise_type=imasig|modsig", >> +#endif >> + NULL >> +}; > > Is there any way to not use conditional compilation in the above array > definition? Maybe define different functions to get "secure_rules" for > when CONFIG_MODULE_SIG_FORCE is defined and when it is not defined. How will you decide which function to be called ? Thanks & Regards, - Nayna
On 10/25/2019 10:02 AM, Nayna Jain wrote: >> Is there any way to not use conditional compilation in >> the above array definition? Maybe define different functions to get >> "secure_rules" for when CONFIG_MODULE_SIG_FORCE is defined and when >> it is not defined. > > How will you decide which function to be called ? Define the array in the C file: const char *const secure_rules_kernel_check[] = { "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", NULL }; const char *const secure_rules_kernel_module_check[] = { "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", "appraise func=MODULE_CHECK appraise_type=imasig|modsig", NULL }; And, in the header file : extern const char *const secure_rules_kernel_check; extern const char *const secure_rules_kernel_module_check; #ifdef CONFIG_MODULE_SIG_FORCE const char *secure_rules() { return secure_rules_kernel_check; } #else const char *secure_rules() { return secure_rules_kernel_module_check;} #endif // #ifdef CONFIG_MODULE_SIG_FORCE If you want to avoid duplication, secure_rules_kernel_check and secure_rules_kernel_module_check could be defined in separate C files and conditionally compiled (in Makefile). I was just trying to suggest the guidelines given in "Section 21) Conditional Compilation" in coding-style.rst. It says: Whenever possible don't use preprocessor conditionals (#ifdef, #if) in .c files;... Feel free to do what you think is appropriate. thanks, -lakshmi
On Fri, 2019-10-25 at 12:02 -0500, Nayna Jain wrote: > On 10/24/19 12:35 PM, Lakshmi Ramasubramanian wrote: > > On 10/23/2019 8:47 PM, Nayna Jain wrote: > > > >> +/* > >> + * The "secure_rules" are enabled only on "secureboot" enabled systems. > >> + * These rules verify the file signatures against known good values. > >> + * The "appraise_type=imasig|modsig" option allows the known good > >> signature > >> + * to be stored as an xattr or as an appended signature. > >> + * > >> + * To avoid duplicate signature verification as much as possible, > >> the IMA > >> + * policy rule for module appraisal is added only if > >> CONFIG_MODULE_SIG_FORCE > >> + * is not enabled. > >> + */ > >> +static const char *const secure_rules[] = { > >> + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", > >> +#ifndef CONFIG_MODULE_SIG_FORCE > >> + "appraise func=MODULE_CHECK appraise_type=imasig|modsig", > >> +#endif > >> + NULL > >> +}; > > > > Is there any way to not use conditional compilation in the above array > > definition? Maybe define different functions to get "secure_rules" for > > when CONFIG_MODULE_SIG_FORCE is defined and when it is not defined. > > How will you decide which function to be called ? You could call "is_module_sig_enforced()". Mimi
On Sat, 2019-10-26 at 19:52 -0400, Mimi Zohar wrote: > On Fri, 2019-10-25 at 12:02 -0500, Nayna Jain wrote: > > On 10/24/19 12:35 PM, Lakshmi Ramasubramanian wrote: > > > On 10/23/2019 8:47 PM, Nayna Jain wrote: > > > > > >> +/* > > >> + * The "secure_rules" are enabled only on "secureboot" enabled systems. > > >> + * These rules verify the file signatures against known good values. > > >> + * The "appraise_type=imasig|modsig" option allows the known good > > >> signature > > >> + * to be stored as an xattr or as an appended signature. > > >> + * > > >> + * To avoid duplicate signature verification as much as possible, > > >> the IMA > > >> + * policy rule for module appraisal is added only if > > >> CONFIG_MODULE_SIG_FORCE > > >> + * is not enabled. > > >> + */ > > >> +static const char *const secure_rules[] = { > > >> + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", > > >> +#ifndef CONFIG_MODULE_SIG_FORCE > > >> + "appraise func=MODULE_CHECK appraise_type=imasig|modsig", > > >> +#endif > > >> + NULL > > >> +}; > > > > > > Is there any way to not use conditional compilation in the above array > > > definition? Maybe define different functions to get "secure_rules" for > > > when CONFIG_MODULE_SIG_FORCE is defined and when it is not defined. > > > > How will you decide which function to be called ? > > You could call "is_module_sig_enforced()". Calling is_module_sig_enforce() would prevent verifying the same kernel module appended signature twice, when CONFIG_MODULE_SIG is enabled, but not CONFIG_MODULE_SIG_FORCE. This comes at the expense of having to define additional policies. Unlike for the kernel image, there is no coordination between lockdown and IMA for kernel modules signature verification. I suggest deferring defining additional policies to when the lockdown/IMA coordination is addressed. Mimi
Hi Lakshmi, Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes: > On 10/25/2019 10:02 AM, Nayna Jain wrote: > > >> Is there any way to not use conditional compilation in > >> the above array definition? Maybe define different functions to get > >> "secure_rules" for when CONFIG_MODULE_SIG_FORCE is defined and when > >> it is not defined. > > > > How will you decide which function to be called ? > > Define the array in the C file: > > const char *const secure_rules_kernel_check[] = { > "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", > NULL > }; > > const char *const secure_rules_kernel_module_check[] = { > "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", > "appraise func=MODULE_CHECK appraise_type=imasig|modsig", > NULL > }; > > And, in the header file : But there's no reason for any of this to be in a header, it's all contained in one file. Moving things into a header purely to avoid a single #ifdef in a C file is a backward step. > extern const char *const secure_rules_kernel_check; > extern const char *const secure_rules_kernel_module_check; > > #ifdef CONFIG_MODULE_SIG_FORCE > const char *secure_rules() { return secure_rules_kernel_check; } > #else > const char *secure_rules() { return secure_rules_kernel_module_check;} > #endif // #ifdef CONFIG_MODULE_SIG_FORCE > > If you want to avoid duplication, secure_rules_kernel_check and > secure_rules_kernel_module_check could be defined in separate C files > and conditionally compiled (in Makefile). Again that's just lots of added complication for no real benefit. > I was just trying to suggest the guidelines given in > "Section 21) Conditional Compilation" in coding-style.rst. > > It says: > Whenever possible don't use preprocessor conditionals (#ifdef, #if) in > .c files;... The key phrase being "guideline" :) That suggestion is aimed at avoiding code with lots of ifdefs sprinkled through the body of functions. Code written in that way can be very hard to read because you have to mentally pre-process it first, and then read the C-level logic. See below for an example. Moving the pre-processing out of line into helpers means when you're reading the function you can just reason about the C control flow. The reference to ".c files" is really talking about moving logic that is #ifdef'ed into static inline helpers. Those typically go in headers, but they don't have to if there's no other reason for them to be in a header. So where the code is all in one C file it would be completely fine to have an #ifdef in the C file around a static inline helper. But in this case where the #ifdef is just in an array I think it's entirely fine to just keep the #ifdef. Its presence there doesn't complicate the logic in anyway. cheers This is a "good" (bad) example of what we're trying to avoid: static long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_info) { #ifdef CONFIG_HAVE_HW_BREAKPOINT int len = 0; struct thread_struct *thread = &(child->thread); struct perf_event *bp; struct perf_event_attr attr; #endif /* CONFIG_HAVE_HW_BREAKPOINT */ #ifndef CONFIG_PPC_ADV_DEBUG_REGS struct arch_hw_breakpoint brk; #endif if (bp_info->version != 1) return -ENOTSUPP; #ifdef CONFIG_PPC_ADV_DEBUG_REGS /* * Check for invalid flags and combinations */ if ((bp_info->trigger_type == 0) || (bp_info->trigger_type & ~(PPC_BREAKPOINT_TRIGGER_EXECUTE | PPC_BREAKPOINT_TRIGGER_RW)) || (bp_info->addr_mode & ~PPC_BREAKPOINT_MODE_MASK) || (bp_info->condition_mode & ~(PPC_BREAKPOINT_CONDITION_MODE | PPC_BREAKPOINT_CONDITION_BE_ALL))) return -EINVAL; #if CONFIG_PPC_ADV_DEBUG_DVCS == 0 if (bp_info->condition_mode != PPC_BREAKPOINT_CONDITION_NONE) return -EINVAL; #endif if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_EXECUTE) { if ((bp_info->trigger_type != PPC_BREAKPOINT_TRIGGER_EXECUTE) || (bp_info->condition_mode != PPC_BREAKPOINT_CONDITION_NONE)) return -EINVAL; return set_instruction_bp(child, bp_info); } if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_EXACT) return set_dac(child, bp_info); #ifdef CONFIG_PPC_ADV_DEBUG_DAC_RANGE return set_dac_range(child, bp_info); #else return -EINVAL; #endif #else /* !CONFIG_PPC_ADV_DEBUG_DVCS */ /* * We only support one data breakpoint */ if ((bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_RW) == 0 || (bp_info->trigger_type & ~PPC_BREAKPOINT_TRIGGER_RW) != 0 || bp_info->condition_mode != PPC_BREAKPOINT_CONDITION_NONE) return -EINVAL; if ((unsigned long)bp_info->addr >= TASK_SIZE) return -EIO; brk.address = bp_info->addr & ~7UL; brk.type = HW_BRK_TYPE_TRANSLATE; brk.len = 8; if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ) brk.type |= HW_BRK_TYPE_READ; if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE) brk.type |= HW_BRK_TYPE_WRITE; #ifdef CONFIG_HAVE_HW_BREAKPOINT /* * Check if the request is for 'range' breakpoints. We can * support it if range < 8 bytes. */ if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE) len = bp_info->addr2 - bp_info->addr; else if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_EXACT) len = 1; else return -EINVAL; bp = thread->ptrace_bps[0]; if (bp) return -ENOSPC; /* Create a new breakpoint request if one doesn't exist already */ hw_breakpoint_init(&attr); attr.bp_addr = (unsigned long)bp_info->addr & ~HW_BREAKPOINT_ALIGN; attr.bp_len = len; arch_bp_generic_fields(brk.type, &attr.bp_type); thread->ptrace_bps[0] = bp = register_user_hw_breakpoint(&attr, ptrace_triggered, NULL, child); if (IS_ERR(bp)) { thread->ptrace_bps[0] = NULL; return PTR_ERR(bp); } return 1; #endif /* CONFIG_HAVE_HW_BREAKPOINT */ if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT) return -EINVAL; if (child->thread.hw_brk.address) return -ENOSPC; if (!ppc_breakpoint_available()) return -ENODEV; child->thread.hw_brk = brk; return 1; #endif /* !CONFIG_PPC_ADV_DEBUG_DVCS */ }
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 56ea0019b616..c795039bdc73 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -938,6 +938,7 @@ config PPC_SECURE_BOOT prompt "Enable secure boot support" bool depends on PPC_POWERNV + depends on IMA_ARCH_POLICY help Systems with firmware secure boot enabled need to define security policies to extend secure boot to the OS. This config allows a user diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile index e2a54fa240ac..e8eb2955b7d5 100644 --- a/arch/powerpc/kernel/Makefile +++ b/arch/powerpc/kernel/Makefile @@ -161,7 +161,7 @@ ifneq ($(CONFIG_PPC_POWERNV)$(CONFIG_PPC_SVM),) obj-y += ucall.o endif -obj-$(CONFIG_PPC_SECURE_BOOT) += secure_boot.o +obj-$(CONFIG_PPC_SECURE_BOOT) += secure_boot.o ima_arch.o # Disable GCOV, KCOV & sanitizers in odd or sensitive code GCOV_PROFILE_prom_init.o := n diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c new file mode 100644 index 000000000000..d88913dc0da7 --- /dev/null +++ b/arch/powerpc/kernel/ima_arch.c @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2019 IBM Corporation + * Author: Nayna Jain + */ + +#include <linux/ima.h> +#include <asm/secure_boot.h> + +bool arch_ima_get_secureboot(void) +{ + return is_ppc_secureboot_enabled(); +} + +/* + * The "secure_rules" are enabled only on "secureboot" enabled systems. + * These rules verify the file signatures against known good values. + * The "appraise_type=imasig|modsig" option allows the known good signature + * to be stored as an xattr or as an appended signature. + * + * To avoid duplicate signature verification as much as possible, the IMA + * policy rule for module appraisal is added only if CONFIG_MODULE_SIG_FORCE + * is not enabled. + */ +static const char *const secure_rules[] = { + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", +#ifndef CONFIG_MODULE_SIG_FORCE + "appraise func=MODULE_CHECK appraise_type=imasig|modsig", +#endif + NULL +}; + +/* + * Returns the relevant IMA arch-specific policies based on the system secure + * boot state. + */ +const char *const *arch_get_ima_policy(void) +{ + if (is_ppc_secureboot_enabled()) + return secure_rules; + + return NULL; +} diff --git a/include/linux/ima.h b/include/linux/ima.h index 1c37f17f7203..6d904754d858 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -29,7 +29,8 @@ extern void ima_kexec_cmdline(const void *buf, int size); extern void ima_add_kexec_buffer(struct kimage *image); #endif -#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) +#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \ + || defined(CONFIG_PPC_SECURE_BOOT) extern bool arch_ima_get_secureboot(void); extern const char * const *arch_get_ima_policy(void); #else
PowerNV system use a Linux-based bootloader, which relies on the IMA subsystem to enforce different secure boot modes. Since the verification policy may differ based on the secure boot mode of the system, the policies must be defined at runtime. This patch implements arch-specific support to define IMA policy rules based on the runtime secure boot mode of the system. This patch provides arch-specific IMA policies if PPC_SECURE_BOOT config is enabled. Signed-off-by: Nayna Jain <nayna@linux.ibm.com> --- arch/powerpc/Kconfig | 1 + arch/powerpc/kernel/Makefile | 2 +- arch/powerpc/kernel/ima_arch.c | 43 ++++++++++++++++++++++++++++++++++ include/linux/ima.h | 3 ++- 4 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 arch/powerpc/kernel/ima_arch.c