| Message ID | 1569594360-7141-4-git-send-email-nayna@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | powerpc: Enabling IMA arch specific secure boot policies | expand |
Hello, Nayna Jain <nayna@linux.ibm.com> writes: > PowerNV systems uses kernel based bootloader, thus its secure boot > implementation uses kernel IMA security subsystem to verify the kernel > before kexec. Since the verification policy might differ based on the > secure boot mode of the system, the policies are defined at runtime. > > This patch implements the arch-specific support to define the 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 | 2 ++ > arch/powerpc/kernel/Makefile | 2 +- > arch/powerpc/kernel/ima_arch.c | 33 +++++++++++++++++++++++++++++++++ > include/linux/ima.h | 3 ++- > 4 files changed, 38 insertions(+), 2 deletions(-) > create mode 100644 arch/powerpc/kernel/ima_arch.c > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index 2c54beb29f1a..54eda07c74e5 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -916,6 +916,8 @@ config PPC_SECURE_BOOT > prompt "Enable secure boot support" > bool > depends on PPC_POWERNV > + depends on IMA > + depends on IMA_ARCH_POLICY > help > Systems with firmware secure boot enabled needs to define security > policies to extend secure boot to the OS. This config allows user > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile > index 875b0785a20e..7156ac1fc956 100644 > --- a/arch/powerpc/kernel/Makefile > +++ b/arch/powerpc/kernel/Makefile > @@ -157,7 +157,7 @@ endif > obj-$(CONFIG_EPAPR_PARAVIRT) += epapr_paravirt.o epapr_hcalls.o > obj-$(CONFIG_KVM_GUEST) += kvm.o kvm_emul.o > > -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..39401b67f19e > --- /dev/null > +++ b/arch/powerpc/kernel/ima_arch.c > @@ -0,0 +1,33 @@ > +// 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_powerpc_os_secureboot_enabled(); > +} > + > +/* Defines IMA appraise rules for secureboot */ > +static const char *const arch_rules[] = { > + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", > +#if !IS_ENABLED(CONFIG_MODULE_SIG) > + "appraise func=MODULE_CHECK appraise_type=imasig|modsig", > +#endif > + NULL > +}; > + > +/* > + * Returns the relevant IMA arch policies based on the system secureboot state. > + */ > +const char *const *arch_get_ima_policy(void) > +{ > + if (is_powerpc_os_secureboot_enabled()) > + return arch_rules; > + > + return NULL; > +} If CONFIG_MODULE_SIG is enabled but module signatures aren't enforced, then IMA won't enforce module signature either. x86's arch_get_ima_policy() calls set_module_sig_enforced(). Doesn't the powerpc version need to do that as well? On the flip side, if module signatures are enforced by the module subsystem then IMA will verify the signature a second time since there's no sharing of signature verification results between the module subsystem and IMA (this was observed by Mimi). IMHO this is a minor issue, since module loading isn't a hot path and the duplicate work shouldn't impact anything. But it could be avoided by having a NULL entry in arch_rules, which arch_get_ima_policy() would dynamically update with the "appraise func=MODULE_CHECK" rule if is_module_sig_enforced() is true. -- Thiago Jung Bauermann IBM Linux Technology Center
On 09/30/2019 09:04 PM, Thiago Jung Bauermann wrote: > Hello, Hi, > >> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c >> new file mode 100644 >> index 000000000000..39401b67f19e >> --- /dev/null >> +++ b/arch/powerpc/kernel/ima_arch.c >> @@ -0,0 +1,33 @@ >> +// 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_powerpc_os_secureboot_enabled(); >> +} >> + >> +/* Defines IMA appraise rules for secureboot */ >> +static const char *const arch_rules[] = { >> + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", >> +#if !IS_ENABLED(CONFIG_MODULE_SIG) >> + "appraise func=MODULE_CHECK appraise_type=imasig|modsig", >> +#endif >> + NULL >> +}; >> + >> +/* >> + * Returns the relevant IMA arch policies based on the system secureboot state. >> + */ >> +const char *const *arch_get_ima_policy(void) >> +{ >> + if (is_powerpc_os_secureboot_enabled()) >> + return arch_rules; >> + >> + return NULL; >> +} > If CONFIG_MODULE_SIG is enabled but module signatures aren't enforced, > then IMA won't enforce module signature either. x86's > arch_get_ima_policy() calls set_module_sig_enforced(). Doesn't the > powerpc version need to do that as well? > > On the flip side, if module signatures are enforced by the module > subsystem then IMA will verify the signature a second time since there's > no sharing of signature verification results between the module > subsystem and IMA (this was observed by Mimi). > > IMHO this is a minor issue, since module loading isn't a hot path and > the duplicate work shouldn't impact anything. But it could be avoided by > having a NULL entry in arch_rules, which arch_get_ima_policy() would > dynamically update with the "appraise func=MODULE_CHECK" rule if > is_module_sig_enforced() is true. Thanks Thiago for reviewing. I am wondering that this will give two meanings for NULL. Can we do something like below, there are possibly two options ? 1. Set IMA_APPRAISED in the iint->flags if is_module_sig_enforced(). OR 2. Let ima_get_action() check for is_module_sig_enforced() when policy is appraise and func is MODULE_CHECK. Thanks & Regards, - Nayna
Hi Nayna, Nayna <nayna@linux.vnet.ibm.com> writes: > On 09/30/2019 09:04 PM, Thiago Jung Bauermann wrote: >>> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c >>> new file mode 100644 >>> index 000000000000..39401b67f19e >>> --- /dev/null >>> +++ b/arch/powerpc/kernel/ima_arch.c >>> @@ -0,0 +1,33 @@ >>> +// 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_powerpc_os_secureboot_enabled(); >>> +} >>> + >>> +/* Defines IMA appraise rules for secureboot */ >>> +static const char *const arch_rules[] = { >>> + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", >>> +#if !IS_ENABLED(CONFIG_MODULE_SIG) >>> + "appraise func=MODULE_CHECK appraise_type=imasig|modsig", >>> +#endif >>> + NULL >>> +}; >>> + >>> +/* >>> + * Returns the relevant IMA arch policies based on the system secureboot state. >>> + */ >>> +const char *const *arch_get_ima_policy(void) >>> +{ >>> + if (is_powerpc_os_secureboot_enabled()) >>> + return arch_rules; >>> + >>> + return NULL; >>> +} >> If CONFIG_MODULE_SIG is enabled but module signatures aren't enforced, >> then IMA won't enforce module signature either. x86's >> arch_get_ima_policy() calls set_module_sig_enforced(). Doesn't the >> powerpc version need to do that as well? >> >> On the flip side, if module signatures are enforced by the module >> subsystem then IMA will verify the signature a second time since there's >> no sharing of signature verification results between the module >> subsystem and IMA (this was observed by Mimi). >> >> IMHO this is a minor issue, since module loading isn't a hot path and >> the duplicate work shouldn't impact anything. But it could be avoided by >> having a NULL entry in arch_rules, which arch_get_ima_policy() would >> dynamically update with the "appraise func=MODULE_CHECK" rule if >> is_module_sig_enforced() is true. > > Thanks Thiago for reviewing. I am wondering that this will give two meanings > for NULL. What are the two meanings? My understanding is that it only means "end of array". The additional NULL just allows arch_get_ima_policy() to dynamically append one item to the array. But I hadn't thought of your other alternatives. They should work just as well. Among those, I think option 1 is cleaner. This addresses the second issue I mentioned, but not the first. Also, one other thing I just noticed is that x86's arch policy has measure rules but powerpc's policy doesn't. What is different in our case? > Can we do something like below, there are possibly two options ? > > 1. Set IMA_APPRAISED in the iint->flags if is_module_sig_enforced(). > > OR > > 2. Let ima_get_action() check for is_module_sig_enforced() when policy is > appraise and func is MODULE_CHECK. > > Thanks & Regards, > - Nayna -- Thiago Jung Bauermann IBM Linux Technology Center
On Tue, 2019-10-01 at 12:07 -0400, Nayna wrote: > > On 09/30/2019 09:04 PM, Thiago Jung Bauermann wrote: > > Hello, > > Hi, > > > > >> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c > >> new file mode 100644 > >> index 000000000000..39401b67f19e > >> --- /dev/null > >> +++ b/arch/powerpc/kernel/ima_arch.c > >> @@ -0,0 +1,33 @@ > >> +// 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_powerpc_os_secureboot_enabled(); > >> +} > >> + > >> +/* Defines IMA appraise rules for secureboot */ > >> +static const char *const arch_rules[] = { > >> + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", > >> +#if !IS_ENABLED(CONFIG_MODULE_SIG) > >> + "appraise func=MODULE_CHECK appraise_type=imasig|modsig", > >> +#endif > >> + NULL > >> +}; > >> + > >> +/* > >> + * Returns the relevant IMA arch policies based on the system secureboot state. > >> + */ > >> +const char *const *arch_get_ima_policy(void) > >> +{ > >> + if (is_powerpc_os_secureboot_enabled()) > >> + return arch_rules; > >> + > >> + return NULL; > >> +} > > If CONFIG_MODULE_SIG is enabled but module signatures aren't enforced, > > then IMA won't enforce module signature either. x86's > > arch_get_ima_policy() calls set_module_sig_enforced(). Doesn't the > > powerpc version need to do that as well? > > > > On the flip side, if module signatures are enforced by the module > > subsystem then IMA will verify the signature a second time since there's > > no sharing of signature verification results between the module > > subsystem and IMA (this was observed by Mimi). > > > > IMHO this is a minor issue, since module loading isn't a hot path and > > the duplicate work shouldn't impact anything. But it could be avoided by > > having a NULL entry in arch_rules, which arch_get_ima_policy() would > > dynamically update with the "appraise func=MODULE_CHECK" rule if > > is_module_sig_enforced() is true. > > Thanks Thiago for reviewing. I am wondering that this will give two > meanings for NULL. Can we do something like below, there are possibly > two options ? > > 1. Set IMA_APPRAISED in the iint->flags if is_module_sig_enforced(). > > OR > > 2. Let ima_get_action() check for is_module_sig_enforced() when policy > is appraise and func is MODULE_CHECK. I'm a bit hesitant about mixing the module subsystem signature verification method with the IMA measure "template=ima-modsig" rules. Does it actually work? We can at least limit verifying the same appended signature twice to when "module.sig_enforce" is specified on the boot command line, by changing "!IS_ENABLED(CONFIG_MODULE_SIG)" to test "CONFIG_MODULE_SIG_FORCE". Mimi
On 10/02/2019 05:49 PM, Mimi Zohar wrote: > On Tue, 2019-10-01 at 12:07 -0400, Nayna wrote: >> On 09/30/2019 09:04 PM, Thiago Jung Bauermann wrote: >>> Hello, >> Hi, >> >>>> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c >>>> new file mode 100644 >>>> index 000000000000..39401b67f19e >>>> --- /dev/null >>>> +++ b/arch/powerpc/kernel/ima_arch.c >>>> @@ -0,0 +1,33 @@ >>>> +// 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_powerpc_os_secureboot_enabled(); >>>> +} >>>> + >>>> +/* Defines IMA appraise rules for secureboot */ >>>> +static const char *const arch_rules[] = { >>>> + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", >>>> +#if !IS_ENABLED(CONFIG_MODULE_SIG) >>>> + "appraise func=MODULE_CHECK appraise_type=imasig|modsig", >>>> +#endif >>>> + NULL >>>> +}; >>>> + >>>> +/* >>>> + * Returns the relevant IMA arch policies based on the system secureboot state. >>>> + */ >>>> +const char *const *arch_get_ima_policy(void) >>>> +{ >>>> + if (is_powerpc_os_secureboot_enabled()) >>>> + return arch_rules; >>>> + >>>> + return NULL; >>>> +} >>> If CONFIG_MODULE_SIG is enabled but module signatures aren't enforced, >>> then IMA won't enforce module signature either. x86's >>> arch_get_ima_policy() calls set_module_sig_enforced(). Doesn't the >>> powerpc version need to do that as well? >>> >>> On the flip side, if module signatures are enforced by the module >>> subsystem then IMA will verify the signature a second time since there's >>> no sharing of signature verification results between the module >>> subsystem and IMA (this was observed by Mimi). >>> >>> IMHO this is a minor issue, since module loading isn't a hot path and >>> the duplicate work shouldn't impact anything. But it could be avoided by >>> having a NULL entry in arch_rules, which arch_get_ima_policy() would >>> dynamically update with the "appraise func=MODULE_CHECK" rule if >>> is_module_sig_enforced() is true. >> Thanks Thiago for reviewing. I am wondering that this will give two >> meanings for NULL. Can we do something like below, there are possibly >> two options ? >> >> 1. Set IMA_APPRAISED in the iint->flags if is_module_sig_enforced(). >> >> OR >> >> 2. Let ima_get_action() check for is_module_sig_enforced() when policy >> is appraise and func is MODULE_CHECK. > I'm a bit hesitant about mixing the module subsystem signature > verification method with the IMA measure "template=ima-modsig" rules. > Does it actually work? > > We can at least limit verifying the same appended signature twice to > when "module.sig_enforce" is specified on the boot command line, by > changing "!IS_ENABLED(CONFIG_MODULE_SIG)" to test > "CONFIG_MODULE_SIG_FORCE". Yes this seems to be a better idea. I have implemented this in the v7 version of the ima_arch version. Thanks & Regards, - Nayna
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 2c54beb29f1a..54eda07c74e5 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -916,6 +916,8 @@ config PPC_SECURE_BOOT prompt "Enable secure boot support" bool depends on PPC_POWERNV + depends on IMA + depends on IMA_ARCH_POLICY help Systems with firmware secure boot enabled needs to define security policies to extend secure boot to the OS. This config allows user diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile index 875b0785a20e..7156ac1fc956 100644 --- a/arch/powerpc/kernel/Makefile +++ b/arch/powerpc/kernel/Makefile @@ -157,7 +157,7 @@ endif obj-$(CONFIG_EPAPR_PARAVIRT) += epapr_paravirt.o epapr_hcalls.o obj-$(CONFIG_KVM_GUEST) += kvm.o kvm_emul.o -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..39401b67f19e --- /dev/null +++ b/arch/powerpc/kernel/ima_arch.c @@ -0,0 +1,33 @@ +// 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_powerpc_os_secureboot_enabled(); +} + +/* Defines IMA appraise rules for secureboot */ +static const char *const arch_rules[] = { + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig", +#if !IS_ENABLED(CONFIG_MODULE_SIG) + "appraise func=MODULE_CHECK appraise_type=imasig|modsig", +#endif + NULL +}; + +/* + * Returns the relevant IMA arch policies based on the system secureboot state. + */ +const char *const *arch_get_ima_policy(void) +{ + if (is_powerpc_os_secureboot_enabled()) + return arch_rules; + + return NULL; +} diff --git a/include/linux/ima.h b/include/linux/ima.h index a20ad398d260..10af09b5b478 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 systems uses kernel based bootloader, thus its secure boot implementation uses kernel IMA security subsystem to verify the kernel before kexec. Since the verification policy might differ based on the secure boot mode of the system, the policies are defined at runtime. This patch implements the arch-specific support to define the 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 | 2 ++ arch/powerpc/kernel/Makefile | 2 +- arch/powerpc/kernel/ima_arch.c | 33 +++++++++++++++++++++++++++++++++ include/linux/ima.h | 3 ++- 4 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 arch/powerpc/kernel/ima_arch.c