Re: [PATCH] ima: add a new CONFIG for loading arch-specific policies
On Wed, 2020-02-26 at 15:36 -0500, Mimi Zohar wrote: > On Wed, 2020-02-26 at 11:21 -0800, Lakshmi Ramasubramanian wrote: > > Hi Nayna, > > > > > + > > > +config IMA_SECURE_AND_OR_TRUSTED_BOOT > > > + bool > > > + depends on IMA > > > + depends on IMA_ARCH_POLICY > > > + default n > > > + help > > > +This option is selected by architectures to enable secure and/or > > > +trusted boot based on IMA runtime policies. > > > > > > > Why is the default for this new config "n"? > > Is there any reason to not turn on this config if both IMA and > > IMA_ARCH_POLICY are set to y? > > Good catch. Having "IMA_SECURE_AND_OR_TRUSTED_BOOT" depend on > "IMA_ARCH_POLICY" doesn't make sense. "IMA_ARCH_POLICY" needs to be > selected. After discussing this some more with Nayna, the new Kconfig indicates that the architecture defines the arch_ima_get_secureboot() and arch_get_ima_policy() functions, but doesn't automatically enable IMA_ARCH_POLICY. The decision to enable IMA_ARCH_POLICY is left up to whoever is building the kernel. The patch, at least this aspect of it, is correct. Mimi
Re: [PATCH] ima: add a new CONFIG for loading arch-specific policies
On Wed, 2020-02-26 at 14:10 -0500, Nayna Jain wrote: > Every time a new architecture defines the IMA architecture specific > functions - arch_ima_get_secureboot() and arch_ima_get_policy(), the IMA > include file needs to be updated. To avoid this "noise", this patch > defines a new IMA Kconfig IMA_SECURE_AND_OR_TRUSTED_BOOT option, allowing > the different architectures to select it. > > Suggested-by: Linus Torvalds > Signed-off-by: Nayna Jain > Cc: Ard Biesheuvel > Cc: Martin Schwidefsky > Cc: Philipp Rudo > Cc: Michael Ellerman > --- > arch/powerpc/Kconfig | 2 +- > arch/s390/Kconfig | 1 + > arch/x86/Kconfig | 1 + > include/linux/ima.h| 3 +-- > security/integrity/ima/Kconfig | 9 + > 5 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index 497b7d0b2d7e..b8ce1b995633 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -246,6 +246,7 @@ config PPC > select SYSCTL_EXCEPTION_TRACE > select THREAD_INFO_IN_TASK > select VIRT_TO_BUS if !PPC64 > + select IMA_SECURE_AND_OR_TRUSTED_BOOT if PPC_SECURE_BOOT > # > # Please keep this list sorted alphabetically. > # > @@ -978,7 +979,6 @@ 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/s390/Kconfig b/arch/s390/Kconfig > index 8abe77536d9d..90ff3633ade6 100644 > --- a/arch/s390/Kconfig > +++ b/arch/s390/Kconfig > @@ -195,6 +195,7 @@ config S390 > select ARCH_HAS_FORCE_DMA_UNENCRYPTED > select SWIOTLB > select GENERIC_ALLOCATOR > + select IMA_SECURE_AND_OR_TRUSTED_BOOT > > > config SCHED_OMIT_FRAME_POINTER > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index beea77046f9b..cafa66313fe2 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -230,6 +230,7 @@ config X86 > select VIRT_TO_BUS > select X86_FEATURE_NAMESif PROC_FS > select PROC_PID_ARCH_STATUS if PROC_FS > + select IMA_SECURE_AND_OR_TRUSTED_BOOT if EFI Not everyone is interested in enabling IMA or requiring IMA runtime policies. With this patch, enabling IMA_ARCH_POLICY is therefore still left up to the person building the kernel. As a result, I'm seeing the following warning, which is kind of cool. WARNING: unmet direct dependencies detected for IMA_SECURE_AND_OR_TRUSTED_BOOT Depends on [n]: INTEGRITY [=y] && IMA [=y] && IMA_ARCH_POLICY [=n] Selected by [y]: - X86 [=y] && EFI [=y] Ard, Michael, Martin, just making sure this type of warning is acceptable before upstreaming this patch. I would appreciate your tags. thanks! Mimi > > config INSTRUCTION_DECODER > def_bool y > diff --git a/include/linux/ima.h b/include/linux/ima.h > index 1659217e9b60..aefe758f4466 100644 > --- a/include/linux/ima.h > +++ b/include/linux/ima.h > @@ -30,8 +30,7 @@ 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) \ > - || defined(CONFIG_PPC_SECURE_BOOT) > +#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT > extern bool arch_ima_get_secureboot(void); > extern const char * const *arch_get_ima_policy(void); > #else > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig > index 3f3ee4e2eb0d..d17972aa413a 100644 > --- a/security/integrity/ima/Kconfig > +++ b/security/integrity/ima/Kconfig > @@ -327,3 +327,12 @@ config IMA_QUEUE_EARLY_BOOT_KEYS > depends on IMA_MEASURE_ASYMMETRIC_KEYS > depends on SYSTEM_TRUSTED_KEYRING > default y > + > +config IMA_SECURE_AND_OR_TRUSTED_BOOT > + bool > + depends on IMA > + depends on IMA_ARCH_POLICY > + default n > + help > +This option is selected by architectures to enable secure and/or > +trusted boot based on IMA runtime policies.
Re: [PATCH] ima: add a new CONFIG for loading arch-specific policies
On Mon, 2020-03-02 at 15:52 +0100, Ard Biesheuvel wrote: > On Mon, 2 Mar 2020 at 15:48, Mimi Zohar wrote: > > > > On Wed, 2020-02-26 at 14:10 -0500, Nayna Jain wrote: > > > Every time a new architecture defines the IMA architecture specific > > > functions - arch_ima_get_secureboot() and arch_ima_get_policy(), the IMA > > > include file needs to be updated. To avoid this "noise", this patch > > > defines a new IMA Kconfig IMA_SECURE_AND_OR_TRUSTED_BOOT option, allowing > > > the different architectures to select it. > > > > > > Suggested-by: Linus Torvalds > > > Signed-off-by: Nayna Jain > > > Cc: Ard Biesheuvel > > > Cc: Martin Schwidefsky > > > Cc: Philipp Rudo > > > Cc: Michael Ellerman > > > --- > > > arch/powerpc/Kconfig | 2 +- > > > arch/s390/Kconfig | 1 + > > > arch/x86/Kconfig | 1 + > > > include/linux/ima.h| 3 +-- > > > security/integrity/ima/Kconfig | 9 + > > > 5 files changed, 13 insertions(+), 3 deletions(-) > > > > > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > > > index 497b7d0b2d7e..b8ce1b995633 100644 > > > --- a/arch/powerpc/Kconfig > > > +++ b/arch/powerpc/Kconfig > > > @@ -246,6 +246,7 @@ config PPC > > > select SYSCTL_EXCEPTION_TRACE > > > select THREAD_INFO_IN_TASK > > > select VIRT_TO_BUS if !PPC64 > > > + select IMA_SECURE_AND_OR_TRUSTED_BOOT if PPC_SECURE_BOOT > > > # > > > # Please keep this list sorted alphabetically. > > > # > > > @@ -978,7 +979,6 @@ 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/s390/Kconfig b/arch/s390/Kconfig > > > index 8abe77536d9d..90ff3633ade6 100644 > > > --- a/arch/s390/Kconfig > > > +++ b/arch/s390/Kconfig > > > @@ -195,6 +195,7 @@ config S390 > > > select ARCH_HAS_FORCE_DMA_UNENCRYPTED > > > select SWIOTLB > > > select GENERIC_ALLOCATOR > > > + select IMA_SECURE_AND_OR_TRUSTED_BOOT > > > > > > > > > config SCHED_OMIT_FRAME_POINTER > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > > index beea77046f9b..cafa66313fe2 100644 > > > --- a/arch/x86/Kconfig > > > +++ b/arch/x86/Kconfig > > > @@ -230,6 +230,7 @@ config X86 > > > select VIRT_TO_BUS > > > select X86_FEATURE_NAMESif PROC_FS > > > select PROC_PID_ARCH_STATUS if PROC_FS > > > + select IMA_SECURE_AND_OR_TRUSTED_BOOT if EFI > > > > Not everyone is interested in enabling IMA or requiring IMA runtime > > policies. With this patch, enabling IMA_ARCH_POLICY is therefore > > still left up to the person building the kernel. As a result, I'm > > seeing the following warning, which is kind of cool. > > > > WARNING: unmet direct dependencies detected for > > IMA_SECURE_AND_OR_TRUSTED_BOOT > > Depends on [n]: INTEGRITY [=y] && IMA [=y] && IMA_ARCH_POLICY [=n] > > Selected by [y]: > > - X86 [=y] && EFI [=y] > > > > Ard, Michael, Martin, just making sure this type of warning is > > acceptable before upstreaming this patch. I would appreciate your > > tags. > > > > Ehm, no, warnings like these are not really acceptable. It means there > is an inconsistency in the way the Kconfig dependencies are defined. > > Does this help: > > select IMA_SECURE_AND_OR_TRUSTED_BOOT if EFI && IMA_ARCH_POLICY > > ? Yes, that's fine for x86. Michael, Martin, do you want something similar or would you prefer actually selecting IMA_ARCH_POLICY? Mimi
Re: [PATCH v2] ima: add a new CONFIG for loading arch-specific policies
On Tue, 2020-03-03 at 23:43 -0800, James Bottomley wrote: > On Tue, 2020-03-03 at 21:33 -0500, Nayna Jain wrote: > > diff --git a/security/integrity/ima/Kconfig > > b/security/integrity/ima/Kconfig > > index 3f3ee4e2eb0d..d17972aa413a 100644 > > --- a/security/integrity/ima/Kconfig > > +++ b/security/integrity/ima/Kconfig > > @@ -327,3 +327,12 @@ config IMA_QUEUE_EARLY_BOOT_KEYS > > depends on IMA_MEASURE_ASYMMETRIC_KEYS > > depends on SYSTEM_TRUSTED_KEYRING > > default y > > + > > +config IMA_SECURE_AND_OR_TRUSTED_BOOT > > + bool > > + depends on IMA > > + depends on IMA_ARCH_POLICY > > + default n > > You can't do this: a symbol designed to be selected can't depend on > other symbols because Kconfig doesn't see the dependencies during > select. We even have a doc for this now: > > Documentation/kbuild/Kconfig.select-break The document is discussing a circular dependency, where C selects B. IMA_SECURE_AND_OR_TRUSTED_BOOT is not selecting anything, but is being selected. All of the Kconfig's are now dependent on IMA_ARCH_POLICY being enabled before selecting IMA_SECURE_AND_OR_TRUSTED_BOOT. As Ard pointed out, both IMA and IMA_ARCH_POLICY are not needed, as IMA_ARCH_POLICY is already dependent on IMA. Mimi
Re: [PATCH v2] ima: add a new CONFIG for loading arch-specific policies
[Cc'ing Thomas Gleixner and x86 mailing list] On Wed, 2020-03-04 at 08:14 +0100, Ard Biesheuvel wrote: > On Wed, 4 Mar 2020 at 03:34, Nayna Jain wrote: > > > > Every time a new architecture defines the IMA architecture specific > > functions - arch_ima_get_secureboot() and arch_ima_get_policy(), the IMA > > include file needs to be updated. To avoid this "noise", this patch > > defines a new IMA Kconfig IMA_SECURE_AND_OR_TRUSTED_BOOT option, allowing > > the different architectures to select it. > > > > Suggested-by: Linus Torvalds > > Signed-off-by: Nayna Jain > > Cc: Ard Biesheuvel > > Cc: Philipp Rudo > > Cc: Michael Ellerman > > Acked-by: Ard Biesheuvel Thanks, Ard. > > for the x86 bits, but I'm not an x86 maintainer. Also, you may need to > split this if you want to permit arch maintainers to pick up their > parts individually. Michael, Philipp, Thomas, do you prefer separate patches? > > > --- > > v2: > > * Fixed the issue identified by Mimi. Thanks Mimi, Ard, Heiko and Michael > > for > > discussing the fix. > > > > arch/powerpc/Kconfig | 1 + > > arch/s390/Kconfig | 1 + > > arch/x86/Kconfig | 1 + > > include/linux/ima.h| 3 +-- > > security/integrity/ima/Kconfig | 9 + > > 5 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > > index 497b7d0b2d7e..a5cfde432983 100644 > > --- a/arch/powerpc/Kconfig > > +++ b/arch/powerpc/Kconfig > > @@ -979,6 +979,7 @@ config PPC_SECURE_BOOT > > bool > > depends on PPC_POWERNV > > depends on IMA_ARCH_POLICY > > + select IMA_SECURE_AND_OR_TRUSTED_BOOT > > 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/s390/Kconfig b/arch/s390/Kconfig > > index 8abe77536d9d..4a502fbcb800 100644 > > --- a/arch/s390/Kconfig > > +++ b/arch/s390/Kconfig > > @@ -195,6 +195,7 @@ config S390 > > select ARCH_HAS_FORCE_DMA_UNENCRYPTED > > select SWIOTLB > > select GENERIC_ALLOCATOR > > + select IMA_SECURE_AND_OR_TRUSTED_BOOT if IMA_ARCH_POLICY > > > > > > config SCHED_OMIT_FRAME_POINTER > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > index beea77046f9b..7f5bfaf0cbd2 100644 > > --- a/arch/x86/Kconfig > > +++ b/arch/x86/Kconfig > > @@ -230,6 +230,7 @@ config X86 > > select VIRT_TO_BUS > > select X86_FEATURE_NAMESif PROC_FS > > select PROC_PID_ARCH_STATUS if PROC_FS > > + select IMA_SECURE_AND_OR_TRUSTED_BOOT if EFI && IMA_ARCH_POLICY > > > > config INSTRUCTION_DECODER > > def_bool y > > diff --git a/include/linux/ima.h b/include/linux/ima.h > > index 1659217e9b60..aefe758f4466 100644 > > --- a/include/linux/ima.h > > +++ b/include/linux/ima.h > > @@ -30,8 +30,7 @@ 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) \ > > - || defined(CONFIG_PPC_SECURE_BOOT) > > +#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT > > extern bool arch_ima_get_secureboot(void); > > extern const char * const *arch_get_ima_policy(void); > > #else > > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig > > index 3f3ee4e2eb0d..d17972aa413a 100644 > > --- a/security/integrity/ima/Kconfig > > +++ b/security/integrity/ima/Kconfig > > @@ -327,3 +327,12 @@ config IMA_QUEUE_EARLY_BOOT_KEYS > > depends on IMA_MEASURE_ASYMMETRIC_KEYS > > depends on SYSTEM_TRUSTED_KEYRING > > default y > > + > > +config IMA_SECURE_AND_OR_TRUSTED_BOOT > > + bool > > + depends on IMA > > + depends on IMA_ARCH_POLICY > > Doesn't the latter already depend on the former? Yes, there's no need for the first. Mimi > > > + default n > > + help > > + This option is selected by architectures to enable secure and/or > > + trusted boot based on IMA runtime policies. > > -- > > 2.13.6 > >
Re: [PATCH v3] ima: add a new CONFIG for loading arch-specific policies
On Sun, 2020-03-08 at 20:57 -0400, Nayna Jain wrote: > From: Nayna Jain > > Every time a new architecture defines the IMA architecture specific > functions - arch_ima_get_secureboot() and arch_ima_get_policy(), the IMA > include file needs to be updated. To avoid this "noise", this patch > defines a new IMA Kconfig IMA_SECURE_AND_OR_TRUSTED_BOOT option, allowing > the different architectures to select it. > > Suggested-by: Linus Torvalds > Signed-off-by: Nayna Jain > Acked-by: Ard Biesheuvel > Cc: Philipp Rudo > Cc: Michael Ellerman Thanks, Michael for the suggestion of using "imply". Seems to be working nicely. Thanks, Nayna. I pushed this patch out to next- integrity-testing. Could we get some tags on this version of the patch? thanks, Mimi
Re: [PATCH v8 4/8] powerpc/ima: add measurement rules to ima arch specific policy
On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote:
> This patch adds the measurement rules to the arch specific policies on
> trusted boot enabled systems.
This version does not add rules to the existing arch specific policy,
but defines an arch specific trusted boot only policy and a combined
secure and trusted boot policy.
>
> Signed-off-by: Nayna Jain
> ---
> arch/powerpc/kernel/ima_arch.c | 34 +-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
> index 65d82ee74ea4..710872ea8f35 100644
> --- a/arch/powerpc/kernel/ima_arch.c
> +++ b/arch/powerpc/kernel/ima_arch.c
> @@ -26,6 +26,32 @@ static const char *const secure_rules[] = {
> NULL
> };
>
> +/*
> + * The "measure_rules" are enabled only on "trustedboot" enabled systems.
Please update the policy name to reflect the new "trusted_rules" name.
> + * These rules add the kexec kernel image and kernel modules file hashes to
> + * the IMA measurement list.
> + */
> +static const char *const trusted_rules[] = {
> + "measure func=KEXEC_KERNEL_CHECK",
> + "measure func=MODULE_CHECK",
> + NULL
> +};
> +
> +/*
> + * The "secure_and_trusted_rules" contains rules for both the secure boot and
> + * trusted boot. The "template=ima-modsig" option includes the appended
> + * signature, when available, in the IMA measurement list.
> + */
> +static const char *const secure_and_trusted_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",
> +#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.
> @@ -33,7 +59,13 @@ static const char *const secure_rules[] = {
> const char *const *arch_get_ima_policy(void)
> {
> if (is_ppc_secureboot_enabled())
> - return secure_rules;
> + if (is_ppc_trustedboot_enabled())
> + return secure_and_trusted_rules;
> + else
> + return secure_rules;
> + else
> + if (is_ppc_trustedboot_enabled())
No need for the "if" statement to be on a separate line. Please
combine the "else" and "if" statements.
Mimi
> + return trusted_rules;
>
> return NULL;
> }
Re: [PATCH v8 2/8] powerpc/ima: add support to initialize ima policy rules
On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote:
> index ..65d82ee74ea4
> --- /dev/null
> +++ b/arch/powerpc/kernel/ima_arch.c
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain
> + */
> +
> +#include
> +#include
> +
> +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.
Please add another sentence or two as a separate paragraph with an
explanation why the kernel module rule is conditional (eg. Only verify
the appended kernel module signatures once.)
> + */
> +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
> +};
> +
Mimi
Re: [PATCH v8 7/8] ima: check against blacklisted hashes for files with modsig
On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote:
> diff --git a/Documentation/ABI/testing/ima_policy
> b/Documentation/ABI/testing/ima_policy
> index 29ebe9afdac4..4c97afcc0f3c 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -25,6 +25,7 @@ Description:
> lsm:[[subj_user=] [subj_role=] [subj_type=]
>[obj_user=] [obj_role=] [obj_type=]]
> option: [[appraise_type=]] [template=] [permit_directio]
> + [appraise_flag=[check_blacklist]]
Like the other options, only "[[appraise_flag=]]" should be defined
here. The values should be defined in the "option:" section.
> base: func:=
> [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
> [FIRMWARE_CHECK]
>
> [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 136ae4e0ee92..7a002b08dde8 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -303,6 +304,36 @@ static int modsig_verify(enum ima_hooks func, const
> struct modsig *modsig,
> return rc;
> }
>
> +/*
> + * ima_blacklist_measurement - Checks whether the binary is blacklisted. If
Please update the function name to reflect the actual function name.
> + * yes, then adds the hash of the blacklisted binary to the measurement list.
Refer to Documentation/process/coding-style.rst section "8)
Commenting" on how to format function comments. Don't start a
sentence with "If yes,".
> + *
> + * Returns -EPERM if the hash is blacklisted.
> + */
> +int ima_check_blacklist(struct integrity_iint_cache *iint,
> + const struct modsig *modsig, int pcr)
> +{
> + enum hash_algo hash_algo;
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 5380aca2b351..bfaae7a8443a 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1172,6 +1173,11 @@ static int ima_parse_rule(char *rule, struct
> ima_rule_entry *entry)
> else
> result = -EINVAL;
> break;
> + case Opt_appraise_flag:
> + ima_log_string(ab, "appraise_flag", args[0].from);
> + if (strstr(args[0].from, "blacklist"))
> + entry->flags |= IMA_CHECK_BLACKLIST;
> + break;
When adding a new policy rule option, ima_policy_show() needs to be
updated as well.
Mimi
> case Opt_permit_directio:
> entry->flags |= IMA_PERMIT_DIRECTIO;
> break;
>
Re: [PATCH v8 5/8] ima: make process_buffer_measurement() generic
On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote:
> process_buffer_measurement() is limited to measuring the kexec boot
> command line. This patch makes process_buffer_measurement() more
> generic, allowing it to measure other types of buffer data (e.g.
> blacklisted binary hashes or key hashes).
based on "func".
>
> This patch modifies the function to conditionally retrieve the policy
> defined pcr and template based on the func.
This would be done in a subsequent patch, not here.
> @@ -642,19 +642,38 @@ static void process_buffer_measurement(const void *buf,
> int size,
> .filename = eventname,
> .buf = buf,
> .buf_len = size};
> - struct ima_template_desc *template_desc = NULL;
> + struct ima_template_desc *template = NULL;
> struct {
> struct ima_digest_data hdr;
> char digest[IMA_MAX_DIGEST_SIZE];
> } hash = {};
> int violation = 0;
> - int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> int action = 0;
> + u32 secid;
>
> - action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr,
> - &template_desc);
> - if (!(action & IMA_MEASURE))
> - return;
> + if (func) {
> + security_task_getsecid(current, &secid);
> + action = ima_get_action(NULL, current_cred(), secid, 0, func,
> + &pcr, &template);
> + if (!(action & IMA_MEASURE))
> + return;
> + }
> +
Initially there is no need to test "func". A specific "func" test
would be added as needed.
Mimi
Re: [PATCH v8 3/8] powerpc: detect the trusted boot state of the system
On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote:
> While secure boot permits only properly verified signed kernels to be
> booted, trusted boot takes a measurement of the kernel image prior to
> boot that can be subsequently compared against good known values via
> attestation services.
>
Instead of "takes a measurement", either "stores a measurement" or
"calculates the file hash of the kernel image and stores the
measurement prior to boot, that".
> This patch reads the trusted boot state of a PowerNV system. The state
> is used to conditionally enable additional measurement rules in the IMA
> arch-specific policies.
>
> Signed-off-by: Nayna Jain
> ---
> arch/powerpc/include/asm/secure_boot.h | 6 ++
> arch/powerpc/kernel/secure_boot.c | 24
> 2 files changed, 30 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/secure_boot.h
> b/arch/powerpc/include/asm/secure_boot.h
> index 07d0fe0ca81f..a2ff556916c6 100644
> --- a/arch/powerpc/include/asm/secure_boot.h
> +++ b/arch/powerpc/include/asm/secure_boot.h
>
> diff --git a/arch/powerpc/kernel/secure_boot.c
> b/arch/powerpc/kernel/secure_boot.c
> index 99bba7915629..9753470ab08a 100644
> --- a/arch/powerpc/kernel/secure_boot.c
> +++ b/arch/powerpc/kernel/secure_boot.c
> @@ -7,6 +7,17 @@
> #include
> #include
>
> +static struct device_node *get_ppc_fw_sb_node(void)
> +{
> + static const struct of_device_id ids[] = {
> + { .compatible = "ibm,secureboot-v1", },
> + { .compatible = "ibm,secureboot-v2", },
> + {},
> + };
> +
scripts/checkpatch.pl is complaining that secureboot-v1, secureboot-v2
are not documented in the device tree bindings.
Mimi
Re: [PATCH v8 7/8] ima: check against blacklisted hashes for files with modsig
On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote: > Asymmetric private keys are used to sign multiple files. The kernel > currently support checking against blacklisted keys. However, if the > public key is blacklisted, any file signed by the blacklisted key will > automatically fail signature verification. We might not want to blacklist > all the files signed by a particular key, but just a single file. > Blacklisting the public key is not fine enough granularity. > > This patch adds support for checking against the blacklisted hash of the > file based on the IMA policy. The blacklisted hash is the file hash > without the appended signature. Defined is a new policy option > "appraise_flag=check_blacklist". Please add an example of how to blacklist a file with an appended signature. The simplest example that works on x86 as well as Power would be blacklisting a kernel module. The example should include calculating the kernel module hash without the appended signature, enabling the Kconfig option (CONFIG_SYSTEM_BLACKLIST_HASH_LIST), and the blacklist hash format (eg. "bin:"). thanks, Mimi
Re: [PATCH v8 7/8] ima: check against blacklisted hashes for files with modsig
On Sun, 2019-10-20 at 12:06 -0400, Mimi Zohar wrote: > On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote: > > Asymmetric private keys are used to sign multiple files. The kernel > > currently support checking against blacklisted keys. However, if the > > public key is blacklisted, any file signed by the blacklisted key will > > automatically fail signature verification. We might not want to blacklist > > all the files signed by a particular key, but just a single file. > > Blacklisting the public key is not fine enough granularity. > > > > This patch adds support for checking against the blacklisted hash of the > > file based on the IMA policy. The blacklisted hash is the file hash > > without the appended signature. Defined is a new policy option > > "appraise_flag=check_blacklist". > > Please add an example of how to blacklist a file with an appended > signature. The simplest example that works on x86 as well as Power > would be blacklisting a kernel module. The example should include > calculating the kernel module hash without the appended signature, > enabling the Kconfig option (CONFIG_SYSTEM_BLACKLIST_HASH_LIST), and > the blacklist hash format (eg. "bin:"). And of course, the IMA appraise kernel module policy rule containing "appraise_flag=check_blacklist". thanks, Mimi
Re: [PATCH v9 2/8] powerpc/ima: add support to initialize ima policy rules
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
Re: [PATCH v9 5/8] ima: make process_buffer_measurement() generic
On Fri, 2019-10-25 at 10:32 -0700, Lakshmi Ramasubramanian wrote:
>
> On 10/25/2019 10:24 AM, Nayna Jain wrote:
> >
> > On 10/24/19 10:20 AM, Lakshmi Ramasubramanian wrote:
> >> On 10/23/19 8:47 PM, Nayna Jain wrote:
> >>
> >> Hi Nayna,
> >>
> >>> +void process_buffer_measurement(const void *buf, int size,
> >>> + const char *eventname, enum ima_hooks func,
> >>> + int pcr)
> >>> {
> >>> int ret = 0;
> >>> struct ima_template_entry *entry = NULL;
> >>
> >>> + if (func) {
Let's comment this line. Perhaps something like /*Unnecessary for
auxiliary buffer measurements */
> >>> + security_task_getsecid(current, &secid);
> >>> + action = ima_get_action(NULL, current_cred(), secid, 0, func,
> >>> + &pcr, &template);
> >>> + if (!(action & IMA_MEASURE))
> >>> + return;
> >>> + }
> >>
> >> In your change set process_buffer_measurement is called with NONE for
> >> the parameter func. So ima_get_action (the above if block) will not be
> >> executed.
> >>
> >> Wouldn't it better to update ima_get_action (and related functions) to
> >> handle the ima policy (func param)?
> >
> >
> > The idea is to use ima-buf template for the auxiliary measurement
> > record. The auxiliary measurement record is an additional record to the
> > one already created based on the existing policy. When func is passed as
> > NONE, it represents it is an additional record. I am not sure what you
> > mean by updating ima_get_action, it is already handling the ima policy.
> >
>
> I was referring to using "func" in process_buffer_measurement to
> determine ima action. In my opinion, process_buffer_measurement should
> be generic.
>
> ima_get_action() should instead determine the required ima action,
> template, pcr, etc. based on "func" passed to it.
Nayna's original patch moved ima_get_action() into the caller, but
that resulted in code duplication in each of the callers. This
solution differentiates between the initial, which requires calling
ima_get_action(), and auxiliary buffer measurement records.
Mimi
Re: [PATCH v9 2/8] powerpc/ima: add support to initialize ima policy rules
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
Re: [PATCH v9 0/8] powerpc: Enabling IMA arch specific secure boot policies
On Wed, 2019-10-23 at 22:47 -0500, Nayna Jain wrote: > This patchset extends the previous version[1] by adding support for > checking against a blacklist of binary hashes. > > The IMA subsystem supports custom, built-in, arch-specific policies to > define the files to be measured and appraised. These policies are honored > based on priority, where arch-specific policy is the highest and custom > is the lowest. > > PowerNV system uses a Linux-based bootloader to kexec the OS. The > bootloader kernel relies on IMA for signature verification of the OS > kernel before doing the kexec. This patchset adds support for powerpc > arch-specific IMA policies that are conditionally defined based on a > system's secure boot and trusted boot states. The OS secure boot and > trusted boot states are determined via device-tree properties. > > The verification needs to be performed only for binaries that are not > blacklisted. The kernel currently only checks against the blacklist of > keys. However, doing so results in blacklisting all the binaries that > are signed by the same key. In order to prevent just one particular > binary from being loaded, it must be checked against a blacklist of > binary hashes. This patchset also adds support to IMA for checking > against a hash blacklist for files. signed by appended signature. > > [1] http://patchwork.ozlabs.org/cover/1149262/ Thanks, Nayna. Please feel free to add my Signed-off-by tag on patches (2, 4, 5, 7 & 8). thanks, Mimi
Re: [PATCH v9 5/8] ima: make process_buffer_measurement() generic
On Wed, 2019-10-30 at 08:22 -0700, Lakshmi Ramasubramanian wrote: > On 10/23/19 8:47 PM, Nayna Jain wrote: > > Hi Nayna, > > > process_buffer_measurement() is limited to measuring the kexec boot > > command line. This patch makes process_buffer_measurement() more > > generic, allowing it to measure other types of buffer data (e.g. > > blacklisted binary hashes or key hashes). > > Now that process_buffer_measurement() is being made generic to measure > any buffer, it would be good to add a tag to indicate what type of > buffer is being measured. > > For example, if the buffer is kexec command line the log could look like: > > "kexec_cmdline: " > > Similarly, if the buffer is blacklisted binary hash: > > "blacklist hash: ". > > If the buffer is key hash: > > ": key data". > > This would greatly help the consumer of the IMA log to know the type of > data represented in each IMA log entry. Both the existing kexec command line and the new blacklist buffer measurement pass that information in the eventname. The [PATCH 7/8] "ima: check against blacklisted hashes for files with modsig" patch description includes an example. Mimi
[PATCH v10 0/9] powerpc: Enabling IMA arch specific secure boot policies
powerpc_sb_mode() function. * Added dependency for PPC_SECURE_BOOT on configs PPC64 and OPAL_SECVAR * Replaced obj-$(CONFIG_IMA) with obj-$(CONFIG_PPC_SECURE_BOOT) in arch/powerpc/kernel/Makefile Mimi Zohar (1): powerpc/ima: indicate kernel modules appended signatures are enforced Nayna Jain (8): powerpc: detect the secure boot mode of the system powerpc/ima: add support to initialize ima policy rules powerpc: detect the trusted boot state of the system powerpc/ima: define trusted boot policy ima: make process_buffer_measurement() generic certs: add wrapper function to check blacklisted binary hash ima: check against blacklisted hashes for files with modsig powerpc/ima: update ima arch policy to check for blacklist Documentation/ABI/testing/ima_policy | 4 ++ arch/powerpc/Kconfig | 11 + arch/powerpc/include/asm/secure_boot.h | 29 + arch/powerpc/kernel/Makefile | 2 + arch/powerpc/kernel/ima_arch.c | 78 ++ arch/powerpc/kernel/secure_boot.c | 58 + certs/blacklist.c | 9 include/keys/system_keyring.h | 6 +++ include/linux/ima.h| 3 +- security/integrity/ima/ima.h | 11 + security/integrity/ima/ima_appraise.c | 33 ++ security/integrity/ima/ima_main.c | 70 -- security/integrity/ima/ima_policy.c| 12 +- security/integrity/integrity.h | 1 + 14 files changed, 302 insertions(+), 25 deletions(-) create mode 100644 arch/powerpc/include/asm/secure_boot.h create mode 100644 arch/powerpc/kernel/ima_arch.c create mode 100644 arch/powerpc/kernel/secure_boot.c -- 2.7.5
[PATCH v10 2/9] powerpc/ima: add support to initialize ima policy rules
From: Nayna Jain
PowerNV systems use a Linux-based bootloader, which rely 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
Signed-off-by: Mimi Zohar
---
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
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 ..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
+#include
+
+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
--
2.7.5
[PATCH v10 3/9] powerpc: detect the trusted boot state of the system
From: Nayna Jain
While secure boot permits only properly verified signed kernels to be
booted, trusted boot calculates the file hash of the kernel image and
stores the measurement prior to boot, that can be subsequently compared
against good known values via attestation services.
This patch reads the trusted boot state of a PowerNV system. The state
is used to conditionally enable additional measurement rules in the IMA
arch-specific policies.
Signed-off-by: Nayna Jain
---
arch/powerpc/include/asm/secure_boot.h | 6 ++
arch/powerpc/kernel/secure_boot.c | 26 ++
2 files changed, 32 insertions(+)
diff --git a/arch/powerpc/include/asm/secure_boot.h
b/arch/powerpc/include/asm/secure_boot.h
index 07d0fe0ca81f..a2ff556916c6 100644
--- a/arch/powerpc/include/asm/secure_boot.h
+++ b/arch/powerpc/include/asm/secure_boot.h
@@ -11,6 +11,7 @@
#ifdef CONFIG_PPC_SECURE_BOOT
bool is_ppc_secureboot_enabled(void);
+bool is_ppc_trustedboot_enabled(void);
#else
@@ -19,5 +20,10 @@ static inline bool is_ppc_secureboot_enabled(void)
return false;
}
+static inline bool is_ppc_trustedboot_enabled(void)
+{
+ return false;
+}
+
#endif
#endif
diff --git a/arch/powerpc/kernel/secure_boot.c
b/arch/powerpc/kernel/secure_boot.c
index 63dc82c50862..a6a5f17ede03 100644
--- a/arch/powerpc/kernel/secure_boot.c
+++ b/arch/powerpc/kernel/secure_boot.c
@@ -7,6 +7,17 @@
#include
#include
+static struct device_node *get_ppc_fw_sb_node(void)
+{
+ static const struct of_device_id ids[] = {
+ { .compatible = "ibm,secureboot-v1", },
+ { .compatible = "ibm,secureboot-v2", },
+ {},
+ };
+
+ return of_find_matching_node(NULL, ids);
+}
+
bool is_ppc_secureboot_enabled(void)
{
struct device_node *node;
@@ -30,3 +41,18 @@ bool is_ppc_secureboot_enabled(void)
pr_info("Secure boot mode %s\n", enabled ? "enabled" : "disabled");
return enabled;
}
+
+bool is_ppc_trustedboot_enabled(void)
+{
+ struct device_node *node;
+ bool enabled = false;
+
+ node = get_ppc_fw_sb_node();
+ enabled = of_property_read_bool(node, "trusted-enabled");
+
+ of_node_put(node);
+
+ pr_info("Trusted boot mode %s\n", enabled ? "enabled" : "disabled");
+
+ return enabled;
+}
--
2.7.5
[PATCH v10 1/9] powerpc: detect the secure boot mode of the system
From: Nayna Jain
This patch defines a function to detect the secure boot state of a
PowerNV system.
The PPC_SECURE_BOOT config represents the base enablement of secure boot
for powerpc.
Signed-off-by: Nayna Jain
---
arch/powerpc/Kconfig | 10 ++
arch/powerpc/include/asm/secure_boot.h | 23 +++
arch/powerpc/kernel/Makefile | 2 ++
arch/powerpc/kernel/secure_boot.c | 32
4 files changed, 67 insertions(+)
create mode 100644 arch/powerpc/include/asm/secure_boot.h
create mode 100644 arch/powerpc/kernel/secure_boot.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 3e56c9c2f16e..56ea0019b616 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -934,6 +934,16 @@ config PPC_MEM_KEYS
If unsure, say y.
+config PPC_SECURE_BOOT
+ prompt "Enable secure boot support"
+ bool
+ depends on PPC_POWERNV
+ help
+ Systems with firmware secure boot enabled need to define security
+ policies to extend secure boot to the OS. This config allows a user
+ to enable OS secure boot on systems that have firmware support for
+ it. If in doubt say N.
+
endmenu
config ISA_DMA_API
diff --git a/arch/powerpc/include/asm/secure_boot.h
b/arch/powerpc/include/asm/secure_boot.h
new file mode 100644
index ..07d0fe0ca81f
--- /dev/null
+++ b/arch/powerpc/include/asm/secure_boot.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Secure boot definitions
+ *
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain
+ */
+#ifndef _ASM_POWER_SECURE_BOOT_H
+#define _ASM_POWER_SECURE_BOOT_H
+
+#ifdef CONFIG_PPC_SECURE_BOOT
+
+bool is_ppc_secureboot_enabled(void);
+
+#else
+
+static inline bool is_ppc_secureboot_enabled(void)
+{
+ return false;
+}
+
+#endif
+#endif
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index a7ca8fe62368..e2a54fa240ac 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -161,6 +161,8 @@ ifneq ($(CONFIG_PPC_POWERNV)$(CONFIG_PPC_SVM),)
obj-y += ucall.o
endif
+obj-$(CONFIG_PPC_SECURE_BOOT) += secure_boot.o
+
# Disable GCOV, KCOV & sanitizers in odd or sensitive code
GCOV_PROFILE_prom_init.o := n
KCOV_INSTRUMENT_prom_init.o := n
diff --git a/arch/powerpc/kernel/secure_boot.c
b/arch/powerpc/kernel/secure_boot.c
new file mode 100644
index ..63dc82c50862
--- /dev/null
+++ b/arch/powerpc/kernel/secure_boot.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain
+ */
+#include
+#include
+#include
+
+bool is_ppc_secureboot_enabled(void)
+{
+ struct device_node *node;
+ bool enabled = false;
+
+ node = of_find_compatible_node(NULL, NULL, "ibm,secvar-v1");
+ if (!of_device_is_available(node)) {
+ pr_err("Cannot find secure variable node in device tree;
failing to secure state\n");
+ goto out;
+ }
+
+ /*
+* secureboot is enabled if os-secure-enforcing property exists,
+* else disabled.
+*/
+ enabled = of_property_read_bool(node, "os-secure-enforcing");
+
+out:
+ of_node_put(node);
+
+ pr_info("Secure boot mode %s\n", enabled ? "enabled" : "disabled");
+ return enabled;
+}
--
2.7.5
[PATCH v10 4/9] powerpc/ima: define trusted boot policy
From: Nayna Jain
This patch defines an arch-specific trusted boot only policy and a
combined secure and trusted boot policy.
Signed-off-by: Nayna Jain
Signed-off-by: Mimi Zohar
---
arch/powerpc/kernel/ima_arch.c | 33 -
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
index d88913dc0da7..0ef5956c9753 100644
--- a/arch/powerpc/kernel/ima_arch.c
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -31,13 +31,44 @@ static const char *const secure_rules[] = {
};
/*
+ * The "trusted_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 trusted_rules[] = {
+ "measure func=KEXEC_KERNEL_CHECK",
+ "measure func=MODULE_CHECK",
+ NULL
+};
+
+/*
+ * The "secure_and_trusted_rules" contains rules for both the secure boot and
+ * trusted boot. The "template=ima-modsig" option includes the appended
+ * signature, when available, in the IMA measurement list.
+ */
+static const char *const secure_and_trusted_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",
+#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;
+ if (is_ppc_trustedboot_enabled())
+ return secure_and_trusted_rules;
+ else
+ return secure_rules;
+ else if (is_ppc_trustedboot_enabled())
+ return trusted_rules;
return NULL;
}
--
2.7.5
[PATCH v10 5/9] ima: make process_buffer_measurement() generic
From: Nayna Jain
process_buffer_measurement() is limited to measuring the kexec boot
command line. This patch makes process_buffer_measurement() more
generic, allowing it to measure other types of buffer data (e.g.
blacklisted binary hashes or key hashes).
process_buffer_measurement() may be called directly from an IMA
hook or as an auxiliary measurement record. In both cases the buffer
measurement is based on policy. This patch modifies the function to
conditionally retrieve the policy defined PCR and template for the IMA
hook case.
Signed-off-by: Nayna Jain
[zo...@linux.ibm.com: added comment in process_buffer_measurement()]
Signed-off-by: Mimi Zohar
---
security/integrity/ima/ima.h | 3 ++
security/integrity/ima/ima_main.c | 58 +++
2 files changed, 43 insertions(+), 18 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 3689081aaf38..a65772ffa427 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -217,6 +217,9 @@ void ima_store_measurement(struct integrity_iint_cache
*iint, struct file *file,
struct evm_ima_xattr_data *xattr_value,
int xattr_len, const struct modsig *modsig, int pcr,
struct ima_template_desc *template_desc);
+void process_buffer_measurement(const void *buf, int size,
+ const char *eventname, enum ima_hooks func,
+ int pcr);
void ima_audit_measurement(struct integrity_iint_cache *iint,
const unsigned char *filename);
int ima_alloc_init_template(struct ima_event_data *event_data,
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 60027c643ecd..a26e3ad4e886 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -626,14 +626,14 @@ int ima_load_data(enum kernel_load_data_id id)
* @buf: pointer to the buffer that needs to be added to the log.
* @size: size of buffer(in bytes).
* @eventname: event name to be used for the buffer entry.
- * @cred: a pointer to a credentials structure for user validation.
- * @secid: the secid of the task to be validated.
+ * @func: IMA hook
+ * @pcr: pcr to extend the measurement
*
* Based on policy, the buffer is measured into the ima log.
*/
-static void process_buffer_measurement(const void *buf, int size,
- const char *eventname,
- const struct cred *cred, u32 secid)
+void process_buffer_measurement(const void *buf, int size,
+ const char *eventname, enum ima_hooks func,
+ int pcr)
{
int ret = 0;
struct ima_template_entry *entry = NULL;
@@ -642,19 +642,45 @@ static void process_buffer_measurement(const void *buf,
int size,
.filename = eventname,
.buf = buf,
.buf_len = size};
- struct ima_template_desc *template_desc = NULL;
+ struct ima_template_desc *template = NULL;
struct {
struct ima_digest_data hdr;
char digest[IMA_MAX_DIGEST_SIZE];
} hash = {};
int violation = 0;
- int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
int action = 0;
+ u32 secid;
- action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr,
- &template_desc);
- if (!(action & IMA_MEASURE))
- return;
+ /*
+* Both LSM hooks and auxilary based buffer measurements are
+* based on policy. To avoid code duplication, differentiate
+* between the LSM hooks and auxilary buffer measurements,
+* retrieving the policy rule information only for the LSM hook
+* buffer measurements.
+*/
+ if (func) {
+ security_task_getsecid(current, &secid);
+ action = ima_get_action(NULL, current_cred(), secid, 0, func,
+ &pcr, &template);
+ if (!(action & IMA_MEASURE))
+ return;
+ }
+
+ if (!pcr)
+ pcr = CONFIG_IMA_MEASURE_PCR_IDX;
+
+ if (!template) {
+ template = lookup_template_desc("ima-buf");
+ ret = template_desc_init_fields(template->fmt,
+ &(template->fields),
+ &(template->num_fields));
+ if (ret < 0) {
+ pr_err("template %s init failed, result: %d\n",
+ (strlen(template->name) ?
+ template->name : template->fmt), ret);
+
[PATCH v10 6/9] certs: add wrapper function to check blacklisted binary hash
From: Nayna Jain
The -EKEYREJECTED error returned by existing is_hash_blacklisted() is
misleading when called for checking against blacklisted hash of a
binary.
This patch adds a wrapper function is_binary_blacklisted() to return
-EPERM error if binary is blacklisted.
Signed-off-by: Nayna Jain
Cc: David Howells
Reviewed-by: Mimi Zohar
---
certs/blacklist.c | 9 +
include/keys/system_keyring.h | 6 ++
2 files changed, 15 insertions(+)
diff --git a/certs/blacklist.c b/certs/blacklist.c
index ec00bf337eb6..6514f9ebc943 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -135,6 +135,15 @@ int is_hash_blacklisted(const u8 *hash, size_t hash_len,
const char *type)
}
EXPORT_SYMBOL_GPL(is_hash_blacklisted);
+int is_binary_blacklisted(const u8 *hash, size_t hash_len)
+{
+ if (is_hash_blacklisted(hash, hash_len, "bin") == -EKEYREJECTED)
+ return -EPERM;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(is_binary_blacklisted);
+
/*
* Initialise the blacklist
*/
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index c1a96fdf598b..fb8b07daa9d1 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -35,12 +35,18 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
extern int mark_hash_blacklisted(const char *hash);
extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
const char *type);
+extern int is_binary_blacklisted(const u8 *hash, size_t hash_len);
#else
static inline int is_hash_blacklisted(const u8 *hash, size_t hash_len,
const char *type)
{
return 0;
}
+
+static inline int is_binary_blacklisted(const u8 *hash, size_t hash_len)
+{
+ return 0;
+}
#endif
#ifdef CONFIG_IMA_BLACKLIST_KEYRING
--
2.7.5
[PATCH v10 7/9] ima: check against blacklisted hashes for files with modsig
From: Nayna Jain Asymmetric private keys are used to sign multiple files. The kernel currently supports checking against blacklisted keys. However, if the public key is blacklisted, any file signed by the blacklisted key will automatically fail signature verification. Blacklisting the public key is not fine enough granularity, as we might want to only blacklist a particular file. This patch adds support for checking against the blacklisted hash of the file, without the appended signature, based on the IMA policy. It defines a new policy option "appraise_flag=check_blacklist". In addition to the blacklisted binary hashes stored in the firmware "dbx" variable, the Linux kernel may be configured to load blacklisted binary hashes onto the .blacklist keyring as well. The following example shows how to blacklist a specific kernel module hash. $ sha256sum kernel/kheaders.ko 77fa889b35a05338ec52e51591c1b89d4c8d1c99a21251d7c22b1a8642a6bad3 kernel/kheaders.ko $ grep BLACKLIST .config CONFIG_SYSTEM_BLACKLIST_KEYRING=y CONFIG_SYSTEM_BLACKLIST_HASH_LIST="blacklist-hash-list" $ cat certs/blacklist-hash-list "bin:77fa889b35a05338ec52e51591c1b89d4c8d1c99a21251d7c22b1a8642a6bad3" Update the IMA custom measurement and appraisal policy rules (/etc/ima-policy): measure func=MODULE_CHECK template=ima-modsig appraise func=MODULE_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig After building, installing, and rebooting the kernel: 545660333 ---lswrv 0 0 \_ blacklist: bin:77fa889b35a05338ec52e51591c1b89d4c8d1c99a21251d7c22b1a8642a6bad3 measure func=MODULE_CHECK template=ima-modsig appraise func=MODULE_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig modprobe: ERROR: could not insert 'kheaders': Permission denied 10 0c9834db5a0182c1fb0cdc5d3adcf11a11fd83dd ima-sig sha256:3bc6ed4f0b4d6e31bc1dbc9ef844605abc7afdc6d81a57d77a1ec9407997c40 2 /usr/lib/modules/5.4.0-rc3+/kernel/kernel/kheaders.ko 10 82aad2bcc3fa8ed94762356b5c14838f3bcfa6a0 ima-modsig sha256:3bc6ed4f0b4d6e31bc1dbc9ef844605abc7afdc6d81a57d77a1ec9407997c40 2 /usr/lib/modules/5.4.0rc3+/kernel/kernel/kheaders.ko sha256:77fa889b3 5a05338ec52e51591c1b89d4c8d1c99a21251d7c22b1a8642a6bad3 3082029a06092a864886f70d010702a082028b30820287020101310d300b0609608648 016503040201300b06092a864886f70d01070131820264 10 25b72217cc1152b44b134ce2cd68f12dfb71acb3 ima-buf sha256:8b58427fedcf8f4b20bc8dc007f2e232bf7285d7b93a66476321f9c2a3aa132 b blacklisted-hash 77fa889b35a05338ec52e51591c1b89d4c8d1c99a21251d7c22b1a8642a6bad3 Signed-off-by: Nayna Jain Cc: Jessica Yu Cc: David Howells [zo...@linux.ibm.com: updated patch description] Signed-off-by: Mimi Zohar --- Documentation/ABI/testing/ima_policy | 4 security/integrity/ima/ima.h | 8 security/integrity/ima/ima_appraise.c | 33 + security/integrity/ima/ima_main.c | 12 security/integrity/ima/ima_policy.c | 12 ++-- security/integrity/integrity.h| 1 + 6 files changed, 64 insertions(+), 6 deletions(-) diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy index 29ebe9afdac4..29aaedf33246 100644 --- a/Documentation/ABI/testing/ima_policy +++ b/Documentation/ABI/testing/ima_policy @@ -25,6 +25,7 @@ Description: lsm:[[subj_user=] [subj_role=] [subj_type=] [obj_user=] [obj_role=] [obj_type=]] option: [[appraise_type=]] [template=] [permit_directio] + [appraise_flag=] base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK] [FIRMWARE_CHECK] [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK] @@ -38,6 +39,9 @@ Description: fowner:= decimal value lsm:are LSM specific option: appraise_type:= [imasig] [imasig|modsig] + appraise_flag:= [check_blacklist] + Currently, blacklist check is only for files signed with appended + signature. template:= name of a defined IMA template type (eg, ima-ng). Only valid when action is "measure". pcr:= decimal value diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index a65772ffa427..df4ca482fb53 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -256,6 +256,8 @@ int ima_policy_show(struct seq_file *m, void *v); #define IMA_APPRAISE_KEXEC 0x40 #ifdef CONFIG_IMA_APPRAISE +int ima_check_blacklist(struct integrity_iint_cache *iint, + const struct modsig *modsig, int pcr); int ima_appraise_measurement(enum ima_hooks func, struct integrity_iint_cache *iint,
[PATCH v10 8/9] powerpc/ima: update ima arch policy to check for blacklist
From: Nayna Jain
This patch updates the arch-specific policies for PowerNV system to make
sure that the binary hash is not blacklisted.
Signed-off-by: Nayna Jain
Cc: Jessica Yu
Signed-off-by: Mimi Zohar
---
arch/powerpc/kernel/ima_arch.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
index 0ef5956c9753..b9de0fb45bb9 100644
--- a/arch/powerpc/kernel/ima_arch.c
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -23,9 +23,9 @@ bool arch_ima_get_secureboot(void)
* is not enabled.
*/
static const char *const secure_rules[] = {
- "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+ "appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist
appraise_type=imasig|modsig",
#ifndef CONFIG_MODULE_SIG_FORCE
- "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+ "appraise func=MODULE_CHECK appraise_flag=check_blacklist
appraise_type=imasig|modsig",
#endif
NULL
};
@@ -49,9 +49,9 @@ static const char *const trusted_rules[] = {
static const char *const secure_and_trusted_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",
+ "appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist
appraise_type=imasig|modsig",
#ifndef CONFIG_MODULE_SIG_FORCE
- "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+ "appraise func=MODULE_CHECK appraise_flag=check_blacklist
appraise_type=imasig|modsig",
#endif
NULL
};
--
2.7.5
[RFC PATCH v10 9/9] powerpc/ima: indicate kernel modules appended signatures are enforced
The arch specific kernel module policy rule requires kernel modules to
be signed, either as an IMA signature, stored as an xattr, or as an
appended signature. As a result, kernel modules appended signatures
could be enforced without "sig_enforce" being set or reflected in
/sys/module/module/parameters/sig_enforce. This patch sets
"sig_enforce".
Signed-off-by: Mimi Zohar
Cc: Jessica Yu
---
arch/powerpc/kernel/ima_arch.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
index b9de0fb45bb9..e34116255ced 100644
--- a/arch/powerpc/kernel/ima_arch.c
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -62,13 +62,17 @@ static const char *const secure_and_trusted_rules[] = {
*/
const char *const *arch_get_ima_policy(void)
{
- if (is_ppc_secureboot_enabled())
+ if (is_ppc_secureboot_enabled()) {
+ if (IS_ENABLED(CONFIG_MODULE_SIG))
+ set_module_sig_enforced();
+
if (is_ppc_trustedboot_enabled())
return secure_and_trusted_rules;
else
return secure_rules;
- else if (is_ppc_trustedboot_enabled())
+ } else if (is_ppc_trustedboot_enabled()) {
return trusted_rules;
+ }
return NULL;
}
--
2.7.5
Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-1 tag
On Sat, 2019-11-30 at 14:42 -0800, Linus Torvalds wrote: > [ Only tangentially related to the power parts ] > > On Sat, Nov 30, 2019 at 2:41 AM Michael Ellerman wrote: > > > > There's some changes in security/integrity as part of the secure boot work. > > They > > were all either written by or acked/reviewed by Mimi. > > -#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) > > This clearly should be its own CONFIG variable, and be generated by > having the different architectures just select it. > > IOW, IMA should probably have a > >config IMA_SECURE_BOOT > > and then s390 would just do the select unconditionally, while x86 and > ppc would do > > select IMA_SECURE_BOOT if EFI > > and > > select IMA_SECURE_BOOT if PPC_SECURE_BOOT > > respectively. > > And then we wouldn't have random architectures adding random "me me me > tooo!!!" type code. Agreed, but the naming is a bit off. The flag somehow needs to take into account "trusted boot" as well. On s390, only secure boot is enabled, at least for the time being. On x86, both secure and trusted boot are enabled. On powerpc, the architecture properly enables secure and/or trusted boot based on OPAL flags. It's a bit long, but could the flag be named IMA_SECURE_AND_OR_TRUSTED_BOOT? thanks, Mimi
Re: [PATCH v10 0/9] powerpc: Enabling IMA arch specific secure boot policies
On Mon, 2019-12-09 at 12:27 -0800, Lakshmi Ramasubramanian wrote: > Hi Mimi, > > On 10/30/2019 8:31 PM, Mimi Zohar wrote: > > > This patchset extends the previous version[1] by adding support for > > checking against a blacklist of binary hashes. > > > > The IMA subsystem supports custom, built-in, arch-specific policies to > > define the files to be measured and appraised. These policies are honored > > based on priority, where arch-specific policy is the highest and custom > > is the lowest. > > Has this change been signed off and merged for the next update of the > kernel (v5.5)? Yes, refer to the linuxppc mailing list archives. Mimi [1] https://lists.ozlabs.org/pipermail/linuxppc-dev/
Re: [PATCH v2] powerpc/ima: fix secure boot rules in ima arch policy
On Fri, 2020-05-01 at 10:16 -0400, Nayna Jain wrote:
> To prevent verifying the kernel module appended signature twice
> (finit_module), once by the module_sig_check() and again by IMA, powerpc
> secure boot rules define an IMA architecture specific policy rule
> only if CONFIG_MODULE_SIG_FORCE is not enabled. This, unfortunately, does
> not take into account the ability of enabling "sig_enforce" on the boot
> command line (module.sig_enforce=1).
>
> Including the IMA module appraise rule results in failing the finit_module
> syscall, unless the module signing public key is loaded onto the IMA
> keyring.
>
> This patch fixes secure boot policy rules to be based on CONFIG_MODULE_SIG
> instead.
>
> Fixes: 4238fad366a6 ("powerpc/ima: Add support to initialize ima policy
> rules")
> Signed-off-by: Nayna Jain
Thanks, Nayna.
Signed-off-by: Mimi Zohar
Re: [PATCH v8 04/14] integrity: Introduce struct evm_xattr
On Fri, 2018-11-16 at 18:07 -0200, Thiago Jung Bauermann wrote:
> Even though struct evm_ima_xattr_data includes a fixed-size array to hold a
> SHA1 digest, most of the code ignores the array and uses the struct to mean
> "type indicator followed by data of unspecified size" and tracks the real
> size of what the struct represents in a separate length variable.
>
> The only exception to that is the EVM code, which correctly uses the
> definition of struct evm_ima_xattr_data.
>
> So make this explicit in the code by removing the length specification from
> the array in struct evm_ima_xattr_data. Also, change the name of the
> element from digest to data since in most places the array doesn't hold a
> digest.
>
> A separate struct evm_xattr is introduced, with the original definition of
> evm_ima_xattr_data to be used in the places that actually expect that
> definition.
, specifically the EVM HMAC code.
>
> Signed-off-by: Thiago Jung Bauermann
Other than commenting the evm_xattr usage is limited to HMAC before
the structure definition, this looks good.
Reviewed-by: Mimi Zohar
> ---
> security/integrity/evm/evm_main.c | 8
> security/integrity/ima/ima_appraise.c | 7 ---
> security/integrity/integrity.h| 5 +
> 3 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> index 7f3f54d89a6e..a1b42d10efc7 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -169,7 +169,7 @@ static enum integrity_status evm_verify_hmac(struct
> dentry *dentry,
> /* check value type */
> switch (xattr_data->type) {
> case EVM_XATTR_HMAC:
> - if (xattr_len != sizeof(struct evm_ima_xattr_data)) {
> + if (xattr_len != sizeof(struct evm_xattr)) {
> evm_status = INTEGRITY_FAIL;
> goto out;
> }
> @@ -179,7 +179,7 @@ static enum integrity_status evm_verify_hmac(struct
> dentry *dentry,
> xattr_value_len, &digest);
> if (rc)
> break;
> - rc = crypto_memneq(xattr_data->digest, digest.digest,
> + rc = crypto_memneq(xattr_data->data, digest.digest,
> SHA1_DIGEST_SIZE);
> if (rc)
> rc = -EINVAL;
> @@ -523,7 +523,7 @@ int evm_inode_init_security(struct inode *inode,
>const struct xattr *lsm_xattr,
>struct xattr *evm_xattr)
> {
> - struct evm_ima_xattr_data *xattr_data;
> + struct evm_xattr *xattr_data;
> int rc;
>
> if (!evm_key_loaded() || !evm_protected_xattr(lsm_xattr->name))
> @@ -533,7 +533,7 @@ int evm_inode_init_security(struct inode *inode,
> if (!xattr_data)
> return -ENOMEM;
>
> - xattr_data->type = EVM_XATTR_HMAC;
> + xattr_data->data.type = EVM_XATTR_HMAC;
> rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
> if (rc < 0)
> goto out;
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index deec1804a00a..8bcef90939f8 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -167,7 +167,8 @@ enum hash_algo ima_get_hash_algo(struct
> evm_ima_xattr_data *xattr_value,
> return sig->hash_algo;
> break;
> case IMA_XATTR_DIGEST_NG:
> - ret = xattr_value->digest[0];
> + /* first byte contains algorithm id */
> + ret = xattr_value->data[0];
> if (ret < HASH_ALGO__LAST)
> return ret;
> break;
> @@ -175,7 +176,7 @@ enum hash_algo ima_get_hash_algo(struct
> evm_ima_xattr_data *xattr_value,
> /* this is for backward compatibility */
> if (xattr_len == 21) {
> unsigned int zero = 0;
> - if (!memcmp(&xattr_value->digest[16], &zero, 4))
> + if (!memcmp(&xattr_value->data[16], &zero, 4))
> return HASH_ALGO_MD5;
> else
> return HASH_ALGO_SHA1;
> @@ -274,7 +275,7 @@ int ima_appraise_measurement(enum ima_hooks func,
> /* xattr length may be longer. md5 hash in previous
> version occupied 20 bytes in xattr, instead of 16
>*/
> - rc = memcmp(&
Re: [PATCH v12 00/11] Appended signatures support for IMA appraisal
On Thu, 2019-06-27 at 23:19 -0300, Thiago Jung Bauermann wrote: > Hello, > > This version is essentially identical to the last one. > > It is only a rebase on top of today's linux-integrity/next-queued-testing, > prompted by conflicts with Prakhar Srivastava's patches to measure the > kernel command line. It also drops two patches that are already present in > that branch. Thanks, Thiago. These patches are now in next-queued-testing waiting for some additional reviews/acks. Mimi
Re: [PATCH] tpm: fixes uninitialized allocated banks for IBM vtpm driver
On Wed, 2019-07-03 at 23:32 -0400, Nayna Jain wrote:
> The nr_allocated_banks and allocated banks are initialized as part of
> tpm_chip_register. Currently, this is done as part of auto startup
> function. However, some drivers, like the ibm vtpm driver, do not run
> auto startup during initialization. This results in uninitialized memory
> issue and causes a kernel panic during boot.
>
> This patch moves the pcr allocation outside the auto startup function
> into tpm_chip_register. This ensures that allocated banks are initialized
> in any case.
>
> Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with
> PCR read")
> Signed-off-by: Nayna Jain
Reviewed-by: Mimi Zohar
Re: [PATCH v2] tpm: tpm_ibm_vtpm: Fix unallocated banks
Hi Jarkko,
On Mon, 2019-07-08 at 18:11 +0300, Jarkko Sakkinen wrote:
> On Sat, 2019-07-06 at 20:18 -0400, Nayna Jain wrote:
> > +/*
> > + * tpm_get_pcr_allocation() - initialize the chip allocated banks for PCRs
> > + * @chip: TPM chip to use.
> > + */
> > +static int tpm_get_pcr_allocation(struct tpm_chip *chip)
> > +{
> > + int rc;
> > +
> > + if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > + rc = tpm2_get_pcr_allocation(chip);
> > + else
> > + rc = tpm1_get_pcr_allocation(chip);
> > +
> > + return rc;
> > +}
>
> It is just a trivial static function, which means that kdoc comment is
> not required and neither it is useful. Please remove that. I would
> rewrite the function like:
>
> static int tpm_get_pcr_allocation(struct tpm_chip *chip)
> {
> int rc;
>
> rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
>tpm2_get_pcr_allocation(chip) :
>tpm1_get_pcr_allocation(chip);
>
> return rc > 0 ? -ENODEV : rc;
> }
>
> This addresses the issue that Stefan also pointed out. You have to
> deal with the TPM error codes.
Hm, in the past I was told by Christoph not to use the ternary
operator. Have things changed? Other than removing the comment, the
only other difference is the return.
Mimi
Re: [PATCH v12 01/11] MODSIGN: Export module signature definitions
On Mon, 2019-08-05 at 15:11 +0200, Philipp Rudo wrote: > Hi Thiago, > > > > The patch looks good now. > > > > Thanks! Can I add your Reviewed-by? > > sorry, for the late answer, but I was on vacation the last two weeks. I hope > it's not too late now. > > Reviewed-by: Philipp Rudo Thanks! This patch set is still in the #next-queued-testing branch. I'm still hoping for a few more tags, before pushing it out to the #next-integrity branch later today. Mimi
Re: [PATCH] ima: add a new CONFIG for loading arch-specific policies
On Wed, 2020-02-26 at 11:21 -0800, Lakshmi Ramasubramanian wrote: > Hi Nayna, > > > + > > +config IMA_SECURE_AND_OR_TRUSTED_BOOT > > + bool > > + depends on IMA > > + depends on IMA_ARCH_POLICY > > + default n > > + help > > + This option is selected by architectures to enable secure and/or > > + trusted boot based on IMA runtime policies. > > > > Why is the default for this new config "n"? > Is there any reason to not turn on this config if both IMA and > IMA_ARCH_POLICY are set to y? Good catch. Having "IMA_SECURE_AND_OR_TRUSTED_BOOT" depend on "IMA_ARCH_POLICY" doesn't make sense. "IMA_ARCH_POLICY" needs to be selected. thanks, Mimi
[PATCH] sefltest/ima: support appended signatures (modsig)
Detect and allow appended signatures.
Signed-off-by: Mimi Zohar
---
.../selftests/kexec/test_kexec_file_load.sh| 38 +++---
1 file changed, 34 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/kexec/test_kexec_file_load.sh
b/tools/testing/selftests/kexec/test_kexec_file_load.sh
index fa7c24e8eefb..2ff600388c30 100755
--- a/tools/testing/selftests/kexec/test_kexec_file_load.sh
+++ b/tools/testing/selftests/kexec/test_kexec_file_load.sh
@@ -37,11 +37,20 @@ is_ima_sig_required()
# sequentially. As a result, a policy rule may be defined, but
# might not necessarily be used. This test assumes if a policy
# rule is specified, that is the intent.
+
+ # First check for appended signature (modsig), then xattr
if [ $ima_read_policy -eq 1 ]; then
check_ima_policy "appraise" "func=KEXEC_KERNEL_CHECK" \
- "appraise_type=imasig"
+ "appraise_type=imasig|modsig"
ret=$?
- [ $ret -eq 1 ] && log_info "IMA signature required";
+ if [ $ret -eq 1 ]; then
+ log_info "IMA or appended(modsig) signature required"
+ else
+ check_ima_policy "appraise" "func=KEXEC_KERNEL_CHECK" \
+ "appraise_type=imasig"
+ ret=$?
+ [ $ret -eq 1 ] && log_info "IMA signature required";
+ fi
fi
return $ret
}
@@ -84,6 +93,22 @@ check_for_imasig()
return $ret
}
+# Return 1 for appended signature (modsig) found and 0 for not found.
+check_for_modsig()
+{
+ local module_sig_string="~Module signature appended~"
+ local sig="$(tail --bytes $((${#module_sig_string} + 1)) $KERNEL_IMAGE)"
+ local ret=0
+
+ if [ "$sig" == "$module_sig_string" ]; then
+ ret=1
+ log_info "kexec kernel image modsig signed"
+ else
+ log_info "kexec kernel image not modsig signed"
+ fi
+ return $ret
+}
+
kexec_file_load_test()
{
local succeed_msg="kexec_file_load succeeded"
@@ -98,7 +123,8 @@ kexec_file_load_test()
# In secureboot mode with an architecture specific
# policy, make sure either an IMA or PE signature exists.
if [ $secureboot -eq 1 ] && [ $arch_policy -eq 1 ] && \
- [ $ima_signed -eq 0 ] && [ $pe_signed -eq 0 ]; then
+ [ $ima_signed -eq 0 ] && [ $pe_signed -eq 0 ] \
+ && [ $ima_modsig -eq 0 ]; then
log_fail "$succeed_msg (missing sig)"
fi
@@ -107,7 +133,8 @@ kexec_file_load_test()
log_fail "$succeed_msg (missing PE sig)"
fi
- if [ $ima_sig_required -eq 1 ] && [ $ima_signed -eq 0 ]; then
+ if [ $ima_sig_required -eq 1 ] && [ $ima_signed -eq 0 ] \
+&& [ $ima_modsig -eq 0 ]; then
log_fail "$succeed_msg (missing IMA sig)"
fi
@@ -204,5 +231,8 @@ pe_signed=$?
check_for_imasig
ima_signed=$?
+check_for_modsig
+ima_modsig=$?
+
# Test loading the kernel image via kexec_file_load syscall
kexec_file_load_test
--
2.7.5
Re: [PATCH v12 00/11] Appended signatures support for IMA appraisal
Hi Jordan, On Mon, 2019-08-26 at 15:46 -0700, Jordan Hand wrote: > On 6/27/19 7:19 PM, Thiago Jung Bauermann wrote: > > On the OpenPOWER platform, secure boot and trusted boot are being > > implemented using IMA for taking measurements and verifying signatures. > > Since the kernel image on Power servers is an ELF binary, kernels are > > signed using the scripts/sign-file tool and thus use the same signature > > format as signed kernel modules. > > > > This patch series adds support in IMA for verifying those signatures. > > It adds flexibility to OpenPOWER secure boot, because it allows it to boot > > kernels with the signature appended to them as well as kernels where the > > signature is stored in the IMA extended attribute. > > I know this is pretty late, but I just wanted to let you know that I > tested this patch set on x86_64 with QEMU. > > That is, I enrolled a key to _ima keyring, signed my kernel and modules > with appended signatures (with scripts/sign-file), set the IMA policy to > appraise and measure my kernel and modules. Also tested kexec appraisal. > > You can add my tested-by if you'd like. I really appreciate your testing. Based on the recent Documentation/maintainer/rebasing-and-merging.rst, I'm trying not to rebase patches already staged in linux-next. Patches are first being staged in the next-queued-testing branch. FYI, I just posted a patch that adds IMA appended signature support to test_kexec_file_load.sh. thanks, Mimi
[PATCH v1] sefltest/ima: support appended signatures (modsig)
In addition to the PE/COFF and IMA xattr signatures, the kexec kernel
image can be signed with an appended signature, using the same
scripts/sign-file tool that is used to sign kernel modules.
This patch adds support for detecting a kernel image signed with an
appended signature and updates the existing test messages
appropriately.
Reviewed-by: Petr Vorel
Signed-off-by: Mimi Zohar
---
.../selftests/kexec/test_kexec_file_load.sh| 38 +++---
1 file changed, 34 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/kexec/test_kexec_file_load.sh
b/tools/testing/selftests/kexec/test_kexec_file_load.sh
index fa7c24e8eefb..2ff600388c30 100755
--- a/tools/testing/selftests/kexec/test_kexec_file_load.sh
+++ b/tools/testing/selftests/kexec/test_kexec_file_load.sh
@@ -37,11 +37,20 @@ is_ima_sig_required()
# sequentially. As a result, a policy rule may be defined, but
# might not necessarily be used. This test assumes if a policy
# rule is specified, that is the intent.
+
+ # First check for appended signature (modsig), then xattr
if [ $ima_read_policy -eq 1 ]; then
check_ima_policy "appraise" "func=KEXEC_KERNEL_CHECK" \
- "appraise_type=imasig"
+ "appraise_type=imasig|modsig"
ret=$?
- [ $ret -eq 1 ] && log_info "IMA signature required";
+ if [ $ret -eq 1 ]; then
+ log_info "IMA or appended(modsig) signature required"
+ else
+ check_ima_policy "appraise" "func=KEXEC_KERNEL_CHECK" \
+ "appraise_type=imasig"
+ ret=$?
+ [ $ret -eq 1 ] && log_info "IMA signature required";
+ fi
fi
return $ret
}
@@ -84,6 +93,22 @@ check_for_imasig()
return $ret
}
+# Return 1 for appended signature (modsig) found and 0 for not found.
+check_for_modsig()
+{
+ local module_sig_string="~Module signature appended~"
+ local sig="$(tail --bytes $((${#module_sig_string} + 1)) $KERNEL_IMAGE)"
+ local ret=0
+
+ if [ "$sig" == "$module_sig_string" ]; then
+ ret=1
+ log_info "kexec kernel image modsig signed"
+ else
+ log_info "kexec kernel image not modsig signed"
+ fi
+ return $ret
+}
+
kexec_file_load_test()
{
local succeed_msg="kexec_file_load succeeded"
@@ -98,7 +123,8 @@ kexec_file_load_test()
# In secureboot mode with an architecture specific
# policy, make sure either an IMA or PE signature exists.
if [ $secureboot -eq 1 ] && [ $arch_policy -eq 1 ] && \
- [ $ima_signed -eq 0 ] && [ $pe_signed -eq 0 ]; then
+ [ $ima_signed -eq 0 ] && [ $pe_signed -eq 0 ] \
+ && [ $ima_modsig -eq 0 ]; then
log_fail "$succeed_msg (missing sig)"
fi
@@ -107,7 +133,8 @@ kexec_file_load_test()
log_fail "$succeed_msg (missing PE sig)"
fi
- if [ $ima_sig_required -eq 1 ] && [ $ima_signed -eq 0 ]; then
+ if [ $ima_sig_required -eq 1 ] && [ $ima_signed -eq 0 ] \
+&& [ $ima_modsig -eq 0 ]; then
log_fail "$succeed_msg (missing IMA sig)"
fi
@@ -204,5 +231,8 @@ pe_signed=$?
check_for_imasig
ima_signed=$?
+check_for_modsig
+ima_modsig=$?
+
# Test loading the kernel image via kexec_file_load syscall
kexec_file_load_test
--
2.7.5
Re: [PATCH] sefltest/ima: support appended signatures (modsig)
On Wed, 2019-08-28 at 08:45 -0600, shuah wrote: > Hi Mimi, > > On 8/28/19 6:39 AM, Mimi Zohar wrote: > > Detect and allow appended signatures. > > > > Can you please add a couple of more sentences on the feature > and what happens without it? I know this is a test for the > feature, however, it will be useful for users and testers to > know more about this test and the feature it is testing. I've updated the patch description as requested. > Also, are there test skip conditions to be concerned about? The kexec selftests tests the coordination of the different methods of verifying the kexec kernel image. As the appended signature support is part of IMA, there is no new skip conditions. > Is there a dependency on another tree or would like me to take > this through kselftest tree? I would prefer upstreaming this test with the rest of IMA support for appended signatures. thanks, Mimi
Re: [PATCH v1] sefltest/ima: support appended signatures (modsig)
On Wed, 2019-08-28 at 09:53 -0600, shuah wrote: > On 8/28/19 9:14 AM, Mimi Zohar wrote: > > In addition to the PE/COFF and IMA xattr signatures, the kexec kernel > > image can be signed with an appended signature, using the same > > scripts/sign-file tool that is used to sign kernel modules. > > > > This patch adds support for detecting a kernel image signed with an > > appended signature and updates the existing test messages > > appropriately. > > > > Reviewed-by: Petr Vorel > > Signed-off-by: Mimi Zohar > > --- > > Thanks Mimi. This commit log looks good. My Ack for the patch > to go through the IMA tree. > > Acked-by: Shuah Khan Thanks! Mimi
Re: [PATCH v1] sefltest/ima: support appended signatures (modsig)
On Wed, 2019-08-28 at 20:38 -0300, Thiago Jung Bauermann wrote: > Hello Mimi, > > Mimi Zohar writes: > > > In addition to the PE/COFF and IMA xattr signatures, the kexec kernel > > image can be signed with an appended signature, using the same > > scripts/sign-file tool that is used to sign kernel modules. > > > > This patch adds support for detecting a kernel image signed with an > > appended signature and updates the existing test messages > > appropriately. > > > > Reviewed-by: Petr Vorel > > Signed-off-by: Mimi Zohar > > Thanks for doing this! You're welcome. This isn't in lieu of a proper regression test that verifies the IMA measurement list template modsig and d-modsig data fields. That still needs to be written. thanks, Mimi
Re: [PATCH v3 3/4] x86/efi: move common keyring handler functions to new file
(Cc'ing Josh Boyer, David Howells) On Mon, 2019-09-02 at 21:55 +1000, Michael Ellerman wrote: > Nayna Jain writes: > > > The handlers to add the keys to the .platform keyring and blacklisted > > hashes to the .blacklist keyring is common for both the uefi and powerpc > > mechanisms of loading the keys/hashes from the firmware. > > > > This patch moves the common code from load_uefi.c to keyring_handler.c > > > > Signed-off-by: Nayna Jain Acked-by: Mimi Zohar > > --- > > security/integrity/Makefile | 3 +- > > .../platform_certs/keyring_handler.c | 80 +++ > > .../platform_certs/keyring_handler.h | 32 > > security/integrity/platform_certs/load_uefi.c | 67 +--- > > 4 files changed, 115 insertions(+), 67 deletions(-) > > create mode 100644 security/integrity/platform_certs/keyring_handler.c > > create mode 100644 security/integrity/platform_certs/keyring_handler.h > > This has no acks from security folks, though I'm not really clear on who > maintains those files. I upstreamed David's, Josh's, and Nayna's patches, so that's probably me. > Do I take it because it's mostly just code movement people are OK with > it going in via the powerpc tree? Yes, the only reason for splitting load_uefi.c is for powerpc. These patches should be upstreamed together. Mimi
Re: [PATCH v3 4/4] powerpc: load firmware trusted keys/hashes into kernel keyring
On Mon, 2019-08-26 at 09:23 -0400, Nayna Jain wrote:
> The keys used to verify the Host OS kernel are managed by firmware as
> secure variables. This patch loads the verification keys into the .platform
> keyring and revocation hashes into .blacklist keyring. This enables
> verification and loading of the kernels signed by the boot time keys which
> are trusted by firmware.
>
> Signed-off-by: Nayna Jain
Feel free to add my tag after addressing the formatting issues.
Reviewed-by: Mimi Zohar
> diff --git a/security/integrity/platform_certs/load_powerpc.c
> b/security/integrity/platform_certs/load_powerpc.c
> new file mode 100644
> index ..359d5063d4da
> --- /dev/null
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain
> + *
> + * - loads keys and hashes stored and controlled by the firmware.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "keyring_handler.h"
> +
> +/*
> + * Get a certificate list blob from the named secure variable.
> + */
> +static __init void *get_cert_list(u8 *key, unsigned long keylen, uint64_t
> *size)
> +{
> + int rc;
> + void *db;
> +
> + rc = secvar_ops->get(key, keylen, NULL, size);
> + if (rc) {
> + pr_err("Couldn't get size: %d\n", rc);
> + return NULL;
> + }
> +
> + db = kmalloc(*size, GFP_KERNEL);
> + if (!db)
> + return NULL;
> +
> + rc = secvar_ops->get(key, keylen, db, size);
> + if (rc) {
> + kfree(db);
> + pr_err("Error reading db var: %d\n", rc);
> + return NULL;
> + }
> +
> + return db;
> +}
> +
> +/*
> + * Load the certs contained in the keys databases into the platform trusted
> + * keyring and the blacklisted X.509 cert SHA256 hashes into the blacklist
> + * keyring.
> + */
> +static int __init load_powerpc_certs(void)
> +{
> + void *db = NULL, *dbx = NULL;
> + uint64_t dbsize = 0, dbxsize = 0;
> + int rc = 0;
> +
> + if (!secvar_ops)
> + return -ENODEV;
> +
> + /* Get db, and dbx. They might not exist, so it isn't
> + * an error if we can't get them.
> + */
> + db = get_cert_list("db", 3, &dbsize);
> + if (!db) {
> + pr_err("Couldn't get db list from firmware\n");
> + } else {
> + rc = parse_efi_signature_list("powerpc:db",
> + db, dbsize, get_handler_for_db);
> + if (rc)
> + pr_err("Couldn't parse db signatures: %d\n",
> + rc);
There's no need to split this line.
> + kfree(db);
> + }
> +
> + dbx = get_cert_list("dbx", 3, &dbxsize);
> + if (!dbx) {
> + pr_info("Couldn't get dbx list from firmware\n");
> + } else {
> + rc = parse_efi_signature_list("powerpc:dbx",
> + dbx, dbxsize,
> + get_handler_for_dbx);
Formatting of this line is off.
> + if (rc)
> + pr_err("Couldn't parse dbx signatures: %d\n", rc);
> + kfree(dbx);
> + }
> +
> + return rc;
> +}
> +late_initcall(load_powerpc_certs);
Re: [PATCH v6 5/9] powerpc/ima: add measurement rules to ima arch specific policy
On Fri, 2019-09-27 at 10:25 -0400, Nayna Jain wrote:
> This patch adds the measurement rules to the arch specific policies for the
> systems with trusted boot.
>
on trusted boot enabled systems.
> Signed-off-by: Nayna Jain
Minor comment correction below.
Reviewed-by: Mimi Zohar
> ---
> arch/powerpc/kernel/ima_arch.c | 44 +++---
> 1 file changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
> index 39401b67f19e..77c61b142042 100644
> --- a/arch/powerpc/kernel/ima_arch.c
> +++ b/arch/powerpc/kernel/ima_arch.c
> @@ -12,8 +12,18 @@ 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 in the IMA measurement list.
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)
> "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> @@ -22,12 +32,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",
> + 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++;
> + }
> +
> + if (is_powerpc_os_secureboot_enabled()
> + && is_powerpc_trustedboot_enabled())
> return arch_rules;
>
> + if (is_powerpc_os_secureboot_enabled())
> + return arch_rules + offset;
> +
> + if (is_powerpc_trustedboot_enabled())
> + return measure_rules;
> +
> return NULL;
> }
Re: [PATCH v6 7/9] ima: check against blacklisted hashes for files with modsig
On Fri, 2019-09-27 at 10:25 -0400, Nayna Jain wrote:
> Asymmetric private keys are used to sign multiple files. The kernel
> currently support checking against the blacklisted keys. However, if the
> public key is blacklisted, any file signed by the blacklisted key will
> automatically fail signature verification. We might not want to blacklist
> all the files signed by a particular key, but just a single file.
> Blacklisting the public key is not fine enough granularity.
>
> This patch adds support for blacklisting binaries with appended signatures,
> based on the IMA policy. Defined is a new policy option
> "appraise_flag=check_blacklist".
>
> Signed-off-by: Nayna Jain
> ---
> Documentation/ABI/testing/ima_policy | 1 +
> security/integrity/ima/ima.h | 12 +
> security/integrity/ima/ima_appraise.c | 35 +++
> security/integrity/ima/ima_main.c | 8 --
> security/integrity/ima/ima_policy.c | 10 ++--
> security/integrity/integrity.h| 1 +
> 6 files changed, 63 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy
> b/Documentation/ABI/testing/ima_policy
> index 29ebe9afdac4..4c97afcc0f3c 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -25,6 +25,7 @@ Description:
> lsm:[[subj_user=] [subj_role=] [subj_type=]
>[obj_user=] [obj_role=] [obj_type=]]
> option: [[appraise_type=]] [template=] [permit_directio]
> + [appraise_flag=[check_blacklist]]
> base: func:=
> [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
> [FIRMWARE_CHECK]
> [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 9bf509217e8e..2c034728b239 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -254,6 +254,9 @@ int ima_policy_show(struct seq_file *m, void *v);
> #define IMA_APPRAISE_KEXEC 0x40
>
> #ifdef CONFIG_IMA_APPRAISE
> +int ima_blacklist_measurement(struct integrity_iint_cache *iint,
> + const struct modsig *modsig, int action,
> + int pcr, struct ima_template_desc *template_desc);
> int ima_appraise_measurement(enum ima_hooks func,
>struct integrity_iint_cache *iint,
>struct file *file, const unsigned char *filename,
> @@ -269,6 +272,15 @@ int ima_read_xattr(struct dentry *dentry,
> struct evm_ima_xattr_data **xattr_value);
>
> #else
> +static inline int ima_blacklist_measurement(struct integrity_iint_cache
> *iint,
> + const struct modsig *modsig,
> + int action, int pcr,
> + struct ima_template_desc
> + *template_desc)
> +{
> + return 0;
> +}
> +
> static inline int ima_appraise_measurement(enum ima_hooks func,
> struct integrity_iint_cache *iint,
> struct file *file,
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 136ae4e0ee92..a5a82e870e24 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
>
> #include "ima.h"
>
> @@ -303,6 +304,40 @@ static int modsig_verify(enum ima_hooks func, const
> struct modsig *modsig,
> return rc;
> }
>
> +/*
> + * ima_blacklist_measurement - checks if the file measurement is blacklisted
> + *
> + * Returns -EKEYREJECTED if the hash is blacklisted.
> + */
In the function comment, please mention that blacklisted binaries are
included in the IMA measurement list.
> +int ima_blacklist_measurement(struct integrity_iint_cache *iint,
> + const struct modsig *modsig, int action, int pcr,
> + struct ima_template_desc *template_desc)
> +{
> + enum hash_algo hash_algo;
> + const u8 *digest = NULL;
> + u32 digestsize = 0;
> + u32 secid;
> + int rc = 0;
> +
> + if (!(iint->flags & IMA_CHECK_BLACKLIST))
> + return 0;
> +
> + if (iint->flags & IMA_MODSIG_ALLOWED) {
> + security_task_getsecid(current, &secid);
> + ima_get_modsig_digest(modsig, &hash_algo, &digest, &digestsize);
> +
> + rc = is_hash_blacklisted(digest, digestsize, "bin");
> +
> + /* Returns -EKEYREJECTED on blacklisted hash found */
is_hash_blacklisted() returning -EKEYREJECTED makes sense if the key
is blacklisted, not so much for a binary. It would make more sense to
define is_binary_blacklis
Re: [PATCH v6 8/9] ima: deprecate permit_directio, instead use appraise_flag
Hi Nayna, On Fri, 2019-09-27 at 10:25 -0400, Nayna Jain wrote: > This patch deprecates the existing permit_directio flag, instead adds > it as possible value to appraise_flag parameter. > For eg. > appraise_flag=permit_directio Defining a generic "appraise_flag=", which supports different options, is the right direction. I would really like to depreciate the "permit_directio" flag, not just change the policy syntax. For now, let's drop this change. Mimi
Re: [PATCH v6 3/9] powerpc: add support to initialize ima policy rules
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 ..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
> >> +#include
> >> +
> >> +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
Re: [PATCH v6 6/9] ima: make process_buffer_measurement() non static
[Cc'ing Prakhar] On Fri, 2019-09-27 at 10:25 -0400, Nayna Jain wrote: > To add the support for checking against blacklist, it would be needed > to add an additional measurement record that identifies the record > as blacklisted. > > This patch modifies the process_buffer_measurement() and makes it > non static to be used by blacklist functionality. It modifies the > function to handle more than just the KEXEC_CMDLINE. > > Signed-off-by: Nayna Jain Making process_buffer_measurement() non static is the end result, not the reason for the change. The reason for changing process_buffer_measurement() is to make it more generic. The blacklist measurement record is the usecase. Please rewrite the patch description. thanks, Mimi
Re: [PATCH v7 2/8] powerpc: add support to initialize ima policy rules
On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote: > PowerNV systems uses kernel based bootloader, thus its secure boot > implementation uses kernel IMA security subsystem to verify the kernel > before kexec. ^use a Linux based bootloader, which rely on the IMA subsystem to enforce different secure boot modes. > Since the verification policy might differ based on the > secure boot mode of the system, the policies are defined at runtime. ^the policies need to be 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 > --- > 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 b4a221886fcf..deb19ec6ba3d 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -938,6 +938,8 @@ config PPC_SECURE_BOOT > prompt "Enable secure boot support" > bool > depends on PPC_POWERNV > + depends on IMA > + depends on IMA_ARCH_POLICY As IMA_ARCH_POLICY is dependent on IMA, I don't see a need for depending on both IMA and IMA_ARCH_POLICY. Mimi
Re: [PATCH v7 5/8] ima: make process_buffer_measurement() generic
[Cc'ing Prakhar Srivastava]
On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
> An additional measurement record is needed to indicate the blacklisted
> binary. The record will measure the blacklisted binary hash.
>
> This patch makes the function process_buffer_measurement() generic to be
> called by the blacklisting function. It modifies the function to handle
> more than just the KEXEC_CMDLINE.
The purpose of this patch is to make process_buffer_measurement() more
generic. The patch description should simply say,
process_buffer_measurement() is limited to measuring the kexec boot
command line. This patch makes process_buffer_measurement() more
generic, allowing it to measure other types of buffer data (eg.
blacklisted binary hashes).
Mimi
>
> Signed-off-by: Nayna Jain
> ---
> security/integrity/ima/ima.h | 3 +++
> security/integrity/ima/ima_main.c | 29 ++---
> 2 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 3689081aaf38..ed86c1f70d7f 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -217,6 +217,9 @@ void ima_store_measurement(struct integrity_iint_cache
> *iint, struct file *file,
> struct evm_ima_xattr_data *xattr_value,
> int xattr_len, const struct modsig *modsig, int pcr,
> struct ima_template_desc *template_desc);
> +void process_buffer_measurement(const void *buf, int size,
> + const char *eventname, int pcr,
> + struct ima_template_desc *template_desc);
> void ima_audit_measurement(struct integrity_iint_cache *iint,
> const unsigned char *filename);
> int ima_alloc_init_template(struct ima_event_data *event_data,
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 60027c643ecd..77115e884496 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -626,14 +626,14 @@ int ima_load_data(enum kernel_load_data_id id)
> * @buf: pointer to the buffer that needs to be added to the log.
> * @size: size of buffer(in bytes).
> * @eventname: event name to be used for the buffer entry.
> - * @cred: a pointer to a credentials structure for user validation.
> - * @secid: the secid of the task to be validated.
> + * @pcr: pcr to extend the measurement
> + * @template_desc: template description
> *
> * Based on policy, the buffer is measured into the ima log.
> */
> -static void process_buffer_measurement(const void *buf, int size,
> -const char *eventname,
> -const struct cred *cred, u32 secid)
> +void process_buffer_measurement(const void *buf, int size,
> + const char *eventname, int pcr,
> + struct ima_template_desc *template_desc)
> {
> int ret = 0;
> struct ima_template_entry *entry = NULL;
> @@ -642,19 +642,11 @@ static void process_buffer_measurement(const void *buf,
> int size,
> .filename = eventname,
> .buf = buf,
> .buf_len = size};
> - struct ima_template_desc *template_desc = NULL;
> struct {
> struct ima_digest_data hdr;
> char digest[IMA_MAX_DIGEST_SIZE];
> } hash = {};
> int violation = 0;
> - int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> - int action = 0;
> -
> - action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr,
> - &template_desc);
> - if (!(action & IMA_MEASURE))
> - return;
>
> iint.ima_hash = &hash.hdr;
> iint.ima_hash->algo = ima_hash_algo;
> @@ -686,12 +678,19 @@ static void process_buffer_measurement(const void *buf,
> int size,
> */
> void ima_kexec_cmdline(const void *buf, int size)
> {
> + int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> + struct ima_template_desc *template_desc = NULL;
> + int action;
> u32 secid;
>
> if (buf && size != 0) {
> security_task_getsecid(current, &secid);
> - process_buffer_measurement(buf, size, "kexec-cmdline",
> -current_cred(), secid);
> + action = ima_get_action(NULL, current_cred(), secid, 0,
> + KEXEC_CMDLINE, &pcr, &template_desc);
> + if (!(action & IMA_MEASURE))
> + return;
> + process_buffer_measurement(buf, size, "kexec-cmdline", pcr,
> +template_desc);
> }
> }
>
Re: [PATCH v7 6/8] certs: add wrapper function to check blacklisted binary hash
On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
> The existing is_hash_blacklisted() function returns -EKEYREJECTED
> error code for both the blacklisted keys and binaries.
>
> This patch adds a wrapper function is_binary_blacklisted() to check
> against binary hashes and returns -EPERM.
>
> Signed-off-by: Nayna Jain
This patch description describes what you're doing, not the
motivation.
Reviewed-by: Mimi Zohar
> ---
> certs/blacklist.c | 9 +
> include/keys/system_keyring.h | 6 ++
> 2 files changed, 15 insertions(+)
>
> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index ec00bf337eb6..6514f9ebc943 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -135,6 +135,15 @@ int is_hash_blacklisted(const u8 *hash, size_t hash_len,
> const char *type)
> }
> EXPORT_SYMBOL_GPL(is_hash_blacklisted);
>
> +int is_binary_blacklisted(const u8 *hash, size_t hash_len)
> +{
> + if (is_hash_blacklisted(hash, hash_len, "bin") == -EKEYREJECTED)
> + return -EPERM;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(is_binary_blacklisted);
> +
> /*
> * Initialise the blacklist
> */
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index c1a96fdf598b..fb8b07daa9d1 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -35,12 +35,18 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
> extern int mark_hash_blacklisted(const char *hash);
> extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
> const char *type);
> +extern int is_binary_blacklisted(const u8 *hash, size_t hash_len);
> #else
> static inline int is_hash_blacklisted(const u8 *hash, size_t hash_len,
> const char *type)
> {
> return 0;
> }
> +
> +static inline int is_binary_blacklisted(const u8 *hash, size_t hash_len)
> +{
> + return 0;
> +}
> #endif
>
> #ifdef CONFIG_IMA_BLACKLIST_KEYRING
Re: [PATCH v7 8/8] powerpc/ima: update ima arch policy to check for blacklist
On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
> This patch updates the arch specific policies for PowernV systems
> to add check against blacklisted binary hashes before doing the
> verification.
This sentence explains how you're doing something. A simple tweak in
the wording provides the motivation.
^to make sure that the binary hash is not blacklisted.
>
> Signed-off-by: Nayna Jain
Reviewed-by: Mimi Zohar
> ---
> arch/powerpc/kernel/ima_arch.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
> index 88bfe4a1a9a5..4fa41537b846 100644
> --- a/arch/powerpc/kernel/ima_arch.c
> +++ b/arch/powerpc/kernel/ima_arch.c
> @@ -25,9 +25,9 @@ bool arch_ima_get_secureboot(void)
> 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",
> + "appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist
> appraise_type=imasig|modsig",
> #if !IS_ENABLED(CONFIG_MODULE_SIG_FORCE)
> - "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> + "appraise func=MODULE_CHECK appraise_flag=check_blacklist
> appraise_type=imasig|modsig",
> #endif
> NULL
> };
Re: [PATCH v7 7/8] ima: check against blacklisted hashes for files with modsig
On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
> Asymmetric private keys are used to sign multiple files. The kernel
> currently support checking against the blacklisted keys. However, if the
> public key is blacklisted, any file signed by the blacklisted key will
> automatically fail signature verification. We might not want to blacklist
> all the files signed by a particular key, but just a single file.
> Blacklisting the public key is not fine enough granularity.
>
> This patch adds support for blacklisting binaries with appended signatures,
> based on the IMA policy. Defined is a new policy option
> "appraise_flag=check_blacklist".
The blacklisted hash is not the same as the file hash, but is the file
hash without the appended signature. Are there tools for calculating
the blacklisted hash? Can you provide an example?
>
> Signed-off-by: Nayna Jain
> ---
> Documentation/ABI/testing/ima_policy | 1 +
> security/integrity/ima/ima.h | 9 +++
> security/integrity/ima/ima_appraise.c | 39 +++
> security/integrity/ima/ima_main.c | 12 ++---
> security/integrity/ima/ima_policy.c | 10 +--
> security/integrity/integrity.h| 1 +
> 6 files changed, 66 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy
> b/Documentation/ABI/testing/ima_policy
> index 29ebe9afdac4..4c97afcc0f3c 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -25,6 +25,7 @@ Description:
> lsm:[[subj_user=] [subj_role=] [subj_type=]
>[obj_user=] [obj_role=] [obj_type=]]
> option: [[appraise_type=]] [template=] [permit_directio]
> + [appraise_flag=[check_blacklist]]
> base: func:=
> [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
> [FIRMWARE_CHECK]
> [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index ed86c1f70d7f..63e20ccc91ce 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -256,6 +256,8 @@ int ima_policy_show(struct seq_file *m, void *v);
> #define IMA_APPRAISE_KEXEC 0x40
>
> #ifdef CONFIG_IMA_APPRAISE
> +int ima_check_blacklist(struct integrity_iint_cache *iint,
> + const struct modsig *modsig, int action, int pcr);
> int ima_appraise_measurement(enum ima_hooks func,
>struct integrity_iint_cache *iint,
>struct file *file, const unsigned char *filename,
> @@ -271,6 +273,13 @@ int ima_read_xattr(struct dentry *dentry,
> struct evm_ima_xattr_data **xattr_value);
>
> #else
> +static inline int ima_check_blacklist(struct integrity_iint_cache *iint,
> + const struct modsig *modsig, int action,
> + int pcr)
> +{
> + return 0;
> +}
> +
> static inline int ima_appraise_measurement(enum ima_hooks func,
> struct integrity_iint_cache *iint,
> struct file *file,
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 136ae4e0ee92..fe34d64a684c 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
>
> #include "ima.h"
>
> @@ -303,6 +304,44 @@ static int modsig_verify(enum ima_hooks func, const
> struct modsig *modsig,
> return rc;
> }
>
> +/*
> + * ima_blacklist_measurement - Checks whether the binary is blacklisted. If
> + * yes, then adds the hash of the blacklisted binary to the measurement list.
> + *
> + * Returns -EPERM if the hash is blacklisted.
> + */
> +int ima_check_blacklist(struct integrity_iint_cache *iint,
> + const struct modsig *modsig, int action, int pcr)
> +{
> + enum hash_algo hash_algo;
> + const u8 *digest = NULL;
> + u32 digestsize = 0;
> + u32 secid;
> + int rc = 0;
> + struct ima_template_desc *template_desc;
> +
> + template_desc = lookup_template_desc("ima-buf");
> + template_desc_init_fields(template_desc->fmt, &(template_desc->fields),
> + &(template_desc->num_fields));
Before using template_desc, make sure that template_desc isn't NULL.
For completeness, check the return code of
template_desc_init_fields()
> +
> + if (!(iint->flags & IMA_CHECK_BLACKLIST))
> + return 0;
Move this check before getting the template_desc and make sure that
modsig isn't NULL.
> +
> + if (iint->flags & IMA_MODSIG_ALLOWED) {
> + security_task_getsecid(current, &secid);
secid isn't being used.
> + ima_get_modsig_digest(mods
Re: [PATCH] integrity: powerpc: Do not select CA_MACHINE_KEYRING
On Tue, 2023-09-12 at 12:49 +0300, Jarkko Sakkinen wrote: > On Tue Sep 12, 2023 at 10:41 AM EEST, Michal Suchánek wrote: > > On Mon, Sep 11, 2023 at 11:39:38PM -0400, Nayna wrote: > > > > > > On 9/7/23 13:32, Michal Suchánek wrote: > > > > Adding more CC's from the original patch, looks like get_maintainers is > > > > not that great for this file. > > > > > > > > On Thu, Sep 07, 2023 at 06:52:19PM +0200, Michal Suchanek wrote: > > > > > No other platform needs CA_MACHINE_KEYRING, either. > > > > > > > > > > This is policy that should be decided by the administrator, not > > > > > Kconfig > > > > > dependencies. > > > > > > We certainly agree that flexibility is important. However, in this case, > > > this also implies that we are expecting system admins to be security > > > experts. As per our understanding, CA based infrastructure(PKI) is the > > > standard to be followed and not the policy decision. And we can only speak > > > for Power. > > > > > > INTEGRITY_CA_MACHINE_KEYRING ensures that we always have CA signed leaf > > > certs. > > > > And that's the problem. > > > > From a distribution point of view there are two types of leaf certs: > > > > - leaf certs signed by the distribution CA which need not be imported > >because the distribution CA cert is enrolled one way or another > > - user generated ad-hoc certificates that are not signed in any way, > >and enrolled by the user > > > > The latter are vouched for by the user by enrolling the certificate, and > > confirming that they really want to trust this certificate. Enrolling > > user certificates is vital for usability or secure boot. Adding extra > > step of creating a CA certificate stored on the same system only > > complicates things with no added benefit. > > This all comes down to the generic fact that kernel should not > proactively define what it *expects* sysadmins. > > CA based infrastructure like anything is a policy decision not > a decision to be enforced by kernel. Secure boot requires a signature chain of trust. IMA extends the secure and trusted boot concepts to the kernel. Missing from that signature chain of trust is the ability of allowing the end machine/system owner to load other certificates without recompiling the kernel. The introduction of the machine keyring was to address this. I'm not questioning the end user's intent on loading local or third party keys via the normal mechanisms. If the existing mechanism(s) for loading local or third party keys were full-proof, then loading a single certificate, self-signed or not, would be fine. However, that isn't the reality. The security of the two-stage approach is simply not equivalent to loading a single certificate. Documentation could help the end user/system owner to safely create (and manage) separate certificate signing and code signing certs. Unlike UEFI based systems, PowerVM defines two variables trustedcadb and moduledb, for storing certificate signing and code signing certificates respectively. First the certs on the trustedcadb are loaded and then the ones on moduledb are loaded. -- thanks, Mimi
Re: [PATCH] integrity: powerpc: Do not select CA_MACHINE_KEYRING
On Tue, 2023-09-12 at 22:32 +0300, Jarkko Sakkinen wrote: > On Tue Sep 12, 2023 at 10:22 PM EEST, Mimi Zohar wrote: > > On Tue, 2023-09-12 at 12:49 +0300, Jarkko Sakkinen wrote: > > > On Tue Sep 12, 2023 at 10:41 AM EEST, Michal Suchánek wrote: > > > > On Mon, Sep 11, 2023 at 11:39:38PM -0400, Nayna wrote: > > > > > > > > > > On 9/7/23 13:32, Michal Suchánek wrote: > > > > > > Adding more CC's from the original patch, looks like > > > > > > get_maintainers is > > > > > > not that great for this file. > > > > > > > > > > > > On Thu, Sep 07, 2023 at 06:52:19PM +0200, Michal Suchanek wrote: > > > > > > > No other platform needs CA_MACHINE_KEYRING, either. > > > > > > > > > > > > > > This is policy that should be decided by the administrator, not > > > > > > > Kconfig > > > > > > > dependencies. > > > > > > > > > > We certainly agree that flexibility is important. However, in this > > > > > case, > > > > > this also implies that we are expecting system admins to be security > > > > > experts. As per our understanding, CA based infrastructure(PKI) is the > > > > > standard to be followed and not the policy decision. And we can only > > > > > speak > > > > > for Power. > > > > > > > > > > INTEGRITY_CA_MACHINE_KEYRING ensures that we always have CA signed > > > > > leaf > > > > > certs. > > > > > > > > And that's the problem. > > > > > > > > From a distribution point of view there are two types of leaf certs: > > > > > > > > - leaf certs signed by the distribution CA which need not be imported > > > >because the distribution CA cert is enrolled one way or another > > > > - user generated ad-hoc certificates that are not signed in any way, > > > >and enrolled by the user > > > > > > > > The latter are vouched for by the user by enrolling the certificate, and > > > > confirming that they really want to trust this certificate. Enrolling > > > > user certificates is vital for usability or secure boot. Adding extra > > > > step of creating a CA certificate stored on the same system only > > > > complicates things with no added benefit. > > > > > > This all comes down to the generic fact that kernel should not > > > proactively define what it *expects* sysadmins. > > > > > > CA based infrastructure like anything is a policy decision not > > > a decision to be enforced by kernel. > > > > Secure boot requires a signature chain of trust. IMA extends the > > secure and trusted boot concepts to the kernel. Missing from that > > signature chain of trust is the ability of allowing the end > > machine/system owner to load other certificates without recompiling the > > kernel. The introduction of the machine keyring was to address this. > > > > I'm not questioning the end user's intent on loading local or third > > party keys via the normal mechanisms. If the existing mechanism(s) for > > loading local or third party keys were full-proof, then loading a > > single certificate, self-signed or not, would be fine. However, that > > isn't the reality. The security of the two-stage approach is simply > > not equivalent to loading a single certificate. Documentation could > > help the end user/system owner to safely create (and manage) separate > > certificate signing and code signing certs. > > > > Unlike UEFI based systems, PowerVM defines two variables trustedcadb > > and moduledb, for storing certificate signing and code signing > > certificates respectively. First the certs on the trustedcadb are > > loaded and then the ones on moduledb are loaded. > > There's pragmatic reasons to make things more open than they should be > in production. As a hardware example I still possess Raspberry Pi 3B for > test workloads because it has a broken TZ implementation. The world is > really bigger than production workloads. > > It would be better to document what you said rather than enforce the > right choice IMHO (e.g. extend Kconfig documentation). PowerVM LPARs are more about production workloads than a Raspberry Pi. :) -- thanks, Mimi
Re: [PATCH 0/6] Enable loading local and third party keys on PowerVM guest
On Fri, 2023-07-14 at 11:34 -0400, Nayna Jain wrote: > On a secure boot enabled PowerVM guest, local and third party code signing > keys are needed to verify signed applications, configuration files, and > kernel modules. > > Loading these keys onto either the .secondary_trusted_keys or .ima > keyrings requires the certificates be signed by keys on the > .builtin_trusted_keys, .machine or .secondary_trusted_keys keyrings. > > Keys on the .builtin_trusted_keys keyring are trusted because of the chain > of trust from secure boot up to and including the linux kernel. Keys on > the .machine keyring that derive their trust from an entity such as a > security officer, administrator, system owner, or machine owner are said > to have "imputed trust." The type of certificates and the mechanism for > loading them onto the .machine keyring is platform dependent. > > Userspace may load certificates onto the .secondary_trusted_keys or .ima > keyrings. However, keys may also need to be loaded by the kernel if they > are needed for verification in early boot time. On PowerVM guest, third > party code signing keys are loaded from the moduledb variable in the > Platform KeyStore(PKS) onto the .secondary_trusted_keys. Thanks, Nayna. I've reviewed and done some initially testing up to 5/6. Mimi
Re: [PATCH 1/6] integrity: PowerVM support for loading CA keys on machine keyring
On Fri, 2023-07-14 at 11:34 -0400, Nayna Jain wrote: > Keys that derive their trust from an entity such as a security officer, > administrator, system owner, or machine owner are said to have "imputed > trust". CA keys with imputed trust can be loaded onto the machine keyring. > The mechanism for loading these keys onto the machine keyring is platform > dependent. > > Load keys stored in the variable trustedcadb onto the .machine keyring > on PowerVM platform. > > Signed-off-by: Nayna Jain Reviewed-and-tested-by: Mimi Zohar
Re: [PATCH 3/6] integrity: remove global variable from machine_keyring.c
On Fri, 2023-07-14 at 11:34 -0400, Nayna Jain wrote: > trust_mok variable is accessed within a single function locally. > > Change trust_mok from global to local static variable. > > Signed-off-by: Nayna Jain Reviewed-and-tested-by: Mimi Zohar
Re: [PATCH 2/6] integrity: ignore keys failing CA restrictions on non-UEFI platform
On Fri, 2023-07-14 at 11:34 -0400, Nayna Jain wrote: > On non-UEFI platforms, handle restrict_link_by_ca failures differently. > > Certificates which do not satisfy CA restrictions on non-UEFI platforms > are ignored. > > Signed-off-by: Nayna Jain Reviewed-and-tested-by: Mimi Zohar
Re: [PATCH 4/6] integrity: check whether imputed trust is enabled
On Fri, 2023-07-14 at 11:34 -0400, Nayna Jain wrote: > trust_moklist() is specific to UEFI enabled systems. Other platforms > rely only on the Kconfig. > > Define a generic wrapper named imputed_trust_enabled(). > > Signed-off-by: Nayna Jain Reviewed-off-by: Mimi Zohar
Re: [PATCH 5/6] integrity: PowerVM machine keyring enablement.
On Fri, 2023-07-14 at 11:34 -0400, Nayna Jain wrote: > Update Kconfig to enable machine keyring and limit to CA certificates > on PowerVM. > > Signed-off-by: Nayna Jain Reviewed-and-tested-by: Mimi Zohar
Re: [PATCH v4 6/6] integrity: PowerVM support for loading third party code signing keys
On Tue, 2023-08-15 at 07:27 -0400, Nayna Jain wrote: > On secure boot enabled PowerVM LPAR, third party code signing keys are > needed during early boot to verify signed third party modules. These > third party keys are stored in moduledb object in the Platform > KeyStore (PKS). > > Load third party code signing keys onto .secondary_trusted_keys keyring. > > Signed-off-by: Nayna Jain Reviewed-and-tested-by: Mimi Zohar
Re: [PATCH v3 2/6] integrity: ignore keys failing CA restrictions on non-UEFI platform
On Mon, 2023-08-14 at 20:38 +0300, Jarkko Sakkinen wrote:
> On Sun Aug 13, 2023 at 5:15 AM EEST, Nayna Jain wrote:
> > On non-UEFI platforms, handle restrict_link_by_ca failures differently.
> >
> > Certificates which do not satisfy CA restrictions on non-UEFI platforms
> > are ignored.
> >
> > Signed-off-by: Nayna Jain
> > Reviewed-and-tested-by: Mimi Zohar
> > ---
> > security/integrity/platform_certs/machine_keyring.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/integrity/platform_certs/machine_keyring.c
> > b/security/integrity/platform_certs/machine_keyring.c
> > index 7aaed7950b6e..389a6e7c9245 100644
> > --- a/security/integrity/platform_certs/machine_keyring.c
> > +++ b/security/integrity/platform_certs/machine_keyring.c
> > @@ -36,7 +36,7 @@ void __init add_to_machine_keyring(const char *source,
> > const void *data, size_t
> > * If the restriction check does not pass and the platform keyring
> > * is configured, try to add it into that keyring instead.
> > */
> > - if (rc && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
> > + if (rc && efi_enabled(EFI_BOOT) &&
> > IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
> > rc = integrity_load_cert(INTEGRITY_KEYRING_PLATFORM, source,
> > data, len, perm);
> >
> > --
> > 2.31.1
>
> Acked-by: Jarkko Sakkinen
Hi Jarkko,
Without the following two commits in your master branch, the last patch
in this series "[PATCH v4 6/6] integrity: PowerVM support for loading
third party code signing keys" doesn't apply cleanly.
- commit 409b465f8a83 ("integrity: Enforce digitalSignature usage in
the ima and evm keyrings")
- commit e34a6c7dd192 ("KEYS: DigitalSignature link restriction")
If you're not planning on upstreaming this patch set, I'd appreciate
your creating a topic branch with these two commits.
--
thanks,
Mimi
Re: [PATCH v4 6/6] integrity: PowerVM support for loading third party code signing keys
On Wed, 2023-08-16 at 23:36 +0300, Jarkko Sakkinen wrote:
> On Tue Aug 15, 2023 at 2:27 PM EEST, Nayna Jain wrote:
> > On secure boot enabled PowerVM LPAR, third party code signing keys are
> > needed during early boot to verify signed third party modules. These
> > third party keys are stored in moduledb object in the Platform
> > KeyStore (PKS).
> >
> > Load third party code signing keys onto .secondary_trusted_keys keyring.
> >
> > Signed-off-by: Nayna Jain
> > ---
> > certs/system_keyring.c| 30 +++
> > include/keys/system_keyring.h | 4 +++
> > .../platform_certs/keyring_handler.c | 8 +
> > .../platform_certs/keyring_handler.h | 5
> > .../integrity/platform_certs/load_powerpc.c | 17 +++
> > 5 files changed, 64 insertions(+)
> >
> > diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> > index b348e0898d34..33841c91f12c 100644
> > --- a/certs/system_keyring.c
> > +++ b/certs/system_keyring.c
> > @@ -152,6 +152,36 @@ static __init struct key_restriction
> > *get_builtin_and_secondary_restriction(void
> >
> > return restriction;
> > }
> > +
> > +/**
> > + * add_to_secondary_keyring - Add to secondary keyring.
> > + * @source: Source of key
> > + * @data: The blob holding the key
> > + * @len: The length of the data blob
> > + *
> > + * Add a key to the secondary keyring. The key must be vouched for by a
> > key in the builtin,
> > + * machine or secondary keyring itself.
> > + */
> > +void __init add_to_secondary_keyring(const char *source, const void *data,
> > size_t len)
> > +{
> > + key_ref_t key;
> > + key_perm_t perm;
> > +
> > + perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
> > +
> > + key = key_create_or_update(make_key_ref(secondary_trusted_keys, 1),
> > + "asymmetric",
> > + NULL, data, len, perm,
> > + KEY_ALLOC_NOT_IN_QUOTA);
> > + if (IS_ERR(key)) {
> > + pr_err("Problem loading X.509 certificate from %s to secondary
> > keyring %ld\n",
> > + source, PTR_ERR(key));
> > + return;
> > + }
> > +
> > + pr_notice("Loaded X.509 cert '%s'\n", key_ref_to_ptr(key)->description);
> > + key_ref_put(key);
> > +}
> > #endif
> > #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> > void __init set_machine_trusted_keys(struct key *keyring)
> > diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> > index 7e2583208820..8365adf842ef 100644
> > --- a/include/keys/system_keyring.h
> > +++ b/include/keys/system_keyring.h
> > @@ -50,9 +50,13 @@ int restrict_link_by_digsig_builtin_and_secondary(struct
> > key *keyring,
> > const struct key_type *type,
> > const union key_payload
> > *payload,
> > struct key *restriction_key);
> > +void __init add_to_secondary_keyring(const char *source, const void *data,
> > size_t len);
> > #else
> > #define restrict_link_by_builtin_and_secondary_trusted
> > restrict_link_by_builtin_trusted
> > #define restrict_link_by_digsig_builtin_and_secondary
> > restrict_link_by_digsig_builtin
> > +static inline void __init add_to_secondary_keyring(const char *source,
> > const void *data, size_t len)
> > +{
> > +}
> > #endif
> >
> > #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> > diff --git a/security/integrity/platform_certs/keyring_handler.c
> > b/security/integrity/platform_certs/keyring_handler.c
> > index 586027b9a3f5..13ea17207902 100644
> > --- a/security/integrity/platform_certs/keyring_handler.c
> > +++ b/security/integrity/platform_certs/keyring_handler.c
> > @@ -78,6 +78,14 @@ __init efi_element_handler_t
> > get_handler_for_ca_keys(const efi_guid_t *sig_type)
> > return NULL;
> > }
> >
> > +__init efi_element_handler_t get_handler_for_code_signing_keys(const
> > efi_guid_t *sig_type)
> > +{
> > + if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
> > + return add_to_secondary_keyring;
> > +
> > + return NULL;
> > +}
> > +
> > /*
> > * Return the appropriate handler for particular signature list types
> > found in
> > * the UEFI dbx and MokListXRT tables.
> > diff --git a/security/integrity/platform_certs/keyring_handler.h
> > b/security/integrity/platform_certs/keyring_handler.h
> > index 6f15bb4cc8dc..f92895cc50f6 100644
> > --- a/security/integrity/platform_certs/keyring_handler.h
> > +++ b/security/integrity/platform_certs/keyring_handler.h
> > @@ -34,6 +34,11 @@ efi_element_handler_t get_handler_for_mok(const
> > efi_guid_t *sig_type);
> > */
> > efi_element_handler_t get_handler_for_ca_keys(const efi_guid_t *sig_type);
> >
> > +/*
> > + * Return the handler for particular signature list types for code signing
> > keys.
> > + */
> > +efi_element_handler_t get_handler_for_code_signing_keys(const efi_guid_t
>
Re: [PATCH v2 12/13] s390/kexec: refactor for kernel/Kconfig.kexec
On Wed, 2023-06-21 at 07:00 +0200, Alexander Gordeev wrote:
> AFAICT the MODULE_SIG_FORMAT dependency was introduced with commit
> c8424e776b09 ("MODSIGN: Export module signature definitions") and
> in fact was not necessary, since s390 did/does not use mod_check_sig()
> anyway. So the SYSTEM_DATA_VERIFICATION could have left intact.
FYI, this patch was included in the patch set to allow IMA to verify
the kexec kernel image appended signature on OpenPOWER.
--
thanks,
Mimi
Re: [PATCH 5.15 0/6] arm64: kexec_file: use more system keyrings to verify kernel image signature + dependencies
On Fri, 2022-09-23 at 19:10 +0200, Michal Suchanek wrote:
> Hello,
>
> this is backport of commit 0d519cadf751
> ("arm64: kexec_file: use more system keyrings to verify kernel image
> signature")
> to table 5.15 tree including the preparatory patches.
>
> Some patches needed minor adjustment for context.
In general when backporting this patch set, there should be a
dependency on backporting these commits as well. In this instance for
linux-5.15.y, they've already been backported.
543ce63b664e ("lockdown: Fix kexec lockdown bypass with ima policy")
af16df54b89d ("ima: force signature verification when CONFIG_KEXEC_SIG is
configured")
--
thanks,
Mimi
Re: [PATCH v4 24/24] integrity/powerpc: Support loading keys from pseries secvar
On Fri, 2023-01-20 at 18:43 +1100, Andrew Donnellan wrote:
> From: Russell Currey
>
> The secvar object format is only in the device tree under powernv.
> We now have an API call to retrieve it in a generic way, so we should
> use that instead of having to handle the DT here.
>
> Add support for pseries secvar, with the "ibm,plpks-sb-v1" format.
> The object format is expected to be the same, so there shouldn't be any
> functional differences between objects retrieved from powernv and
> pseries.
>
> Signed-off-by: Russell Currey
> Signed-off-by: Andrew Donnellan
>
> ---
>
> v3: New patch
>
> v4: Pass format buffer size (stefanb, npiggin)
> ---
> .../integrity/platform_certs/load_powerpc.c | 17 ++---
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_powerpc.c
> b/security/integrity/platform_certs/load_powerpc.c
> index dee51606d5f4..d4ce91bf3fec 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -10,7 +10,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include "keyring_handler.h"
> @@ -59,16 +58,22 @@ static int __init load_powerpc_certs(void)
> void *db = NULL, *dbx = NULL;
> u64 dbsize = 0, dbxsize = 0;
> int rc = 0;
> - struct device_node *node;
> + ssize_t len;
> + char buf[32];
>
> if (!secvar_ops)
> return -ENODEV;
>
> - /* The following only applies for the edk2-compat backend. */
> - node = of_find_compatible_node(NULL, NULL, "ibm,edk2-compat-v1");
> - if (!node)
> + len = secvar_ops->format(buf, 32);
"powerpc/secvar: Handle format string in the consumer" defines
opal_secvar_format() for the object format "ibm,secvar-backend". Here
shouldn't it being returning the format for "ibm,edk2-compat-v1"?
Mimi
> + if (len <= 0)
> return -ENODEV;
>
> + // Check for known secure boot implementations from OPAL or PLPKS
> + if (strcmp("ibm,edk2-compat-v1", buf) && strcmp("ibm,plpks-sb-v1",
> buf)) {
> + pr_err("Unsupported secvar implementation \"%s\", not loading
> certs\n", buf);
> + return -ENODEV;
> + }
> +
> /*
>* Get db, and dbx. They might not exist, so it isn't an error if we
>* can't get them.
> @@ -103,8 +108,6 @@ static int __init load_powerpc_certs(void)
> kfree(dbx);
> }
>
> - of_node_put(node);
> -
> return rc;
> }
> late_initcall(load_powerpc_certs);
Re: [PATCH v4 23/24] integrity/powerpc: Improve error handling & reporting when loading certs
On Fri, 2023-01-20 at 18:43 +1100, Andrew Donnellan wrote: > From: Russell Currey > > A few improvements to load_powerpc.c: > > - include integrity.h for the pr_fmt() > - move all error reporting out of get_cert_list() > - use ERR_PTR() to better preserve error detail > - don't use pr_err() for missing keys > > Signed-off-by: Russell Currey > Signed-off-by: Andrew Donnellan Thanks, Reviewed-by: Mimi Zohar
Re: [PATCH v4 24/24] integrity/powerpc: Support loading keys from pseries secvar
On Wed, 2023-01-25 at 13:23 +1100, Russell Currey wrote:
> On Tue, 2023-01-24 at 10:14 -0500, Mimi Zohar wrote:
> > On Fri, 2023-01-20 at 18:43 +1100, Andrew Donnellan wrote:
> > > From: Russell Currey
> > >
> > > The secvar object format is only in the device tree under powernv.
> > > We now have an API call to retrieve it in a generic way, so we
> > > should
> > > use that instead of having to handle the DT here.
> > >
> > > Add support for pseries secvar, with the "ibm,plpks-sb-v1" format.
> > > The object format is expected to be the same, so there shouldn't be
> > > any
> > > functional differences between objects retrieved from powernv and
> > > pseries.
> > >
> > > Signed-off-by: Russell Currey
> > > Signed-off-by: Andrew Donnellan
> > >
> > > ---
> > >
> > > v3: New patch
> > >
> > > v4: Pass format buffer size (stefanb, npiggin)
> > > ---
> > > .../integrity/platform_certs/load_powerpc.c | 17 ++---
> > >
> > > 1 file changed, 10 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/security/integrity/platform_certs/load_powerpc.c
> > > b/security/integrity/platform_certs/load_powerpc.c
> > > index dee51606d5f4..d4ce91bf3fec 100644
> > > --- a/security/integrity/platform_certs/load_powerpc.c
> > > +++ b/security/integrity/platform_certs/load_powerpc.c
> > > @@ -10,7 +10,6 @@
> > > #include
> > > #include
> > > #include
> > > -#include
> > > #include
> > > #include
> > > #include "keyring_handler.h"
> > > @@ -59,16 +58,22 @@ static int __init load_powerpc_certs(void)
> > > void *db = NULL, *dbx = NULL;
> > > u64 dbsize = 0, dbxsize = 0;
> > > int rc = 0;
> > > - struct device_node *node;
> > > + ssize_t len;
> > > + char buf[32];
> > >
> > > if (!secvar_ops)
> > > return -ENODEV;
> > >
> > > - /* The following only applies for the edk2-compat backend.
> > > */
> > > - node = of_find_compatible_node(NULL, NULL, "ibm,edk2-
> > > compat-v1");
> > > - if (!node)
> > > + len = secvar_ops->format(buf, 32);
> >
> > "powerpc/secvar: Handle format string in the consumer" defines
> > opal_secvar_format() for the object format "ibm,secvar-backend".
> > Here
> > shouldn't it being returning the format for "ibm,edk2-compat-v1"?
> >
>
> They end up with the same value. The DT structure on powernv looks
> like this:
>
> /proc/device-tree/ibm,opal/secvar:
> name "secvar"
> compatible "ibm,secvar-backend"
>"ibm,edk2-compat-v1"
> format "ibm,edk2-compat-v1"
> max-var-key-len 0400
> phandle 805a (32858)
> max-var-size 2000
>
> The existing code is checking for a node compatible with "ibm,edk2-
> compat-v1", which would match the node above. opal_secvar_format()
> checks for a node compatible with "ibm,secvar-backend" (again, matching
> above) and then returns the contents of the "format" string, which is
> "ibm,edk2-compat-v1".
>
> Ultimately it's two different ways of doing the same thing, but this
> way load_powerpc_certs() doesn't have to interact with the device tree.
Agreed. Thank you for the explanation. To simplify review, I suggest
either adding this explanation in the patch description or stage the
change by replacing the existing "ibm,edk2-compat-v1" usage first.
thanks,
Mimi
>
>
> > Mimi
> >
> > > + if (len <= 0)
> > > return -ENODEV;
> > >
> > > + // Check for known secure boot implementations from OPAL or
> > > PLPKS
> > > + if (strcmp("ibm,edk2-compat-v1", buf) && strcmp("ibm,plpks-
> > > sb-v1", buf)) {
> > > + pr_err("Unsupported secvar implementation \"%s\",
> > > not loading certs\n", buf);
> > > + return -ENODEV;
> > > + }
> > > +
> > > /*
> > > * Get db, and dbx. They might not exist, so it isn't an
> > > error if we
> > > * can't get them.
> > > @@ -103,8 +108,6 @@ static int __init load_powerpc_certs(void)
> > > kfree(dbx);
> > > }
> > >
> > > - of_node_put(node);
> > > -
> > > return rc;
> > > }
> > > late_initcall(load_powerpc_certs);
> >
> >
>
Re: [PATCH v6 0/3] Carry forward IMA measurement log on kexec on ARM64
[Cc'ing Nayna Jain, linuxppc-dev@lists.ozlabs.org]
Hi Lakshmi,
On Tue, 2020-09-08 at 16:08 -0700, Lakshmi Ramasubramanian wrote:
> On kexec file load Integrity Measurement Architecture(IMA) subsystem
> may verify the IMA signature of the kernel and initramfs, and measure
> it. The command line parameters passed to the kernel in the kexec call
> may also be measured by IMA. A remote attestation service can verify
> the measurement through the IMA log and the TPM PCR data. This can be
> achieved only if the IMA measurement log is carried over from
> the current kernel to the next kernel across the kexec call.
> However in the current implementation the IMA measurement logs are not
> carried over on ARM64 platforms. Therefore a remote attestation service
> cannot verify the authenticity of the running kernel on ARM64 platforms
> when the kernel is updated through the kexec system call.
>
> This patch series adds support for carrying forward the IMA measurement
> log on kexec on ARM64. powerpc already supports carrying forward
> the IMA measurement log on kexec.
>
> This series refactors the platform independent code defined for powerpc
> such that it can be reused for ARM64 as well. A chosen node namely
> "linux,ima-kexec-buffer" is added to the DTB for ARM64 to hold
> the address and the size of the memory reserved to carry
> the IMA measurement log.
>
> This patch series has been tested for ARM64 platform using QEMU.
> I would like help from the community for testing this change on powerpc.
> Thanks.
Getting access to and upgrading our group's Power system firmware took
time due to limited employee site access. Confirmed, with these
patches applied, ima-evm-utils still properly verifies carrying the IMA
measurement list across kexec.
I plan on reviewing this patch set shortly.
thanks,
Mimi
>
> This series is based on commit f4d51dffc6c0 ("Linux 5.9-rc4") in
> https://github.com/torvalds/linux "master" branch.
>
> Changelog:
>
> v6:
> - Remove any existing FDT_PROP_IMA_KEXEC_BUFFER property in the device
> tree and also its corresponding memory reservation in the currently
> running kernel.
> - Moved the function remove_ima_buffer() defined for powerpc to IMA
> and renamed the function to ima_remove_kexec_buffer(). Also, moved
> delete_fdt_mem_rsv() from powerpc to IMA.
>
> v5:
> - Merged get_addr_size_cells() and do_get_kexec_buffer() into a single
> function when moving the arch independent code from powerpc to IMA
> - Reverted the change to use FDT functions in powerpc code and added
> back the original code in get_addr_size_cells() and
> do_get_kexec_buffer() for powerpc.
> - Added fdt_add_mem_rsv() for ARM64 to reserve the memory for
> the IMA log buffer during kexec.
> - Fixed the warning reported by kernel test bot for ARM64
> arch_ima_add_kexec_buffer() - moved this function to a new file
> namely arch/arm64/kernel/ima_kexec.c
>
> v4:
> - Submitting the patch series on behalf of the original author
> Prakhar Srivastava
> - Moved FDT_PROP_IMA_KEXEC_BUFFER ("linux,ima-kexec-buffer") to
> libfdt.h so that it can be shared by multiple platforms.
>
> v3:
> Breakup patches further into separate patches.
> - Refactoring non architecture specific code out of powerpc
> - Update powerpc related code to use fdt functions
> - Update IMA buffer read related code to use of functions
> - Add support to store the memory information of the IMA
> measurement logs to be carried forward.
> - Update the property strings to align with documented nodes
> https://github.com/devicetree-org/dt-schema/pull/46
>
> v2:
> Break patches into separate patches.
> - Powerpc related Refactoring
> - Updating the docuemntation for chosen node
> - Updating arm64 to support IMA buffer pass
>
> v1:
> Refactoring carrying over IMA measuremnet logs over Kexec. This patch
> moves the non-architecture specific code out of powerpc and adds to
> security/ima.(Suggested by Thiago)
> Add Documentation regarding the ima-kexec-buffer node in the chosen
> node documentation
>
> v0:
> Add a layer of abstraction to use the memory reserved by device tree
> for ima buffer pass.
> Add support for ima buffer pass using reserved memory for arm64 kexec.
> Update the arch sepcific code path in kexec file load to store the
> ima buffer in the reserved memory. The same reserved memory is read
> on kexec or cold boot.
>
> Lakshmi Ramasubramanian (3):
> powerpc: Refactor kexec functions to move arch independent code to IMA
> arm64: Store IMA log information in kimage used for kexec
> arm64: Add IMA kexec buffer to DTB
>
> arch/arm64/Kconfig | 1 +
> arch/arm64/include/asm/ima.h| 18
> arch/arm64/include/asm/kexec.h | 3 +
> arch/arm64/kernel/Makefile | 1 +
> arch/arm64/kernel/ima_kexec.c | 34
> arch/arm64/kernel/machine_ke
Re: [PATCH] linux: configure CONFIG_I2C_OPAL as in-built.
Hi Joel, On Tue, 2020-09-29 at 06:14 +, Joel Stanley wrote: > On Fri, 25 Sep 2020 at 18:19, Mimi Zohar wrote: > > > > Hi Nayna, > > > > On Wed, 2020-09-23 at 14:25 -0400, Nayna Jain wrote: > > > Currently, skiroot_defconfig CONFIG_I2C_OPAL is built as a loadable > > > module rather than builtin, even if CONFIG_I2C=y is defined. This > > > results in a delay in the TPM initialization, causing IMA to go into > > > TPM bypass mode. As a result, the IMA measurements are added to the > > > measurement list, but do not extend the TPM. Because of this, it is > > > impossible to verify or attest to the system's integrity, either from > > > skiroot or the target Host OS. > > > > The patch description is good, but perhaps we could provide a bit more > > context before. > > > > The concept of trusted boot requires the measurement to be added to the > > measurement list and extend the TPM, prior to allowing access to the > > file. By allowing access to a file before its measurement is included > > in the measurement list and extended into the TPM PCR, a malicious file > > could potentially prevent its own measurement from being added. As the > > PCRs are tamper proof, measuring and extending the TPM prior to giving > > access to the file, guarantees that all file measurements are included > > in the measurement list, including the malicious file. > > > > IMA needs to be enabled before any files are accessed in order to > > verify a file's integrity and extend the TPM with the file > > measurement. Queueing file measurements breaks the measure and extend, > > before usage, trusted boot paradigm. > > > > The ima-evm-utils package includes a test for walking the IMA > > measurement list, calculating the expected TPM PCRs, and comparing the > > calculated PCR values with the physical TPM. Testing is important to > > ensure the TPM is initialized prior to IMA. Failure to validate the > > IMA measurement list may indicate IMA went into TPM bypass mode, like > > in this case. > > Thanks for the explanation Mimi. It's lucky that the TPM drivers can > be loaded early enough! > > Should we add something like this to security/integrity/ima/Kconfig? > > select I2C_OPAL if PPC_POWERNV > > It's generally frowned upon to select user visible symbols, but IMA > does this for the TCG options already. The other examples enable the TPM. I2C_OPAL is dependent on I2C being builtin. I'm not sure if this select is complete, nor if this is where it belongs. Mimi
Re: [PATCH v2] ima: defer arch_ima_get_secureboot() call to IMA init time
[Cc'ing linuxppc-dev@lists.ozlabs.org] On Tue, 2020-10-13 at 10:18 +0200, Ard Biesheuvel wrote: > Chester reports that it is necessary to introduce a new way to pass > the EFI secure boot status between the EFI stub and the core kernel > on ARM systems. The usual way of obtaining this information is by > checking the SecureBoot and SetupMode EFI variables, but this can > only be done after the EFI variable workqueue is created, which > occurs in a subsys_initcall(), whereas arch_ima_get_secureboot() > is called much earlier by the IMA framework. > > However, the IMA framework itself is started as a late_initcall, > and the only reason the call to arch_ima_get_secureboot() occurs > so early is because it happens in the context of a __setup() > callback that parses the ima_appraise= command line parameter. > > So let's refactor this code a little bit, by using a core_param() > callback to capture the command line argument, and deferring any > reasoning based on its contents to the IMA init routine. > > Cc: Chester Lin > Cc: Mimi Zohar > Cc: Dmitry Kasatkin > Cc: James Morris > Cc: "Serge E. Hallyn" > Link: > https://lore.kernel.org/linux-arm-kernel/20200904072905.25332-2-c...@suse.com/ > Signed-off-by: Ard Biesheuvel > --- > v2: rebase onto series 'integrity: improve user feedback for invalid > bootparams' Thanks, Ard. Based on my initial, limited testing on Power, it looks good, but I'm hesistant to include it in the integrity 5.10 pull request without it having been in linux-next and some additional testing. It's now queued in the next-integrity-testing branch awaiting some tags. thanks, Mimi
Re: [PATCH v2] ima: defer arch_ima_get_secureboot() call to IMA init time
On Wed, 2020-10-14 at 17:35 +0800, Chester Lin wrote: > Hi Ard & Mimi, > > On Tue, Oct 13, 2020 at 06:59:21PM +0200, Ard Biesheuvel wrote: > > On Tue, 13 Oct 2020 at 18:46, Mimi Zohar wrote: > > > > > > [Cc'ing linuxppc-dev@lists.ozlabs.org] > > > > > > On Tue, 2020-10-13 at 10:18 +0200, Ard Biesheuvel wrote: > > > > Chester reports that it is necessary to introduce a new way to pass > > > > the EFI secure boot status between the EFI stub and the core kernel > > > > on ARM systems. The usual way of obtaining this information is by > > > > checking the SecureBoot and SetupMode EFI variables, but this can > > > > only be done after the EFI variable workqueue is created, which > > > > occurs in a subsys_initcall(), whereas arch_ima_get_secureboot() > > > > is called much earlier by the IMA framework. > > > > > > > > However, the IMA framework itself is started as a late_initcall, > > > > and the only reason the call to arch_ima_get_secureboot() occurs > > > > so early is because it happens in the context of a __setup() > > > > callback that parses the ima_appraise= command line parameter. > > > > > > > > So let's refactor this code a little bit, by using a core_param() > > > > callback to capture the command line argument, and deferring any > > > > reasoning based on its contents to the IMA init routine. > > > > > > > > Cc: Chester Lin > > > > Cc: Mimi Zohar > > > > Cc: Dmitry Kasatkin > > > > Cc: James Morris > > > > Cc: "Serge E. Hallyn" > > > > Link: > > > > https://lore.kernel.org/linux-arm-kernel/20200904072905.25332-2-c...@suse.com/ > > > > Signed-off-by: Ard Biesheuvel > > > > --- > > > > v2: rebase onto series 'integrity: improve user feedback for invalid > > > > bootparams' > > > > > > Thanks, Ard. Based on my initial, limited testing on Power, it looks > > > good, but I'm hesistant to include it in the integrity 5.10 pull > > > request without it having been in linux-next and some additional > > > testing. It's now queued in the next-integrity-testing branch awaiting > > > some tags. > > > > > Tested-by: Chester Lin > > I have tested this patch on x86 VM. > > * System configuration: > - Platform: QEMU/KVM > - Firmware: EDK2/OVMF + secure boot enabled. > - OS: SLE15-SP2 + SUSE's kernel-vanilla (=linux v5.9) + the follow commits > from linux-next and upstream: > * [PATCH v2] ima: defer arch_ima_get_secureboot() call to IMA init time > https://www.spinics.net/lists/linux-efi/msg20645.html > * e4d7e2df3a09 "ima: limit secure boot feedback scope for appraise" > * 7fe2bb7e7e5c "integrity: invalid kernel parameters feedback" > * 4afb28ab03d5 "ima: add check for enforced appraise option" > > * Logs with UEFI secure boot enabled: > > [0.00] Linux version 5.9.0-858-g865c50e1d279-1.g8764d18-vanilla > (geeko@b > uildhost) (gcc (SUSE Linux) 10.2.1 20200825 [revision > c0746a1beb1ba073c7981eb09f > 55b3d993b32e5c], GNU ld (GNU Binutils; openSUSE Tumbleweed) > 2.34.0.20200325-1) # > 1 SMP Wed Oct 14 04:00:11 UTC 2020 (8764d18) > [0.00] Command line: > BOOT_IMAGE=/boot/vmlinuz-5.9.0-858-g865c50e1d279-1. > g8764d18-vanilla root=UUID=5304c03e-4d8a-4d67-873a-32a32e57cdeb > console=ttyS0,11 > 5200 resume=/dev/disk/by-path/pci-:04:00.0-part4 mitigations=auto > ignore_log > level crashkernel=192M,high crashkernel=72M,low ima_appraise=off > [0.00] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point > regi > sters' > [0.00] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers' > [0.00] x86/fpu: Supporting XSAVE feature 0x004: 'AVX registers' > > > [1.720309] ima: Secure boot enabled: ignoring ima_appraise=off option > [1.720314] ima: No TPM chip found, activating TPM-bypass! > [1.722129] ima: Allocated hash algorithm: sha256 Thank you for testing the options aren't being set in secure boot mode. My main concern, however, is that IMA doesn't go into TPM-bypass mode. Does this system have a TPM? Mimi
Re: [PATCH v2 1/2] ima: Free IMA measurement buffer on error
On Fri, 2021-02-05 at 09:39 -0800, Lakshmi Ramasubramanian wrote:
> On 2/5/21 2:05 AM, Greg KH wrote:
> > On Thu, Feb 04, 2021 at 09:49:50AM -0800, Lakshmi Ramasubramanian wrote:
> >> IMA allocates kernel virtual memory to carry forward the measurement
> >> list, from the current kernel to the next kernel on kexec system call,
> >> in ima_add_kexec_buffer() function. In error code paths this memory
> >> is not freed resulting in memory leak.
> >>
> >> Free the memory allocated for the IMA measurement list in
> >> the error code paths in ima_add_kexec_buffer() function.
> >>
> >> Signed-off-by: Lakshmi Ramasubramanian
> >> Suggested-by: Tyler Hicks
> >> Fixes: 7b8589cc29e7 ("ima: on soft reboot, save the measurement list")
> >> ---
> >> security/integrity/ima/ima_kexec.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >
> >
> >
> > This is not the correct way to submit patches for inclusion in the
> > stable kernel tree. Please read:
> > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > for how to do this properly.
> >
> >
> >
>
> Thanks for the info Greg.
>
> I will re-submit the two patches in the proper format.
No need. I'm testing these patches now. I'm not exactly sure what the
problem is. Stable wasn't Cc'ed. Is it that you sent the patch
directly to Greg or added "Fixes"?
thanks,
Mimi
Re: [PATCH v17 00/10] Carry forward IMA measurement log on kexec on ARM64
On Wed, 2021-02-10 at 14:42 -0600, Rob Herring wrote:
> On Wed, Feb 10, 2021 at 11:33 AM Lakshmi Ramasubramanian
> wrote:
> >
> > On 2/10/21 9:15 AM, Rob Herring wrote:
> > > On Tue, Feb 09, 2021 at 10:21:50AM -0800, Lakshmi Ramasubramanian wrote:
> > >> On kexec file load Integrity Measurement Architecture (IMA) subsystem
> > >> may verify the IMA signature of the kernel and initramfs, and measure
> > >> it. The command line parameters passed to the kernel in the kexec call
> > >> may also be measured by IMA. A remote attestation service can verify
> > >> a TPM quote based on the TPM event log, the IMA measurement list, and
> > >> the TPM PCR data. This can be achieved only if the IMA measurement log
> > >> is carried over from the current kernel to the next kernel across
> > >> the kexec call.
> > >>
> > >> powerpc already supports carrying forward the IMA measurement log on
> > >> kexec. This patch set adds support for carrying forward the IMA
> > >> measurement log on kexec on ARM64.
> > >>
> > >> This patch set moves the platform independent code defined for powerpc
> > >> such that it can be reused for other platforms as well. A chosen node
> > >> "linux,ima-kexec-buffer" is added to the DTB for ARM64 to hold
> > >> the address and the size of the memory reserved to carry
> > >> the IMA measurement log.
> > >>
> > >> This patch set has been tested for ARM64 platform using QEMU.
> > >> I would like help from the community for testing this change on powerpc.
> > >> Thanks.
> > >>
> > >> This patch set is based on
> > >> commit 96acc833dec8 ("ima: Free IMA measurement buffer after kexec
> > >> syscall")
> > >> in
> > >> https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
> > >> "next-integrity" branch.
> > >
> > > Is that a hard dependency still? Given this is now almost entirely
> > > deleting arch code and adding drivers/of/ code, I was going to apply it.
> > >
> >
> > I tried applying the patches in Linus' mainline branch -
> > PATCH #5 0005-powerpc-Move-ima-buffer-fields-to-struct-kimage.patch
> > doesn't apply.
> >
> > But if I apply the dependent patch set (link given below), all the
> > patches in this patch set apply fine.
> >
> > https://patchwork.kernel.org/project/linux-integrity/patch/20210204174951.25771-2-nra...@linux.microsoft.com/
>
> Ideally, we don't apply the same patch in 2 branches. It looks like
> there's a conflict but no real dependence on the above patch (the
> ima_buffer part). The conflict seems trivial enough that Linus can
> resolve it in the merge window.
>
> Or Mimi can take the whole thing if preferred?
How about I create a topic branch with just the two patches, allowing
both of us to merge it? There shouldn't be a problem with re-writing
next-integrity history.
Mimi
Re: [PATCH v17 00/10] Carry forward IMA measurement log on kexec on ARM64
On Wed, 2021-02-10 at 15:55 -0500, Mimi Zohar wrote: > On Wed, 2021-02-10 at 14:42 -0600, Rob Herring wrote: > > On Wed, Feb 10, 2021 at 11:33 AM Lakshmi Ramasubramanian > > > Ideally, we don't apply the same patch in 2 branches. It looks like > > there's a conflict but no real dependence on the above patch (the > > ima_buffer part). The conflict seems trivial enough that Linus can > > resolve it in the merge window. > > > > Or Mimi can take the whole thing if preferred? > > How about I create a topic branch with just the two patches, allowing > both of us to merge it? There shouldn't be a problem with re-writing > next-integrity history. The 2 patches are now in the ima-kexec-fixes branch. Mimi
Re: [PATCH] of: error: 'const struct kimage' has no member named 'arch'
On Thu, 2021-02-18 at 14:33 -0800, Lakshmi Ramasubramanian wrote:
> of_kexec_alloc_and_setup_fdt() defined in drivers/of/kexec.c builds
> a new device tree object that includes architecture specific data
> for kexec system call. This should be defined only if the architecture
> being built defines kexec architecture structure "struct kimage_arch".
>
> Define a new boolean config OF_KEXEC that is enabled if
> CONFIG_KEXEC_FILE and CONFIG_OF_FLATTREE are enabled, and
> the architecture is arm64 or powerpc64. Build drivers/of/kexec.c
> if CONFIG_OF_KEXEC is enabled.
>
> Signed-off-by: Lakshmi Ramasubramanian
> Fixes: 33488dc4d61f ("of: Add a common kexec FDT setup function")
> Reported-by: kernel test robot
> ---
> drivers/of/Kconfig | 6 ++
> drivers/of/Makefile | 7 +--
> 2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 18450437d5d5..f2e8fa54862a 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -100,4 +100,10 @@ config OF_DMA_DEFAULT_COHERENT
> # arches should select this if DMA is coherent by default for OF devices
> bool
>
> +config OF_KEXEC
> + bool
> + depends on KEXEC_FILE
> + depends on OF_FLATTREE
> + default y if ARM64 || PPC64
> +
> endif # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index c13b982084a3..287579dd1695 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -13,11 +13,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> obj-$(CONFIG_OF_RESOLVE) += resolver.o
> obj-$(CONFIG_OF_OVERLAY) += overlay.o
> obj-$(CONFIG_OF_NUMA) += of_numa.o
> -
> -ifdef CONFIG_KEXEC_FILE
> -ifdef CONFIG_OF_FLATTREE
> -obj-y+= kexec.o
> -endif
> -endif
> +obj-$(CONFIG_OF_KEXEC) += kexec.o
>
> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
Is it possible to reuse CONFIG_HAVE_IMA_KEXEC here?
Mimi
Re: [PATCH] of: error: 'const struct kimage' has no member named 'arch'
On Fri, 2021-02-19 at 11:08 -0300, Thiago Jung Bauermann wrote:
> Lakshmi Ramasubramanian writes:
>
> > On 2/18/21 5:13 PM, Thiago Jung Bauermann wrote:
> >> Lakshmi Ramasubramanian writes:
> >>
> >>> On 2/18/21 4:07 PM, Mimi Zohar wrote:
> >>>
> >>> Hi Mimi,
> >>>
> >>>> On Thu, 2021-02-18 at 14:33 -0800, Lakshmi Ramasubramanian wrote:
> >>>>> of_kexec_alloc_and_setup_fdt() defined in drivers/of/kexec.c builds
> >>>>> a new device tree object that includes architecture specific data
> >>>>> for kexec system call. This should be defined only if the architecture
> >>>>> being built defines kexec architecture structure "struct kimage_arch".
> >>>>>
> >>>>> Define a new boolean config OF_KEXEC that is enabled if
> >>>>> CONFIG_KEXEC_FILE and CONFIG_OF_FLATTREE are enabled, and
> >>>>> the architecture is arm64 or powerpc64. Build drivers/of/kexec.c
> >>>>> if CONFIG_OF_KEXEC is enabled.
> >>>>>
> >>>>> Signed-off-by: Lakshmi Ramasubramanian
> >>>>> Fixes: 33488dc4d61f ("of: Add a common kexec FDT setup function")
> >>>>> Reported-by: kernel test robot
> >>>>> ---
> >>>>>drivers/of/Kconfig | 6 ++
> >>>>>drivers/of/Makefile | 7 +--
> >>>>>2 files changed, 7 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> >>>>> index 18450437d5d5..f2e8fa54862a 100644
> >>>>> --- a/drivers/of/Kconfig
> >>>>> +++ b/drivers/of/Kconfig
> >>>>> @@ -100,4 +100,10 @@ config OF_DMA_DEFAULT_COHERENT
> >>>>> # arches should select this if DMA is coherent by default for
> >>>>> OF devices
> >>>>> bool
> >>>>>+config OF_KEXEC
> >>>>> + bool
> >>>>> + depends on KEXEC_FILE
> >>>>> + depends on OF_FLATTREE
> >>>>> + default y if ARM64 || PPC64
> >>>>> +
> >>>>>endif # OF
> >>>>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> >>>>> index c13b982084a3..287579dd1695 100644
> >>>>> --- a/drivers/of/Makefile
> >>>>> +++ b/drivers/of/Makefile
> >>>>> @@ -13,11 +13,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> >>>>>obj-$(CONFIG_OF_RESOLVE) += resolver.o
> >>>>>obj-$(CONFIG_OF_OVERLAY) += overlay.o
> >>>>>obj-$(CONFIG_OF_NUMA) += of_numa.o
> >>>>> -
> >>>>> -ifdef CONFIG_KEXEC_FILE
> >>>>> -ifdef CONFIG_OF_FLATTREE
> >>>>> -obj-y += kexec.o
> >>>>> -endif
> >>>>> -endif
> >>>>> +obj-$(CONFIG_OF_KEXEC) += kexec.o
> >>>>> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> >>>> Is it possible to reuse CONFIG_HAVE_IMA_KEXEC here?
> >>>>
> >>>
> >>> For ppc64 CONFIG_HAVE_IMA_KEXEC is selected when CONFIG_KEXEC_FILE is
> >>> enabled.
> >>> So I don't see a problem in reusing CONFIG_HAVE_IMA_KEXEC for ppc.
> >>>
> >>> But for arm64, CONFIG_HAVE_IMA_KEXEC is enabled in the final patch in the
> >>> patch
> >>> set (the one for carrying forward IMA log across kexec for arm64). arm64
> >>> calls
> >>> of_kexec_alloc_and_setup_fdt() prior to enabling CONFIG_HAVE_IMA_KEXEC
> >>> and hence
> >>> breaks the build for arm64.
> >> One problem is that I believe that this patch won't placate the robot,
> >> because IIUC it generates config files at random and this change still
> >> allows hppa and s390 to enable CONFIG_OF_KEXEC.
> >
> > I enabled CONFIG_OF_KEXEC for s390. With my patch applied, CONFIG_OF_KEXEC
> > is
> > removed. So I think the robot enabling this config would not be a problem.
> >
> >> Perhaps a new CONFIG_HAVE_KIMAGE_ARCH option? Not having that option
> >> would still allow building kexec.o, but would be used inside kexec.c to
> >> avoid accessing kimage.arch members.
> >>
> >
> > I think this is a good idea - a new CONFIG_HAVE_KIMAGE_ARCH, which will be
> > selected by arm64 and ppc for now. I tried this, and it fixes the build
> > issue.
> >
> > Although, the name for the new config can be misleading since PARISC, for
> > instance, also defines "struct kimage_arch". Perhaps,
> > CONFIG_HAVE_ELF_KIMAGE_ARCH since of_kexec_alloc_and_setup_fdt() is
> > accessing ELF specific fields in "struct kimage_arch"?
>
> Ah, right. I should have digged into the code before making my
> suggestion. CONFIG_HAVE_KIMAGE_ARCH isn't appropriate, indeed.
>
> >
> > Rob/Mimi - please let us know which approach you think is better.
>
> Ah! We can actually use the existing CONFIG_HAVE_IMA_KEXEC, no? I don't
> know why I didn't think of it before.
Including kexec.o based on CONFIG_HAVE_IMA_KEXEC is a bisect issue on
ARM64, as Lakshmi pointed out. Defining a new, maybe temporary, flag
would solve the problem.
Mimi
Re: [PATCH] of: error: 'const struct kimage' has no member named 'arch'
On Fri, 2021-02-19 at 11:43 -0600, Rob Herring wrote:
> On Fri, Feb 19, 2021 at 10:57 AM Lakshmi Ramasubramanian
> wrote:
> >
> > On 2/19/21 6:16 AM, Rob Herring wrote:
> > > On Thu, Feb 18, 2021 at 8:53 PM Lakshmi Ramasubramanian
> > > wrote:
> > >>
> > >> On 2/18/21 5:13 PM, Thiago Jung Bauermann wrote:
> > >>>
> > >>> Lakshmi Ramasubramanian writes:
> > >>>
> > >>>> On 2/18/21 4:07 PM, Mimi Zohar wrote:
> > >>>>
> > >>>> Hi Mimi,
> > >>>>
> > >>>>> On Thu, 2021-02-18 at 14:33 -0800, Lakshmi Ramasubramanian wrote:
> > >>>>>> of_kexec_alloc_and_setup_fdt() defined in drivers/of/kexec.c builds
> > >>>>>> a new device tree object that includes architecture specific data
> > >>>>>> for kexec system call. This should be defined only if the
> > >>>>>> architecture
> > >>>>>> being built defines kexec architecture structure "struct
> > >>>>>> kimage_arch".
> > >>>>>>
> > >>>>>> Define a new boolean config OF_KEXEC that is enabled if
> > >>>>>> CONFIG_KEXEC_FILE and CONFIG_OF_FLATTREE are enabled, and
> > >>>>>> the architecture is arm64 or powerpc64. Build drivers/of/kexec.c
> > >>>>>> if CONFIG_OF_KEXEC is enabled.
> > >>>>>>
> > >>>>>> Signed-off-by: Lakshmi Ramasubramanian
> > >>>>>> Fixes: 33488dc4d61f ("of: Add a common kexec FDT setup function")
> > >>>>>> Reported-by: kernel test robot
> > >>>>>> ---
> > >>>>>> drivers/of/Kconfig | 6 ++
> > >>>>>> drivers/of/Makefile | 7 +--
> > >>>>>> 2 files changed, 7 insertions(+), 6 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > >>>>>> index 18450437d5d5..f2e8fa54862a 100644
> > >>>>>> --- a/drivers/of/Kconfig
> > >>>>>> +++ b/drivers/of/Kconfig
> > >>>>>> @@ -100,4 +100,10 @@ config OF_DMA_DEFAULT_COHERENT
> > >>>>>> # arches should select this if DMA is coherent by
> > >>>>>> default for OF devices
> > >>>>>> bool
> > >>>>>> +config OF_KEXEC
> > >>>>>> + bool
> > >>>>>> + depends on KEXEC_FILE
> > >>>>>> + depends on OF_FLATTREE
> > >>>>>> + default y if ARM64 || PPC64
> > >>>>>> +
> > >>>>>> endif # OF
> > >>>>>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > >>>>>> index c13b982084a3..287579dd1695 100644
> > >>>>>> --- a/drivers/of/Makefile
> > >>>>>> +++ b/drivers/of/Makefile
> > >>>>>> @@ -13,11 +13,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> > >>>>>> obj-$(CONFIG_OF_RESOLVE) += resolver.o
> > >>>>>> obj-$(CONFIG_OF_OVERLAY) += overlay.o
> > >>>>>> obj-$(CONFIG_OF_NUMA) += of_numa.o
> > >>>>>> -
> > >>>>>> -ifdef CONFIG_KEXEC_FILE
> > >>>>>> -ifdef CONFIG_OF_FLATTREE
> > >>>>>> -obj-y += kexec.o
> > >>>>>> -endif
> > >>>>>> -endif
> > >>>>>> +obj-$(CONFIG_OF_KEXEC) += kexec.o
> > >>>>>> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> > >>>>> Is it possible to reuse CONFIG_HAVE_IMA_KEXEC here?
> > >>>>>
> > >>>>
> > >>>> For ppc64 CONFIG_HAVE_IMA_KEXEC is selected when CONFIG_KEXEC_FILE is
> > >>>> enabled.
> > >>>> So I don't see a problem in reusing CONFIG_HAVE_IMA_KEXEC for ppc.
> > >>>>
> > >>>> But for arm64, CONFIG_HAVE_IMA_KEXEC is enabled in the final patch in
> > >>>> the patch
> > >>>> set (the one for carrying forward IMA log across kexec for arm64).
> > >>>> arm64 calls
> > >>>> of_kexec_alloc_and_setup_fdt() prior to enabling CONFIG_HAVE_IMA_KEXEC
&
Re: [PATCH] powerpc: Mark arch_get_ima_policy() and is_ppc_trustedboot_enabled() as __init
On Fri, 2022-04-08 at 00:15 +1000, Michael Ellerman wrote: > We can mark arch_get_ima_policy() as __init because it's only caller > ima_init_arch_policy() is __init. We can then mark > is_ppc_trustedboot_enabled() __init because its only caller is > arch_get_ima_policy(). > > Signed-off-by: Michael Ellerman I assume you want to upstream this via power, Reviewed-by: Mimi Zohar thanks, Mimi
Re: [PATCH] powerpc: Mark arch_get_ima_policy() and is_ppc_trustedboot_enabled() as __init
On Fri, 2022-04-08 at 12:05 -0400, Mimi Zohar wrote: > On Fri, 2022-04-08 at 00:15 +1000, Michael Ellerman wrote: > > We can mark arch_get_ima_policy() as __init because it's only caller > > ima_init_arch_policy() is __init. We can then mark > > is_ppc_trustedboot_enabled() __init because its only caller is > > arch_get_ima_policy(). > > > > Signed-off-by: Michael Ellerman > > I assume you want to upstream this via power, > > Reviewed-by: Mimi Zohar Sorry, I just noticed that is_ppc_trustedboot_enabled() is also called by arch_ima_get_secureboot(). Mimi
Re: [PATCH] powerpc: Mark arch_get_ima_policy() and is_ppc_trustedboot_enabled() as __init
On Fri, 2022-04-08 at 13:31 -0400, Mimi Zohar wrote: > On Fri, 2022-04-08 at 12:05 -0400, Mimi Zohar wrote: > > On Fri, 2022-04-08 at 00:15 +1000, Michael Ellerman wrote: > > > We can mark arch_get_ima_policy() as __init because it's only caller > > > ima_init_arch_policy() is __init. We can then mark > > > is_ppc_trustedboot_enabled() __init because its only caller is > > > arch_get_ima_policy(). > > > > > > Signed-off-by: Michael Ellerman > > > > I assume you want to upstream this via power, > > > > Reviewed-by: Mimi Zohar > > Sorry, I just noticed that is_ppc_trustedboot_enabled() is also called > by arch_ima_get_secureboot(). Never mind, arch_ima_get_secureboot() calls is_ppc_secureboot_enabled(), not is_ppc_trustedboot_enabled().
Re: [PATCH v5] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime
On Thu, 2020-07-09 at 13:46 -0300, Bruno Meneguele wrote:
> APPRAISE_BOOTPARAM has been marked as dependent on !ARCH_POLICY in compile
> time, enforcing the appraisal whenever the kernel had the arch policy option
> enabled.
> However it breaks systems where the option is set but the system didn't
> boot in a "secure boot" platform. In this scenario, anytime an appraisal
> policy (i.e. ima_policy=appraisal_tcb) is used it will be forced, without
> giving the user the opportunity to label the filesystem, before enforcing
> integrity.
>
> Considering the ARCH_POLICY is only effective when secure boot is actually
> enabled this patch remove the compile time dependency and move it to a
> runtime decision, based on the secure boot state of that platform.
Perhaps we could simplify this patch description a bit?
The IMA_APPRAISE_BOOTPARAM config allows enabling different
"ima_appraise=" modes - log, fix, enforce - at run time, but not when
IMA architecture specific policies are enabled. This prevents
properly labeling the filesystem on systems where secure boot is
supported, but not enabled on the platform. Only when secure boot is
enabled, should these IMA appraise modes be disabled.
This patch removes the compile time dependency and makes it a runtime
decision, based on the secure boot state of that platform.
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index a9649b04b9f1..884de471b38a 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -19,6 +19,11 @@
> static int __init default_appraise_setup(c
> har *str)
> {
> #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> + if (arch_ima_get_secureboot()) {
> + pr_info("appraise boot param ignored: secure boot enabled");
Instead of a generic statement, is it possible to include the actual
option being denied? Perhaps something like: "Secure boot enabled,
ignoring %s boot command line option"
Mimi
> + return 1;
> + }
> +
> if (strncmp(str, "off", 3) == 0)
> ima_appraise = 0;
> else if (strncmp(str, "log", 3) == 0)
Re: [PATCH v5] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime
On Fri, 2020-07-10 at 15:34 -0300, Bruno Meneguele wrote:
> On Fri, Jul 10, 2020 at 03:03:38PM -0300, Bruno Meneguele wrote:
> > On Fri, Jul 10, 2020 at 01:23:24PM -0400, Mimi Zohar wrote:
> > > On Thu, 2020-07-09 at 13:46 -0300, Bruno Meneguele wrote:
> > > > APPRAISE_BOOTPARAM has been marked as dependent on !ARCH_POLICY in
> > > > compile
> > > > time, enforcing the appraisal whenever the kernel had the arch policy
> > > > option
> > > > enabled.
> > >
> > > > However it breaks systems where the option is set but the system didn't
> > > > boot in a "secure boot" platform. In this scenario, anytime an appraisal
> > > > policy (i.e. ima_policy=appraisal_tcb) is used it will be forced,
> > > > without
> > > > giving the user the opportunity to label the filesystem, before
> > > > enforcing
> > > > integrity.
> > > >
> > > > Considering the ARCH_POLICY is only effective when secure boot is
> > > > actually
> > > > enabled this patch remove the compile time dependency and move it to a
> > > > runtime decision, based on the secure boot state of that platform.
> > >
> > > Perhaps we could simplify this patch description a bit?
> > >
> > > The IMA_APPRAISE_BOOTPARAM config allows enabling different
> > > "ima_appraise=" modes - log, fix, enforce - at run time, but not when
> > > IMA architecture specific policies are enabled. This prevents
> > > properly labeling the filesystem on systems where secure boot is
> > > supported, but not enabled on the platform. Only when secure boot is
> > > enabled, should these IMA appraise modes be disabled.
> > >
> > > This patch removes the compile time dependency and makes it a runtime
> > > decision, based on the secure boot state of that platform.
> > >
> >
> > Sounds good to me.
> >
> > >
> > >
> > > > diff --git a/security/integrity/ima/ima_appraise.c
> > > > b/security/integrity/ima/ima_appraise.c
> > > > index a9649b04b9f1..884de471b38a 100644
> > > > --- a/security/integrity/ima/ima_appraise.c
> > > > +++ b/security/integrity/ima/ima_appraise.c
> > > > @@ -19,6 +19,11 @@
> > > > static int __init default_appraise_setup(c
> > >
> > > > har *str)
> > > > {
> > > > #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> > > > + if (arch_ima_get_secureboot()) {
> > > > + pr_info("appraise boot param ignored: secure boot
> > > > enabled");
> > >
> > > Instead of a generic statement, is it possible to include the actual
> > > option being denied? Perhaps something like: "Secure boot enabled,
> > > ignoring %s boot command line option"
> > >
> > > Mimi
> > >
> >
> > Yes, sure.
> >
>
> Btw, would it make sense to first make sure we have a valid "str"
> option and not something random to print?
>
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index a9649b04b9f1..1f1175531d3e 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -25,6 +25,16 @@ static int __init default_appraise_setup(char *str)
> ima_appraise = IMA_APPRAISE_LOG;
> else if (strncmp(str, "fix", 3) == 0)
> ima_appraise = IMA_APPRAISE_FIX;
> + else
> + pr_info("invalid \"%s\" appraise option");
> +
> + if (arch_ima_get_secureboot()) {
> + if (!is_ima_appraise_enabled()) {
> + pr_info("Secure boot enabled: ignoring
> ima_appraise=%s boot parameter option",
> + str);
> + ima_appraise = IMA_APPRAISE_ENFORCE;
> + }
> + }
Providing feedback is probably a good idea. However, the
"arch_ima_get_secureboot" test can't come after setting
"ima_appraise."
Mimi
> #endif
> return 1;
> }
>
>
> The "else" there I think would make sense as well, at least to give the
> user some feedback about a possible mispelling of him (as a separate
> patch).
>
> And "if(!is_ima_appraise_enabled())" would avoid to print anything about
> "ignoring the option" to the user in case he explicitly set "enforce",
> which we know there isn't any real effect but is allowed and shown in
> kernel-parameters.txt.
>
> > Thanks!
> >
> > > > + return 1;
> > > > + }
> > > > +
> > > > if (strncmp(str, "off", 3) == 0)
> > > > ima_appraise = 0;
> > > > else if (strncmp(str, "log", 3) == 0)
> > >
> >
> > --
> > bmeneg
> > PGP Key: http://bmeneg.com/pubkey.txt
>
>
>
Re: [PATCH 18/20] Documentation: security/keys: eliminate duplicated word
On Tue, 2020-07-07 at 11:04 -0700, Randy Dunlap wrote: > Drop the doubled word "in". > > Signed-off-by: Randy Dunlap Reviewed-by: Mimi Zohar
Re: [PATCH v2] powerpc/pseries: detect secure and trusted boot state of the system.
On Tue, 2020-07-14 at 16:38 +1000, Daniel Axtens wrote:
> Hi Nayna,
>
> Thanks! Would you be able to fold in some of the information from my
> reply to v1 into the changelog? Until we have public PAPR release with
> it, that information is the extent of the public documentation. It would
> be good to get it into the git log rather than just floating around in
> the mail archives!
>
> A couple of small nits:
>
> > + if (enabled)
> > + goto out;
> > +
> > + if (!of_property_read_u32(of_root, "ibm,secure-boot", &secureboot)) {
> > + if (secureboot)
> > + enabled = (secureboot > 1) ? true : false;
>
> Your tests double up here - you don't need both the 'if' statement and
> the 'secureboot > 1' ternary operator.
>
> Just
>
> + if (!of_property_read_u32(of_root, "ibm,secure-boot", &secureboot)) {
> + enabled = (secureboot > 1) ? true : false;
>
> or even
>
> + if (!of_property_read_u32(of_root, "ibm,secure-boot", &secureboot)) {
> + enabled = (secureboot > 1);
>
> would work.
I haven't been following this thread, which might be the reason I'm
missing something here. The patch description should explain why the
test is for "(secureboot > 1)", rather than a fixed number.
thanks,
Mimi
Re: [PATCH v3] powerpc/pseries: detect secure and trusted boot state of the system.
On Wed, 2020-07-15 at 07:52 -0400, Nayna Jain wrote: > The device-tree property to check secure and trusted boot state is > different for guests(pseries) compared to baremetal(powernv). > > This patch updates the existing is_ppc_secureboot_enabled() and > is_ppc_trustedboot_enabled() functions to add support for pseries. > > The secureboot and trustedboot state are exposed via device-tree property: > /proc/device-tree/ibm,secure-boot and /proc/device-tree/ibm,trusted-boot > > The values of ibm,secure-boot under pseries are interpreted as: > > 0 - Disabled > 1 - Enabled in Log-only mode. This patch interprets this value as > disabled, since audit mode is currently not supported for Linux. > 2 - Enabled and enforced. > 3-9 - Enabled and enforcing; requirements are at the discretion of the > operating system. > > The values of ibm,trusted-boot under pseries are interpreted as: > 0 - Disabled > 1 - Enabled > > Signed-off-by: Nayna Jain > Reviewed-by: Daniel Axtens Thanks for updating the patch description. Reviewed-by: Mimi Zohar
Re: [PATCH v6] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime
On Mon, 2020-07-20 at 10:40 -0400, Nayna wrote:
> On 7/13/20 12:48 PM, Bruno Meneguele wrote:
> > The IMA_APPRAISE_BOOTPARAM config allows enabling different "ima_appraise="
> > modes - log, fix, enforce - at run time, but not when IMA architecture
> > specific policies are enabled. This prevents properly labeling the
> > filesystem on systems where secure boot is supported, but not enabled on the
> > platform. Only when secure boot is actually enabled should these IMA
> > appraise modes be disabled.
> >
> > This patch removes the compile time dependency and makes it a runtime
> > decision, based on the secure boot state of that platform.
> >
> > Test results as follows:
> >
> > -> x86-64 with secure boot enabled
> >
> > [0.015637] Kernel command line: <...> ima_policy=appraise_tcb
> > ima_appraise=fix
> > [0.015668] ima: Secure boot enabled: ignoring ima_appraise=fix boot
> > parameter option
> >
Is it common to have two colons in the same line? Is the colon being
used as a delimiter when parsing the kernel logs? Should the second
colon be replaced with a hyphen? (No need to repost. I'll fix it
up.)
> > -> powerpc with secure boot disabled
> >
> > [0.00] Kernel command line: <...> ima_policy=appraise_tcb
> > ima_appraise=fix
> > [0.00] Secure boot mode disabled
> >
> > -> Running the system without secure boot and with both options set:
> >
> > CONFIG_IMA_APPRAISE_BOOTPARAM=y
> > CONFIG_IMA_ARCH_POLICY=y
> >
> > Audit prompts "missing-hash" but still allow execution and, consequently,
> > filesystem labeling:
> >
> > type=INTEGRITY_DATA msg=audit(07/09/2020 12:30:27.778:1691) : pid=4976
> > uid=root auid=root ses=2
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=appraise_data
> > cause=missing-hash comm=bash name=/usr/bin/evmctl dev="dm-0" ino=493150
> > res=no
> >
> > Cc: sta...@vger.kernel.org
> > Fixes: d958083a8f64 ("x86/ima: define arch_get_ima_policy() for x86")
> > Signed-off-by: Bruno Meneguele
>
>
> Reviewed-by: Nayna Jain
> Tested-by: Nayna Jain
Thanks, Nayna.
Mimi
Re: [PATCH v6] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime
On Mon, 2020-07-20 at 12:38 -0300, Bruno Meneguele wrote: > On Mon, Jul 20, 2020 at 10:56:55AM -0400, Mimi Zohar wrote: > > On Mon, 2020-07-20 at 10:40 -0400, Nayna wrote: > > > On 7/13/20 12:48 PM, Bruno Meneguele wrote: > > > > The IMA_APPRAISE_BOOTPARAM config allows enabling different > > > > "ima_appraise=" > > > > modes - log, fix, enforce - at run time, but not when IMA architecture > > > > specific policies are enabled. This prevents properly labeling the > > > > filesystem on systems where secure boot is supported, but not enabled > > > > on the > > > > platform. Only when secure boot is actually enabled should these IMA > > > > appraise modes be disabled. > > > > > > > > This patch removes the compile time dependency and makes it a runtime > > > > decision, based on the secure boot state of that platform. > > > > > > > > Test results as follows: > > > > > > > > -> x86-64 with secure boot enabled > > > > > > > > [0.015637] Kernel command line: <...> ima_policy=appraise_tcb > > > > ima_appraise=fix > > > > [0.015668] ima: Secure boot enabled: ignoring ima_appraise=fix boot > > > > parameter option > > > > > > > > Is it common to have two colons in the same line? Is the colon being > > used as a delimiter when parsing the kernel logs? Should the second > > colon be replaced with a hyphen? (No need to repost. I'll fix it > > up.) > > > > AFAICS it has been used without any limitations, e.g: > > PM: hibernation: Registered nosave memory: [mem 0x-0x0fff] > clocksource: hpet: mask: 0x max_cycles: 0x, max_idle_ns: > 133484873504 ns > microcode: CPU0: patch_level=0x08701013 > Lockdown: modprobe: unsigned module loading is restricted; see man > kernel_lockdown.7 > ... > > I'd say we're fine using it. Ok. FYI, it's now in next-integrity. Mimi
Re: [PATCH 0/3] KEXEC_SIG with appended signature
On Fri, 2021-11-19 at 12:18 +0100, Michal Suchánek wrote:
> Maybe I was not clear enough. If you happen to focus on an architecture
> that supports IMA fully it's great.
>
> My point of view is maintaining multiple architectures. Both end users
> and people conecerend with security are rarely familiar with
> architecture specifics. Portability of documentation and debugging
> instructions across architectures is a concern.
>
> IMA has large number of options with varying availablitily across
> architectures for no apparent reason. The situation is complex and hard
> to grasp.
IMA measures, verifies, and audits the integrity of files based on a
system wide policy. The known "good" integrity value may be stored in
the security.ima xattr or more recently as an appended signature.
With both IMA kexec appraise and measurement policy rules, not only is
the kernel image signature verified and the file hash included in the
IMA measurement list, but the signature used to verify the integrity of
the kexec kernel image is also included in the IMA measurement list
(ima_template=ima-sig).
Even without PECOFF support in IMA, IMA kexec measurement policy rules
can be defined to supplement the KEXEC_SIG signature verfication.
>
> In comparison the *_SIG options are widely available. The missing
> support for KEXEC_SIG on POWER is trivial to add by cut&paste from s390.
> With that all the documentation that exists already is also trivially
> applicable to POWER. Any additional code cleanup is a bonus but not
> really needed to enable the kexec lockdown on POWER.
Before lockdown was upstreamed, Matthew made sure that IMA signature
verification could co-exist. Refer to commit 29d3c1c8dfe7 ("kexec:
Allow kexec_file() with appropriate IMA policy when locked down"). If
there is a problem with the downstream kexec lockdown patches, they
should be fixed.
The kexec kselftest might provide some insight into how the different
signature verification methods and lockdown co-exist.
As for adding KEXEC_SIG appended signature support on PowerPC based on
the s390 code, it sounds reasonable.
thanks,
Mimi
Re: [PATCH 0/3] KEXEC_SIG with appended signature
On Wed, 2021-11-24 at 12:09 +0100, Philipp Rudo wrote: > Now Michal wants to adapt KEXEC_SIG for ppc too so distros can rely on all > architectures using the same mechanism and thus reduce maintenance cost. > On the way there he even makes some absolutely reasonable improvements > for everybody. > > Why is that so controversial? What is the real problem that should be > discussed here? Nothing is controversial with what Michal wants to do. I've already said, "As for adding KEXEC_SIG appended signature support on PowerPC based on the s390 code, it sounds reasonable." thanks, Mimi
Re: [PATCH v15 09/10] arm64: Call kmalloc() to allocate DTB buffer
Hi Ard,
On Fri, 2021-01-15 at 09:30 -0800, Lakshmi Ramasubramanian wrote:
> create_dtb() function allocates kernel virtual memory for
> the device tree blob (DTB). This is not consistent with other
> architectures, such as powerpc, which calls kmalloc() for allocating
> memory for the DTB.
>
> Call kmalloc() to allocate memory for the DTB, and kfree() to free
> the allocated memory.
The vmalloc() function description says, "vmalloc - allocate virtually
contiguous memory". I'd appreciate your reviewing this patch, in
particular, which replaces vmalloc() with kmalloc().
thanks,
Mimi
>
> Co-developed-by: Prakhar Srivastava
> Signed-off-by: Prakhar Srivastava
> Signed-off-by: Lakshmi Ramasubramanian
> ---
> arch/arm64/kernel/machine_kexec_file.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/kernel/machine_kexec_file.c
> b/arch/arm64/kernel/machine_kexec_file.c
> index 7de9c47dee7c..51c40143d6fa 100644
> --- a/arch/arm64/kernel/machine_kexec_file.c
> +++ b/arch/arm64/kernel/machine_kexec_file.c
> @@ -29,7 +29,7 @@ const struct kexec_file_ops * const kexec_file_loaders[] = {
>
> int arch_kimage_file_post_load_cleanup(struct kimage *image)
> {
> - vfree(image->arch.dtb);
> + kfree(image->arch.dtb);
> image->arch.dtb = NULL;
>
> vfree(image->arch.elf_headers);
> @@ -59,19 +59,21 @@ static int create_dtb(struct kimage *image,
> + cmdline_len + DTB_EXTRA_SPACE;
>
> for (;;) {
> - buf = vmalloc(buf_size);
> + buf = kmalloc(buf_size, GFP_KERNEL);
> if (!buf)
> return -ENOMEM;
>
> /* duplicate a device tree blob */
> ret = fdt_open_into(initial_boot_params, buf, buf_size);
> - if (ret)
> + if (ret) {
> + kfree(buf);
> return -EINVAL;
> + }
>
> ret = of_kexec_setup_new_fdt(image, buf, initrd_load_addr,
>initrd_len, cmdline);
> if (ret) {
> - vfree(buf);
> + kfree(buf);
> if (ret == -ENOMEM) {
> /* unlikely, but just in case */
> buf_size += DTB_EXTRA_SPACE;
> @@ -217,6 +219,6 @@ int load_other_segments(struct kimage *image,
> return 0;
>
> out_err:
> - vfree(dtb);
> + kfree(dtb);
> return ret;
> }
Re: [PATCH v15 10/10] arm64: Add IMA log information in kimage used for kexec
On Wed, 2021-01-27 at 10:24 -0800, Lakshmi Ramasubramanian wrote:
> On 1/27/21 10:02 AM, Will Deacon wrote:
> > On Wed, Jan 27, 2021 at 09:56:53AM -0800, Lakshmi Ramasubramanian wrote:
> >> On 1/27/21 8:54 AM, Will Deacon wrote:
> >>> On Fri, Jan 15, 2021 at 09:30:17AM -0800, Lakshmi Ramasubramanian wrote:
> Address and size of the buffer containing the IMA measurement log need
> to be passed from the current kernel to the next kernel on kexec.
>
> Add address and size fields to "struct kimage_arch" for ARM64 platform
> to hold the address and size of the IMA measurement log buffer.
>
> Update CONFIG_KEXEC_FILE to select CONFIG_HAVE_IMA_KEXEC, if CONFIG_IMA
> is enabled, to indicate that the IMA measurement log information is
> present in the device tree for ARM64.
>
> Co-developed-by: Prakhar Srivastava
> Signed-off-by: Prakhar Srivastava
> Signed-off-by: Lakshmi Ramasubramanian
> Reviewed-by: Thiago Jung Bauermann
> ---
> arch/arm64/Kconfig | 1 +
> arch/arm64/include/asm/kexec.h | 5 +
> 2 files changed, 6 insertions(+)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 1d466addb078..ea7f7fe3dccd 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1094,6 +1094,7 @@ config KEXEC
> config KEXEC_FILE
> bool "kexec file based system call"
> select KEXEC_CORE
> +select HAVE_IMA_KEXEC if IMA
> help
> This is new version of kexec system call. This system call is
> file based and takes file descriptors as system call argument
> diff --git a/arch/arm64/include/asm/kexec.h
> b/arch/arm64/include/asm/kexec.h
> index d24b527e8c00..2bd19ccb6c43 100644
> --- a/arch/arm64/include/asm/kexec.h
> +++ b/arch/arm64/include/asm/kexec.h
> @@ -100,6 +100,11 @@ struct kimage_arch {
> void *elf_headers;
> unsigned long elf_headers_mem;
> unsigned long elf_headers_sz;
> +
> +#ifdef CONFIG_IMA_KEXEC
> +phys_addr_t ima_buffer_addr;
> +size_t ima_buffer_size;
> +#endif
> >>>
> >>> Why do these need to be in the arch structure instead of 'struct kimage'?
> >>>
> >>
> >> Currently, only powerpc and, with this patch set, arm64 have support for
> >> carrying forward IMA measurement list across kexec system call. The above
> >> fields are used for tracking IMA measurement list.
> >>
> >> Do you see a reason to move these fields to "struct kimage"?
> >
> > If they're gated on CONFIG_IMA_KEXEC, then it seems harmless for them to
> > be added to the shared structure. Or are you saying that there are
> > architectures which have CONFIG_IMA_KEXEC but do not want these fields?
> >
>
> As far as I know, there are no other architectures that define
> CONFIG_IMA_KEXEC, but do not use these fields.
Yes, CONFIG_IMA_KEXEC enables "carrying the IMA measurement list across
a soft boot". The only arch that currently carries the IMA
measurement across kexec is powerpc.
Mimi
Re: [v1 PATCH 1/2] Refactoring carrying over IMA measuremnet logs over Kexec.
Hi Prakhar, On Sun, 2020-06-07 at 16:33 -0700, Prakhar Srivastava wrote: > This patch moves the non-architecture specific code out of powerpc and > adds to security/ima. > Update the arm64 and powerpc kexec file load paths to carry the IMA > measurement > logs. >From your patch description, this patch should be broken up. Moving the non-architecture specific code out of powerpc should be one patch. Additional support should be in another patch. After each patch, the code should work properly. Before posting patches, please review them, making sure unnecessary/unwanted changes haven't crept in - commenting out code, moving code without removing the original code. thanks, Mimi
Re: [PATCH v10 03/12] PKCS#7: Introduce pkcs7_get_digest()
On Thu, 2019-04-18 at 00:51 -0300, Thiago Jung Bauermann wrote: > IMA will need to access the digest of the PKCS7 message (as calculated by > the kernel) before the signature is verified, so introduce > pkcs7_get_digest() for that purpose. > > Also, modify pkcs7_digest() to detect when the digest was already > calculated so that it doesn't have to do redundant work. Verifying that > sinfo->sig->digest isn't NULL is sufficient because both places which > allocate sinfo->sig (pkcs7_parse_message() and pkcs7_note_signed_info()) > use kzalloc() so sig->digest is always initialized to zero. > > Signed-off-by: Thiago Jung Bauermann > Cc: David Howells > Cc: Herbert Xu > Cc: "David S. Miller" Reviewed-by: Mimi Zohar
Re: [PATCH v10 06/12] ima: Use designated initializers for struct ima_event_data
On Thu, 2019-04-18 at 00:51 -0300, Thiago Jung Bauermann wrote:
> Designated initializers allow specifying only the members of the struct
> that need initialization. Non-mentioned members are initialized to zero.
>
> This makes the code a bit clearer (particularly in ima_add_boot_aggregate()
> and also allows adding a new member to the struct without having to update
> all struct initializations.
>
> Signed-off-by: Thiago Jung Bauermann
Reviewed-by: Mimi Zohar
> ---
> security/integrity/ima/ima_api.c | 11 +++
> security/integrity/ima/ima_init.c | 4 ++--
> 2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/security/integrity/ima/ima_api.c
> b/security/integrity/ima/ima_api.c
> index c7505fb122d4..0639d0631f2c 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -133,8 +133,9 @@ void ima_add_violation(struct file *file, const unsigned
> char *filename,
> {
> struct ima_template_entry *entry;
> struct inode *inode = file_inode(file);
> - struct ima_event_data event_data = {iint, file, filename, NULL, 0,
> - cause};
> + struct ima_event_data event_data = { .iint = iint, .file = file,
> + .filename = filename,
> + .violation = cause };
> int violation = 1;
> int result;
>
> @@ -284,8 +285,10 @@ void ima_store_measurement(struct integrity_iint_cache
> *iint,
> int result = -ENOMEM;
> struct inode *inode = file_inode(file);
> struct ima_template_entry *entry;
> - struct ima_event_data event_data = {iint, file, filename, xattr_value,
> - xattr_len, NULL};
> + struct ima_event_data event_data = { .iint = iint, .file = file,
> + .filename = filename,
> + .xattr_value = xattr_value,
> + .xattr_len = xattr_len };
> int violation = 0;
>
> if (iint->measured_pcrs & (0x1 << pcr))
> diff --git a/security/integrity/ima/ima_init.c
> b/security/integrity/ima/ima_init.c
> index 6c9295449751..ef6c3a26296e 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -49,8 +49,8 @@ static int __init ima_add_boot_aggregate(void)
> const char *audit_cause = "ENOMEM";
> struct ima_template_entry *entry;
> struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
> - struct ima_event_data event_data = {iint, NULL, boot_aggregate_name,
> - NULL, 0, NULL};
> + struct ima_event_data event_data = { .iint = iint,
> + .filename = boot_aggregate_name };
> int result = -ENOMEM;
> int violation = 0;
> struct {
Re: [PATCH v10 01/12] MODSIGN: Export module signature definitions
On Thu, 2019-04-18 at 00:51 -0300, Thiago Jung Bauermann wrote: > IMA will use the module_signature format for append signatures, so export > the relevant definitions and factor out the code which verifies that the > appended signature trailer is valid. > > Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it > and be able to use mod_check_sig() without having to depend on either > CONFIG_MODULE_SIG or CONFIG_MODULES. > > Signed-off-by: Thiago Jung Bauermann > Cc: Jessica Yu Just a couple minor questions/comments below. Reviewed-by: Mimi Zohar > --- < snip > > diff --git a/init/Kconfig b/init/Kconfig > index 4592bf7997c0..a71019553ee1 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -1906,7 +1906,7 @@ config MODULE_SRCVERSION_ALL > config MODULE_SIG > bool "Module signature verification" > depends on MODULES > - select SYSTEM_DATA_VERIFICATION > + select MODULE_SIG_FORMAT > help > Check modules for valid signatures upon load: the signature > is simply appended to the module. For more information see > @@ -2036,6 +2036,10 @@ config TRIM_UNUSED_KSYMS > > endif # MODULES > > +config MODULE_SIG_FORMAT > + def_bool n > + select SYSTEM_DATA_VERIFICATION Normally Kconfigs, in the same file, are defined before they are used. I'm not sure if that is required or just a convention. > config MODULES_TREE_LOOKUP > def_bool y > depends on PERF_EVENTS || TRACING > diff --git a/kernel/Makefile b/kernel/Makefile > index 6c57e78817da..d2f2488f80ab 100644 > --- a/kernel/Makefile > +++ b/kernel/Makefile > @@ -57,6 +57,7 @@ endif > obj-$(CONFIG_UID16) += uid16.o > obj-$(CONFIG_MODULES) += module.o > obj-$(CONFIG_MODULE_SIG) += module_signing.o > +obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signature.o > obj-$(CONFIG_KALLSYMS) += kallsyms.o > obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o > obj-$(CONFIG_CRASH_CORE) += crash_core.o > diff --git a/kernel/module.c b/kernel/module.c > index 985caa467aef..326ddeb364dd 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include > #include > #include > #include > diff --git a/kernel/module_signature.c b/kernel/module_signature.c > new file mode 100644 > index ..6d5e59f27f55 > --- /dev/null > +++ b/kernel/module_signature.c > @@ -0,0 +1,45 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Module signature checker > + * > + * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved. > + * Written by David Howells (dhowe...@redhat.com) > + */ > + > +#include > +#include > +#include > +#include > + > +/** > + * mod_check_sig - check that the given signature is sane > + * > + * @ms: Signature to check. > + * @file_len:Size of the file to which @ms is appended. "name" is missing. Mimi > + */
Re: [PATCH v10 02/12] PKCS#7: Refactor verify_pkcs7_signature()
On Thu, 2019-04-18 at 00:51 -0300, Thiago Jung Bauermann wrote: > IMA will need to verify a PKCS#7 signature which has already been parsed. > For this reason, factor out the code which does that from > verify_pkcs7_signature() into a new function which takes a struct > pkcs7_message instead of a data buffer. > > Signed-off-by: Thiago Jung Bauermann > Reviewed-by: Mimi Zohar > Cc: David Howells > Cc: David Woodhouse > Cc: Herbert Xu > Cc: "David S. Miller" Reviewed-by: Mimi Zohar
