2019-10-24 19:51:59

by Nayna Jain

[permalink] [raw]
Subject: [PATCH v9 0/8] powerpc: Enabling IMA arch specific secure boot policies

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/

Changelog:

v9:
* Includes feedbacks from Michael
* fix the missing of_node_put()
* Includes Mimi's feedbacks
* fix the policy show() function to display check_blacklist
* fix the other comment related and patch description
* add the example of blacklist in the Patch 7/8
Note: Patch 7/8 is giving errors when checkpatch.pl is run because
of the format of showing measurement record as part of the example. I am
not very sure if that can be fixed as we need to represent the
measurements as is.

v8:
* Updates the Patch Description as per Michael's and Mimi's feedback
* Includes feedbacks from Michael for the device tree and policies
* removes the arch-policy hack by defining three arrays.
* fixes related to device-tree calls
* other code specific feedbacks
* Includes feedbacks from Mimi on the blacklist
* generic blacklist function is modified than previous version
* other coding fixes

v7:
* Removes patch related to dt-bindings as per input from Rob Herring.
* fixes Patch 1/8 to use new device-tree updates as per Oliver
feedback to device-tree documentation in skiboot mailing list.
(https://lists.ozlabs.org/pipermail/skiboot/2019-September/015329.html)
* Includes feedbacks from Mimi, Thiago
* moves function get_powerpc_fw_sb_node() from Patch 1 to Patch 3
* fixes Patch 2/8 to use CONFIG_MODULE_SIG_FORCE.
* updates Patch description in Patch 5/8
* adds a new patch to add wrapper is_binary_blacklisted()
* removes the patch that deprecated permit_directio

v6:
* includes feedbacks from Michael Ellerman on the patchset v5
* removed email ids from comments
* add the doc for the device-tree
* renames the secboot.c to secure_boot.c and secboot.h to secure_boot.h
* other code specific fixes
* split the patches to differentiate between secureboot and trustedboot
state of the system
* adds the patches to support the blacklisting of the binary hash.

v5:
* secureboot state is now read via device tree entry rather than OPAL
secure variables
* ima arch policies are updated to use policy based template for
measurement rules

v4:
* Fixed the build issue as reported by Satheesh Rajendran.

v3:
* OPAL APIs in Patch 1 are updated to provide generic interface based on
key/keylen. This patchset updates kernel OPAL APIs to be compatible with
generic interface.
* Patch 2 is cleaned up to use new OPAL APIs.
* Since OPAL can support different types of backend which can vary in the
variable interpretation, the Patch 2 is updated to add a check for the
backend version
* OPAL API now expects consumer to first check the supported backend version
before calling other secvar OPAL APIs. This check is now added in patch 2.
* IMA policies in Patch 3 is updated to specify appended signature and
per policy template.
* The patches now are free of any EFIisms.

v2:

* Removed Patch 1: powerpc/include: Override unneeded early ioremap
functions
* Updated Subject line and patch description of the Patch 1 of this series
* Removed dependency of OPAL_SECVAR on EFI, CPU_BIG_ENDIAN and UCS2_STRING
* Changed OPAL APIs from static to non-static. Added opal-secvar.h for the
same
* Removed EFI hooks from opal_secvar.c
* Removed opal_secvar_get_next(), opal_secvar_enqueue() and
opal_query_variable_info() function
* get_powerpc_sb_mode() in secboot.c now directly calls OPAL Runtime API
rather than via EFI hooks.
* Fixed log messages in get_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

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 | 74 ++++++++++++++++++++++++++
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 | 63 ++++++++++++++--------
security/integrity/ima/ima_policy.c | 12 ++++-
security/integrity/integrity.h | 1 +
14 files changed, 291 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.20.1


2019-10-24 19:52:04

by Nayna Jain

[permalink] [raw]
Subject: [PATCH v9 1/8] powerpc: detect the secure boot mode of the system

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 <[email protected]>
---
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 000000000000..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 000000000000..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 <linux/types.h>
+#include <linux/of.h>
+#include <asm/secure_boot.h>
+
+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.20.1

2019-10-24 19:52:28

by Nayna Jain

[permalink] [raw]
Subject: [PATCH v9 5/8] ima: make process_buffer_measurement() generic

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 <[email protected]>
---
security/integrity/ima/ima.h | 3 ++
security/integrity/ima/ima_main.c | 51 ++++++++++++++++++++-----------
2 files changed, 36 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..fe0b704ffdeb 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,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;
+ }
+
+ 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);
+ return;
+ }
+ }

