Re: [PATCH v5 2/2] powerpc: Add support to initialize ima policy rules

[Nayna]

On 09/02/2019 07:52 AM, Michael Ellerman wrote:
> Hi Nayna,

Hi Michael,

> Some more comments below.
>
> Nayna Jain <nayna@xxxxxxxxxxxxx> writes:
> > POWER secure boot relies on the kernel IMA security subsystem to
> > perform the OS kernel image signature verification.
> Again this is just a design choice we've made, it's not specified
> anywhere or anything like that. And it only applies to bare metal secure
> boot, at least so far. AIUI.

Yes. I will make it consistent to use "PowerNV".

> > Since each secure
> > boot mode has different IMA policy requirements, dynamic definition of
> > the policy rules based on the runtime secure boot mode of the system is
> > required. On systems that support secure boot, but have it disabled,
> > only measurement policy rules of the kernel image and modules are
> > defined.
> It's probably worth mentioning that we intend to use this in our
> Linux-based boot loader, which uses kexec, and that's one of the reasons
> why we're particularly interested in defining the rules for kexec?

Yes. Agreed. I will update patch description to add this.

> > This patch defines the arch-specific implementation to retrieve the
> > secure boot mode of the system and accordingly configures the IMA policy
> > rules.
> >
> > This patch provides arch-specific IMA policies if PPC_SECURE_BOOT
> > config is enabled.
> >
> > Signed-off-by: Nayna Jain <nayna@xxxxxxxxxxxxx>
> > ---
> >   arch/powerpc/Kconfig           |  2 ++
> >   arch/powerpc/kernel/Makefile   |  2 +-
> >   arch/powerpc/kernel/ima_arch.c | 50 ++++++++++++++++++++++++++++++++++
> >   include/linux/ima.h            |  3 +-
> >   4 files changed, 55 insertions(+), 2 deletions(-)
> >   create mode 100644 arch/powerpc/kernel/ima_arch.c
> >
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index c902a39124dc..42109682b727 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -917,6 +917,8 @@ config PPC_SECURE_BOOT
> >   	bool
> >   	default n
> >   	depends on PPC64
> > +	depends on IMA
> > +	depends on IMA_ARCH_POLICY
> >   	help
> >   	  Linux on POWER with firmware secure boot enabled needs to define
> >   	  security policies to extend secure boot to the OS.This config
> > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> > index d310ebb4e526..520b1c814197 100644
> > --- a/arch/powerpc/kernel/Makefile
> > +++ b/arch/powerpc/kernel/Makefile
> > @@ -157,7 +157,7 @@ endif
> >   obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
> >   obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
> >   
> > -obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o
> > +obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o ima_arch.o
> >   
> >   # Disable GCOV, KCOV & sanitizers in odd or sensitive code
> >   GCOV_PROFILE_prom_init.o := n
> > diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
> > new file mode 100644
> > index 000000000000..ac90fac83338
> > --- /dev/null
> > +++ b/arch/powerpc/kernel/ima_arch.c
> > @@ -0,0 +1,50 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019 IBM Corporation
> > + * Author: Nayna Jain <nayna@xxxxxxxxxxxxx>
> > + *
> > + * ima_arch.c
> > + *      - initialize ima policies for PowerPC Secure Boot
> > + */
> > +
> > +#include <linux/ima.h>
> > +#include <asm/secboot.h>
> > +
> > +bool arch_ima_get_secureboot(void)
> > +{
> > +	return get_powerpc_secureboot();
> > +}
> > +
> > +/*
> > + * File signature verification is not needed, include only measurements
> > + */
> > +static const char *const default_arch_rules[] = {
> > +	"measure func=KEXEC_KERNEL_CHECK",
> > +	"measure func=MODULE_CHECK",
> > +	NULL
> > +};
> The rules above seem fairly self explanatory.
>
> > +
> > +/* Both file signature verification and measurements are needed */
> > +static const char *const sb_arch_rules[] = {
> > +	"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
> > +	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> > +#if IS_ENABLED(CONFIG_MODULE_SIG)
> > +	"measure func=MODULE_CHECK",
> > +#else
> > +	"measure func=MODULE_CHECK template=ima-modsig",
> > +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> > +#endif
> But these ones are not so obvious, at least to me who knows very little
> about IMA.
>
> Can you add a one line comment to each of the ones in here saying what
> it does and why we want it?

Sure.

> > +	NULL
> > +};
> > +
> > +/*
> > + * On PowerPC, file measurements are to be added to the IMA measurement list
> > + * irrespective of the secure boot state of the system.
> Why? Just because we think it's useful? Would be good to provide some
> further justification.

Sure. I will clarify this in the next version.

Thanks & Regards,
ÂÂÂÂÂÂÂ - Nayna