iint.ima_hash = &hash.hdr;
iint.ima_hash->algo = ima_hash_algo;
@@ -664,7 +683,7 @@ static void process_buffer_measurement(const void *buf, int size,
if (ret < 0)
goto out;

- ret = ima_alloc_init_template(&event_data, &entry, template_desc);
+ ret = ima_alloc_init_template(&event_data, &entry, template);
if (ret < 0)
goto out;

@@ -686,13 +705,9 @@ static void process_buffer_measurement(const void *buf, int size,
*/
void ima_kexec_cmdline(const void *buf, int size)
{
- u32 secid;
-
- if (buf && size != 0) {
- security_task_getsecid(current, &secid);
+ if (buf && size != 0)
process_buffer_measurement(buf, size, "kexec-cmdline",
- current_cred(), secid);
- }
+ KEXEC_CMDLINE, 0);
}

static int __init init_ima(void)
--
2.20.1

2019-10-24 19:54:39

by Nayna Jain

[permalink] [raw]
Subject: [PATCH v9 8/8] powerpc/ima: update ima arch policy to check for blacklist

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 <[email protected]>
Reviewed-by: Mimi Zohar <[email protected]>
---
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.20.1

2019-10-25 17:59:54

by Lakshmi Ramasubramanian

[permalink] [raw]
Subject: Re: [PATCH v9 5/8] ima: make process_buffer_measurement() generic

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) {
> + 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)?

thanks,
-lakshmi

2019-10-25 18:47:40

by Lakshmi Ramasubramanian

[permalink] [raw]
Subject: Re: [PATCH v9 1/8] powerpc: detect the secure boot mode of the system

On 10/23/2019 8:47 PM, Nayna Jain wrote:
> This patch defines a function to detect the secure boot state of a
> PowerNV system.

> +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;

Related to "goto out;" above:

Would of_find_compatible_node return NULL if the given node is not found?

If of_device_is_available returns false (say, because node is NULL or it
does not find the specified node) would it be correct to call of_node_put?

> +
> +out:
> + of_node_put(node);

-lakshmi

2019-10-25 20:45:54

by Nayna Jain

[permalink] [raw]
Subject: Re: [PATCH v9 1/8] powerpc: detect the secure boot mode of the system


On 10/24/19 12:26 PM, Lakshmi Ramasubramanian wrote:
> On 10/23/2019 8:47 PM, Nayna Jain wrote:
>> This patch defines a function to detect the secure boot state of a
>> PowerNV system.
>
>> +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;
>
> Related to "goto out;" above:
>
> Would of_find_compatible_node return NULL if the given node is not found?
>
> If of_device_is_available returns false (say, because node is NULL or
> it does not find the specified node) would it be correct to call
> of_node_put?

of_node_put() handles NULL.

Thanks & Regards,

     - Nayna

2019-10-25 20:47:50

by Nayna Jain

[permalink] [raw]
Subject: Re: [PATCH v9 5/8] ima: make process_buffer_measurement() generic


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) {
>> +        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.

Thanks & Regards,

    - Nayna

2019-10-25 20:47:53

by Lakshmi Ramasubramanian

[permalink] [raw]
Subject: Re: [PATCH v9 5/8] ima: make process_buffer_measurement() generic



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) {
>>> +        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.

thanks,
-lakshmi

2019-10-27 00:14:51

by Mimi Zohar

[permalink] [raw]
Subject: 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 

2019-10-28 20:07:35

by Mimi Zohar

[permalink] [raw]
Subject: 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

2019-10-30 15:23:09

by Lakshmi Ramasubramanian

[permalink] [raw]
Subject: Re: [PATCH v9 5/8] ima: make process_buffer_measurement() generic

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: <command line data>"

Similarly, if the buffer is blacklisted binary hash:

"blacklist hash: <data>".

If the buffer is key hash:

"<name of the keyring>: key data".

This would greatly help the consumer of the IMA log to know the type of
data represented in each IMA log entry.

thanks,
-lakshmi

2019-10-30 16:41:29

by Mimi Zohar

[permalink] [raw]
Subject: 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: <command line data>"
>
> Similarly, if the buffer is blacklisted binary hash:
>
> "blacklist hash: <data>".
>
> If the buffer is key hash:
>
> "<name of the keyring>: 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