[PATCH v6 0/9] powerpc: Enabling IMA arch specific secure boot policies

[PATCH v6 0/9] powerpc: Enabling IMA arch specific secure boot policies

[Nayna Jain]

This patchset extends the previous version of the patchset[1] by adding
the support for checking against the binary blacklisted hashes.

IMA subsystem supports custom, built-in, arch-specific policies to define
the files to be measured and appraised. These policies are honored based
on the priority where arch-specific policies is the highest and custom
is the lowest.

PowerNV systems uses the linux based bootloader and kexec the Host OS.
It rely on IMA for signature verification of the kernel before doing the
kexec. This patchset adds support for powerpc arch specific ima policies
that are defined based on system's OS secureboot and trustedboot state.
The OS secureboot and trustedboot state are determined via device-tree
properties.

The verification needs to be done only for the binaries which are not
blacklisted. The kernel currently checks against the blacklisted keys.
However that results in blacklisting all the binaries that are signed by
that key. In order to prevent single binary from loading, it is required
to support checking against blacklisting of the binary hash. This patchset
adds the support in IMA to check against blacklisted hashes for the files
signed by appended signature.

[1] http://patchwork.ozlabs.org/cover/1149262/ 

Changelog:
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 (9):
  dt-bindings: ibm,secureboot: secure boot specific properties for
    PowerNV
  powerpc: detect the secure boot mode of the system
  powerpc: add support to initialize ima policy rules
  powerpc: detect the trusted boot state of the system
  powerpc/ima: add measurement rules to ima arch specific policy
  ima: make process_buffer_measurement() non-static
  ima: check against blacklisted hashes for files with modsig
  ima: deprecate permit_directio, instead use appraise_flag
  powerpc/ima: update ima arch policy to check for blacklist

 Documentation/ABI/testing/ima_policy          |  3 +-
 .../bindings/powerpc/ibm,secureboot.rst       | 76 +++++++++++++++
 .../devicetree/bindings/powerpc/secvar.rst    | 89 +++++++++++++++++
 arch/powerpc/Kconfig                          | 12 +++
 arch/powerpc/include/asm/secure_boot.h        | 37 +++++++
 arch/powerpc/kernel/Makefile                  |  2 +
 arch/powerpc/kernel/ima_arch.c                | 71 ++++++++++++++
 arch/powerpc/kernel/secure_boot.c             | 96 +++++++++++++++++++
 include/linux/ima.h                           |  3 +-
 security/integrity/ima/ima.h                  | 15 +++
 security/integrity/ima/ima_appraise.c         | 35 +++++++
 security/integrity/ima/ima_main.c             | 37 +++----
 security/integrity/ima/ima_policy.c           | 12 ++-
 security/integrity/integrity.h                |  1 +
 14 files changed, 468 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/powerpc/ibm,secureboot.rst
 create mode 100644 Documentation/devicetree/bindings/powerpc/secvar.rst
 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

[PATCH v6 1/9] dt-bindings: ibm, secureboot: secure boot specific properties for PowerNV

[Nayna Jain]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 7540 bytes --]

PowerNV represents both the firmware and Host OS secureboot state of the
system via device tree. This patch adds the documentation to give
the definition of the nodes and the properties.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 .../bindings/powerpc/ibm,secureboot.rst       | 76 ++++++++++++++++
 .../devicetree/bindings/powerpc/secvar.rst    | 89 +++++++++++++++++++
 2 files changed, 165 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/powerpc/ibm,secureboot.rst
 create mode 100644 Documentation/devicetree/bindings/powerpc/secvar.rst

diff --git a/Documentation/devicetree/bindings/powerpc/ibm,secureboot.rst b/Documentation/devicetree/bindings/powerpc/ibm,secureboot.rst
new file mode 100644
index 000000000000..03d32099d2eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/powerpc/ibm,secureboot.rst
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: GPL-2.0
+*** NOTE ***
+This document is copied from OPAL firmware
+(skiboot/doc/device-tree/ibm,secureboot.rst)
+************
+.. _device-tree/ibm,secureboot:
+
+ibm,secureboot
+==============
+
+The ``ìbm,secureboot`` node provides secure boot and trusted boot information
+up to the target OS. Further information can be found in :ref:`stb-overview`.
+
+Required properties
+-------------------
+
+.. code-block:: none
+
+    compatible:         Either one of the following values:
+
+                        ibm,secureboot-v1  :  The container-verification-code
+                                              is stored in a secure ROM memory.
+
+                        ibm,secureboot-v2  :  The container-verification-code
+                                              is stored in a reserved memory.
+                                              It described by the ibm,cvc child
+                                              node.
+
+                        ibm,secureboot-v3  :  The container-verification-code
+                                              is stored in a reserved memory.
+                                              It described by the ibm,cvc child
+                                              node. Secure variables are
+                                              supported. `secvar` node should
+                                              be created.
+
+    secure-enabled:     this property exists when the firmware stack is booting
+                        in secure mode (hardware secure boot jumper asserted).
+
+    trusted-enabled:    this property exists when the firmware stack is booting
+                        in trusted mode.
+
+    hw-key-hash:        hash of the three hardware public keys trusted by the
+                        platformw owner. This is used to verify if a firmware
+                        code is signed with trusted keys.
+
+    hw-key-hash-size:   hw-key-hash size
+
+    secvar:             this node is created if the platform supports secure
+                        variables. Contains information about the current
+                        secvar status, see 'secvar.rst'.
+
+Obsolete properties
+-------------------
+
+.. code-block:: none
+
+    hash-algo:          Superseded by the hw-key-hash-size property in
+                        'ibm,secureboot-v2'.
+
+Example
+-------
+
+.. code-block:: dts
+
+    ibm,secureboot {
+        compatible = "ibm,secureboot-v2";
+        secure-enabled;
+        trusted-enabled;
+        hw-key-hash-size = <0x40>;
+        hw-key-hash = <0x40d487ff 0x7380ed6a 0xd54775d5 0x795fea0d 0xe2f541fe
+                       0xa9db06b8 0x466a42a3 0x20e65f75 0xb4866546 0x0017d907
+                       0x515dc2a5 0xf9fc5095 0x4d6ee0c9 0xb67d219d 0xfb708535
+                       0x1d01d6d1>;
+        phandle = <0x100000fd>;
+        linux,phandle = <0x100000fd>;
+    };
diff --git a/Documentation/devicetree/bindings/powerpc/secvar.rst b/Documentation/devicetree/bindings/powerpc/secvar.rst
new file mode 100644
index 000000000000..47793ab9c2a7
--- /dev/null
+++ b/Documentation/devicetree/bindings/powerpc/secvar.rst
@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: GPL-2.0
+*** NOTE ***
+This document is copied from OPAL firmware
+(skiboot/doc/device-tree/secvar.rst)
+************
+.. _device-tree/ibm,secureboot/secvar:
+
+secvar
+======
+
+The ``secvar`` node provides secure variable information for the secure
+boot of the target OS.
+
+Required properties
+-------------------
+
+.. code-block:: none
+
+    compatible:         this property is set based on the current secure
+                        variable scheme as set by the platform.
+
+    status:             set to "fail" if the secure variables could not
+                        be initialized, validated, or some other
+                        catastrophic failure.
+
+    update-status:      contains the return code of the update queue
+                        process run during initialization. Signifies if
+                        updates were processed or not, and if there was
+                        an error. See table below
+
+    secure-mode:        a u64 bitfield set by the backend to determine
+                        what secure mode we should be in, and if host
+                        secure boot should be enforced.
+
+Example
+-------
+
+.. code-block:: dts
+
+    secvar {
+        compatible = "ibm,edk2-compat-v1";
+        status = "okay";
+        secure-mode = "1";
+    };
+
+Update Status
+-------------
+
+The update status property should be set by the backend driver to a value
+that best fits its error condtion. The following table defines the
+general intent of each error code, check backend specific documentation
+for more detail.
+
++-----------------+-----------------------------------------------+
+| update-status   | Generic Reason                                |
++-----------------|-----------------------------------------------+
+| OPAL_SUCCESS    | Updates were found and processed successfully |
++-----------------|-----------------------------------------------+
+| OPAL_EMPTY      | No updates were found, none processed         |
++-----------------|-----------------------------------------------+
+| OPAL_PARAMETER  | Unable to parse data in the update section    |
++-----------------|-----------------------------------------------+
+| OPAL_PERMISSION | Update failed to apply, possible auth failure |
++-----------------|-----------------------------------------------+
+| OPAL_HARDWARE   | Misc. storage-related error                   |
++-----------------|-----------------------------------------------+
+| OPAL_RESOURCE   | Out of space (somewhere)                      |
++-----------------|-----------------------------------------------+
+| OPAL_NO_MEM     | Out of memory                                 |
++-----------------+-----------------------------------------------+
+
+Secure Mode
+-----------
+
++-----------------------+------------------------+
+| backend specific-bits |      generic mode bits |
++-----------------------+------------------------+
+64                     32                        0
+
+The secure mode property should be set by the backend driver. The least
+significant 32 bits are reserved for generic modes, shared across all
+possible backends. The other 32 bits are open for backends to determine
+their own modes. Any kernel must be made aware of any custom modes.
+
+At the moment, only one general-purpose bit is defined:
+
+``#define SECVAR_SECURE_MODE_ENFORCING  0x1``
+
+which signals that a kernel should enforce host secure boot.
-- 
2.20.1

[PATCH v6 2/9] powerpc: detect the secure boot mode of the system

[Nayna Jain]

Secure boot on PowerNV defines different IMA policies based on the secure
boot state of the system.

This patch defines a function to detect the secure boot state of the
system.

The PPC_SECURE_BOOT config represents the base enablement of secureboot
on POWER.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/Kconfig                   | 10 ++++
 arch/powerpc/include/asm/secure_boot.h | 31 ++++++++++
 arch/powerpc/kernel/Makefile           |  2 +
 arch/powerpc/kernel/secure_boot.c      | 82 ++++++++++++++++++++++++++
 4 files changed, 125 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 77f6ebf97113..2c54beb29f1a 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -912,6 +912,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 needs to define security
+	  policies to extend secure boot to the OS. This config allows 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..4e8e2b08a993
--- /dev/null
+++ b/arch/powerpc/include/asm/secure_boot.h
@@ -0,0 +1,31 @@
+/* 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
+
+#define SECURE_BOOT_MASK	0xFFFFFFFF00000000
+
+bool is_powerpc_os_secureboot_enabled(void);
+int get_powerpc_os_sb_node(struct device_node **node);
+
+#else
+
+static inline bool is_powerpc_os_secureboot_enabled(void)
+{
+	return false;
+}
+
+static inline int get_powerpc_os_sb_node(struct device_node **node)
+{
+	return -ENOENT;
+}
+
+#endif
+#endif
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index ea0c69236789..875b0785a20e 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -157,6 +157,8 @@ endif
 obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
 obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
 
+obj-$(CONFIG_PPC_SECURE_BOOT)	+= secure_boot.o
+
 # 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..45ca19f5e836
--- /dev/null
+++ b/arch/powerpc/kernel/secure_boot.c
@@ -0,0 +1,82 @@
+// 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>
+
+static struct device_node *get_powerpc_fw_sb_node(void)
+{
+	return of_find_node_by_name(NULL, "ibm,secureboot");
+}
+
+bool is_powerpc_os_sb_supported(void)
+{
+	struct device_node *node = NULL;
+
+	node = get_powerpc_fw_sb_node();
+	if (node && of_device_is_compatible(node, "ibm,secureboot-v3"))
+		return true;
+
+	return false;
+}
+
+int get_powerpc_os_sb_node(struct device_node **node)
+{
+	struct device_node *fwsbnode;
+
+	if (!is_powerpc_os_sb_supported())
+		return -ENOTSUPP;
+
+	fwsbnode = get_powerpc_fw_sb_node();
+	if (!fwsbnode)
+		return -ENOENT;
+
+	*node = of_find_node_by_name(fwsbnode, "secvar");
+	if (*node)
+		return 0;
+
+	return -ENOENT;
+}
+
+bool is_powerpc_os_secureboot_enabled(void)
+{
+	struct device_node *node;
+	u64 sbmode = 0;
+	int rc;
+
+	rc = get_powerpc_os_sb_node(&node);
+	if (rc == -ENOTSUPP)
+		goto disabled;
+
+	/* Fail secure for any failure related to secvar */
+	if (rc) {
+		pr_err("Expected secure variables support, fail secure\n");
+		goto enabled;
+	}
+
+	if (!of_device_is_available(node)) {
+		pr_err("Secure variables support is in error state, fail secure\n");
+		goto enabled;
+	}
+
+	rc = of_property_read_u64(node, "os-secure-mode", &sbmode);
+	if (rc)
+		goto enabled;
+
+	sbmode = be64_to_cpu(sbmode);
+
+	/* checks for the secure mode enforcing bit */
+	if (!(sbmode & SECURE_BOOT_MASK))
+		goto disabled;
+
+enabled:
+	pr_info("secureboot mode enabled\n");
+	return true;
+
+disabled:
+	pr_info("secureboot mode disabled\n");
+	return false;
+}
-- 
2.20.1

[PATCH v6 3/9] powerpc: add support to initialize ima policy rules

[Nayna Jain]

PowerNV systems uses kernel based bootloader, thus its secure boot
implementation uses kernel IMA security subsystem to verify the kernel
before kexec. Since the verification policy might differ based on the
secure boot mode of the system, the policies are defined at runtime.

This patch implements the arch-specific support to define the IMA policy
rules based on the runtime secure boot mode of the system.

This patch provides arch-specific IMA policies if PPC_SECURE_BOOT
config is enabled.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/Kconfig           |  2 ++
 arch/powerpc/kernel/Makefile   |  2 +-
 arch/powerpc/kernel/ima_arch.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/ima.h            |  3 ++-
 4 files changed, 38 insertions(+), 2 deletions(-)
 create mode 100644 arch/powerpc/kernel/ima_arch.c

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 2c54beb29f1a..54eda07c74e5 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -916,6 +916,8 @@ config PPC_SECURE_BOOT
 	prompt "Enable secure boot support"
 	bool
 	depends on PPC_POWERNV
+	depends on IMA
+	depends on IMA_ARCH_POLICY
 	help
 	  Systems with firmware secure boot enabled needs to define security
 	  policies to extend secure boot to the OS. This config allows user
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 875b0785a20e..7156ac1fc956 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -157,7 +157,7 @@ endif
 obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
 obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
 
-obj-$(CONFIG_PPC_SECURE_BOOT)	+= secure_boot.o
+obj-$(CONFIG_PPC_SECURE_BOOT)	+= secure_boot.o ima_arch.o
 
 # Disable GCOV, KCOV & sanitizers in odd or sensitive code
 GCOV_PROFILE_prom_init.o := n
diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
new file mode 100644
index 000000000000..39401b67f19e
--- /dev/null
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain
+ */
+
+#include <linux/ima.h>
+#include <asm/secure_boot.h>
+
+bool arch_ima_get_secureboot(void)
+{
+	return is_powerpc_os_secureboot_enabled();
+}
+
+/* Defines IMA appraise rules for secureboot */
+static const char *const arch_rules[] = {
+	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+#if !IS_ENABLED(CONFIG_MODULE_SIG)
+	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+#endif
+	NULL
+};
+
+/*
+ * Returns the relevant IMA arch policies based on the system secureboot state.
+ */
+const char *const *arch_get_ima_policy(void)
+{
+	if (is_powerpc_os_secureboot_enabled())
+		return arch_rules;
+
+	return NULL;
+}
diff --git a/include/linux/ima.h b/include/linux/ima.h
index a20ad398d260..10af09b5b478 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -29,7 +29,8 @@ extern void ima_kexec_cmdline(const void *buf, int size);
 extern void ima_add_kexec_buffer(struct kimage *image);
 #endif
 
-#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390)
+#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \
+	|| defined(CONFIG_PPC_SECURE_BOOT)
 extern bool arch_ima_get_secureboot(void);
 extern const char * const *arch_get_ima_policy(void);
 #else
-- 
2.20.1

[PATCH v6 4/9] powerpc: detect the trusted boot state of the system

[Nayna Jain]

PowerNV systems enables the IMA measurement rules only if the
trusted boot is enabled on the system.

This patch adds the function to detect if the system has trusted
boot enabled.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/include/asm/secure_boot.h |  6 ++++++
 arch/powerpc/kernel/secure_boot.c      | 14 ++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/arch/powerpc/include/asm/secure_boot.h b/arch/powerpc/include/asm/secure_boot.h
index 4e8e2b08a993..192caaedbe7a 100644
--- a/arch/powerpc/include/asm/secure_boot.h
+++ b/arch/powerpc/include/asm/secure_boot.h
@@ -14,6 +14,7 @@
 
 bool is_powerpc_os_secureboot_enabled(void);
 int get_powerpc_os_sb_node(struct device_node **node);
+bool is_powerpc_trustedboot_enabled(void);
 
 #else
 
@@ -27,5 +28,10 @@ static inline int get_powerpc_os_sb_node(struct device_node **node)
 	return -ENOENT;
 }
 
+static inline bool is_powerpc_os_trustedboot_enabled(void)
+{
+	return false;
+}
+
 #endif
 #endif
diff --git a/arch/powerpc/kernel/secure_boot.c b/arch/powerpc/kernel/secure_boot.c
index 45ca19f5e836..9d452e1550ae 100644
--- a/arch/powerpc/kernel/secure_boot.c
+++ b/arch/powerpc/kernel/secure_boot.c
@@ -80,3 +80,17 @@ bool is_powerpc_os_secureboot_enabled(void)
 	pr_info("secureboot mode disabled\n");
 	return false;
 }
+
+bool is_powerpc_trustedboot_enabled(void)
+{
+	struct device_node *node;
+
+	node = get_powerpc_fw_sb_node();
+	if (node && (of_find_property(node, "trusted-enabled", NULL))) {
+		pr_info("trustedboot mode enabled\n");
+		return true;
+	}
+
+	pr_info("trustedboot mode disabled\n");
+	return false;
+}
-- 
2.20.1

[PATCH v6 5/9] powerpc/ima: add measurement rules to ima arch specific policy

[Nayna Jain]

This patch adds the measurement rules to the arch specific policies for the
systems with trusted boot.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 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.
+ */
 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;
 }
-- 
2.20.1

[PATCH v6 6/9] ima: make process_buffer_measurement() non static

[Nayna Jain]

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 <nayna@linux.ibm.com>
---
 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 19769bf5f6ab..9bf509217e8e 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -215,6 +215,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 79c01516211b..ae0c1bdc4eaf 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);
 	}
 }
 
-- 
2.20.1

[PATCH v6 7/9] ima: check against blacklisted hashes for files with modsig

[Nayna Jain]

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 <nayna@linux.ibm.com>
---
 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 <linux/magic.h>
 #include <linux/ima.h>
 #include <linux/evm.h>
+#include <keys/system_keyring.h>
 
 #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.
+ */
+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 */
+		if ((rc == -EKEYREJECTED) && (iint->flags & IMA_MEASURE))
+			process_buffer_measurement(digest, digestsize,
+						   "blacklisted-hash", pcr,
+						   template_desc);
+	}
+
+	return rc;
+}
+
 /*
  * ima_appraise_measurement - appraise file measurement
  *
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index ae0c1bdc4eaf..92c446045637 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -336,8 +336,12 @@ static int process_measurement(struct file *file, const struct cred *cred,
 				      template_desc);
 	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
 		inode_lock(inode);
-		rc = ima_appraise_measurement(func, iint, file, pathname,
-					      xattr_value, xattr_len, modsig);
+		rc = ima_blacklist_measurement(iint, modsig, action, pcr,
+					       template_desc);
+		if (rc != -EKEYREJECTED)
+			rc = ima_appraise_measurement(func, iint, file,
+						      pathname, xattr_value,
+						      xattr_len, modsig);
 		inode_unlock(inode);
 		if (!rc)
 			rc = mmap_violation_check(func, file, &pathbuf,
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 4badc4fcda98..ad3b3af69460 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -765,8 +765,8 @@ enum {
 	Opt_fsuuid, Opt_uid_eq, Opt_euid_eq, Opt_fowner_eq,
 	Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
 	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
-	Opt_appraise_type, Opt_permit_directio,
-	Opt_pcr, Opt_template, Opt_err
+	Opt_appraise_type, Opt_appraise_flag,
+	Opt_permit_directio, Opt_pcr, Opt_template, Opt_err
 };
 
 static const match_table_t policy_tokens = {
@@ -798,6 +798,7 @@ static const match_table_t policy_tokens = {
 	{Opt_euid_lt, "euid<%s"},
 	{Opt_fowner_lt, "fowner<%s"},
 	{Opt_appraise_type, "appraise_type=%s"},
+	{Opt_appraise_flag, "appraise_flag=%s"},
 	{Opt_permit_directio, "permit_directio"},
 	{Opt_pcr, "pcr=%s"},
 	{Opt_template, "template=%s"},
@@ -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;
 		case Opt_permit_directio:
 			entry->flags |= IMA_PERMIT_DIRECTIO;
 			break;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index d9323d31a3a8..73fc286834d7 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -32,6 +32,7 @@
 #define EVM_IMMUTABLE_DIGSIG	0x08000000
 #define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
 #define IMA_MODSIG_ALLOWED	0x20000000
+#define IMA_CHECK_BLACKLIST	0x40000000
 
 #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
 				 IMA_HASH | IMA_APPRAISE_SUBMASK)
-- 
2.20.1

[PATCH v6 8/9] ima: deprecate permit_directio, instead use appraise_flag

[Nayna Jain]

This patch deprecates the existing permit_directio flag, instead adds
it as possible value to appraise_flag parameter.
For eg.
appraise_flag=permit_directio

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 Documentation/ABI/testing/ima_policy | 4 ++--
 security/integrity/ima/ima_policy.c  | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 4c97afcc0f3c..9a2a140dc561 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -24,8 +24,8 @@ Description:
 				[euid=] [fowner=] [fsname=]]
 			lsm:	[[subj_user=] [subj_role=] [subj_type=]
 				 [obj_user=] [obj_role=] [obj_type=]]
-			option:	[[appraise_type=]] [template=] [permit_directio]
-				[appraise_flag=[check_blacklist]]
+			option:	[[appraise_type=]] [template=] [permit_directio(deprecated)]
+				[appraise_flag=[check_blacklist]|[permit_directio]]
 		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_policy.c b/security/integrity/ima/ima_policy.c
index ad3b3af69460..d9df54c75d46 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1177,6 +1177,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			ima_log_string(ab, "appraise_flag", args[0].from);
 			if (strstr(args[0].from, "blacklist"))
 				entry->flags |= IMA_CHECK_BLACKLIST;
+			if (strstr(args[0].from, "permit_directio"))
+				entry->flags |= IMA_PERMIT_DIRECTIO;
 			break;
 		case Opt_permit_directio:
 			entry->flags |= IMA_PERMIT_DIRECTIO;
-- 
2.20.1

[PATCH v6 9/9] powerpc/ima: update ima arch policy to check for blacklist

[Nayna Jain]

This patch updates the arch specific policies for PowernV systems
to add check against blacklisted hashes before doing the verification.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 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 77c61b142042..3f57433c0824 100644
--- a/arch/powerpc/kernel/ima_arch.c
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -24,9 +24,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)
-	"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

Re: [PATCH v6 5/9] powerpc/ima: add measurement rules to ima arch specific policy

[Mimi Zohar]

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 <nayna@linux.ibm.com>

Minor comment correction below.

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

> ---
>  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 3/9] powerpc: add support to initialize ima policy rules

[Thiago Jung Bauermann]

Hello,

Nayna Jain <nayna@linux.ibm.com> writes:

> PowerNV systems uses kernel based bootloader, thus its secure boot
> implementation uses kernel IMA security subsystem to verify the kernel
> before kexec. Since the verification policy might differ based on the
> secure boot mode of the system, the policies are defined at runtime.
>
> This patch implements the arch-specific support to define the IMA policy
> rules based on the runtime secure boot mode of the system.
>
> This patch provides arch-specific IMA policies if PPC_SECURE_BOOT
> config is enabled.
>
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> ---
>  arch/powerpc/Kconfig           |  2 ++
>  arch/powerpc/kernel/Makefile   |  2 +-
>  arch/powerpc/kernel/ima_arch.c | 33 +++++++++++++++++++++++++++++++++
>  include/linux/ima.h            |  3 ++-
>  4 files changed, 38 insertions(+), 2 deletions(-)
>  create mode 100644 arch/powerpc/kernel/ima_arch.c
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 2c54beb29f1a..54eda07c74e5 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -916,6 +916,8 @@ config PPC_SECURE_BOOT
>  	prompt "Enable secure boot support"
>  	bool
>  	depends on PPC_POWERNV
> +	depends on IMA
> +	depends on IMA_ARCH_POLICY
>  	help
>  	  Systems with firmware secure boot enabled needs to define security
>  	  policies to extend secure boot to the OS. This config allows user
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index 875b0785a20e..7156ac1fc956 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -157,7 +157,7 @@ endif
>  obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
>  obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
>
> -obj-$(CONFIG_PPC_SECURE_BOOT)	+= secure_boot.o
> +obj-$(CONFIG_PPC_SECURE_BOOT)	+= secure_boot.o ima_arch.o
>
>  # Disable GCOV, KCOV & sanitizers in odd or sensitive code
>  GCOV_PROFILE_prom_init.o := n
> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
> new file mode 100644
> index 000000000000..39401b67f19e
> --- /dev/null
> +++ b/arch/powerpc/kernel/ima_arch.c
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain
> + */
> +
> +#include <linux/ima.h>
> +#include <asm/secure_boot.h>
> +
> +bool arch_ima_get_secureboot(void)
> +{
> +	return is_powerpc_os_secureboot_enabled();
> +}
> +
> +/* Defines IMA appraise rules for secureboot */
> +static const char *const arch_rules[] = {
> +	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> +#if !IS_ENABLED(CONFIG_MODULE_SIG)
> +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> +#endif
> +	NULL
> +};
> +
> +/*
> + * Returns the relevant IMA arch policies based on the system secureboot state.
> + */
> +const char *const *arch_get_ima_policy(void)
> +{
> +	if (is_powerpc_os_secureboot_enabled())
> +		return arch_rules;
> +
> +	return NULL;
> +}

If CONFIG_MODULE_SIG is enabled but module signatures aren't enforced,
then IMA won't enforce module signature either. x86's
arch_get_ima_policy() calls set_module_sig_enforced(). Doesn't the
powerpc version need to do that as well?

On the flip side, if module signatures are enforced by the module
subsystem then IMA will verify the signature a second time since there's
no sharing of signature verification results between the module
subsystem and IMA (this was observed by Mimi).

IMHO this is a minor issue, since module loading isn't a hot path and
the duplicate work shouldn't impact anything. But it could be avoided by
having a NULL entry in arch_rules, which arch_get_ima_policy() would
dynamically update with the "appraise func=MODULE_CHECK" rule if
is_module_sig_enforced() is true.

--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v6 1/9] dt-bindings: ibm, secureboot: secure boot specific properties for PowerNV

[Rob Herring]

On Fri, Sep 27, 2019 at 10:25:52AM -0400, Nayna Jain wrote:
> PowerNV represents both the firmware and Host OS secureboot state of the
> system via device tree. This patch adds the documentation to give
> the definition of the nodes and the properties.
> 
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> ---
>  .../bindings/powerpc/ibm,secureboot.rst       | 76 ++++++++++++++++
>  .../devicetree/bindings/powerpc/secvar.rst    | 89 +++++++++++++++++++
>  2 files changed, 165 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/powerpc/ibm,secureboot.rst
>  create mode 100644 Documentation/devicetree/bindings/powerpc/secvar.rst
> 
> diff --git a/Documentation/devicetree/bindings/powerpc/ibm,secureboot.rst b/Documentation/devicetree/bindings/powerpc/ibm,secureboot.rst
> new file mode 100644
> index 000000000000..03d32099d2eb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/powerpc/ibm,secureboot.rst
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: GPL-2.0

Not the right form for reST files.

> +*** NOTE ***
> +This document is copied from OPAL firmware
> +(skiboot/doc/device-tree/ibm,secureboot.rst)

Why copy into the kernel?

Plus, the bindings are in the process of being converted to schema. What 
would I do with these files?

> +************
> +.. _device-tree/ibm,secureboot:
> +
> +ibm,secureboot
> +==============
> +
> +The ``ìbm,secureboot`` node provides secure boot and trusted boot information
> +up to the target OS. Further information can be found in :ref:`stb-overview`.
> +
> +Required properties
> +-------------------
> +
> +.. code-block:: none
> +
> +    compatible:         Either one of the following values:
> +
> +                        ibm,secureboot-v1  :  The container-verification-code
> +                                              is stored in a secure ROM memory.
> +
> +                        ibm,secureboot-v2  :  The container-verification-code
> +                                              is stored in a reserved memory.
> +                                              It described by the ibm,cvc child
> +                                              node.
> +
> +                        ibm,secureboot-v3  :  The container-verification-code
> +                                              is stored in a reserved memory.
> +                                              It described by the ibm,cvc child
> +                                              node. Secure variables are
> +                                              supported. `secvar` node should
> +                                              be created.
> +
> +    secure-enabled:     this property exists when the firmware stack is booting
> +                        in secure mode (hardware secure boot jumper asserted).
> +
> +    trusted-enabled:    this property exists when the firmware stack is booting
> +                        in trusted mode.
> +
> +    hw-key-hash:        hash of the three hardware public keys trusted by the
> +                        platformw owner. This is used to verify if a firmware
> +                        code is signed with trusted keys.
> +
> +    hw-key-hash-size:   hw-key-hash size
> +
> +    secvar:             this node is created if the platform supports secure
> +                        variables. Contains information about the current
> +                        secvar status, see 'secvar.rst'.
> +
> +Obsolete properties
> +-------------------
> +
> +.. code-block:: none
> +
> +    hash-algo:          Superseded by the hw-key-hash-size property in
> +                        'ibm,secureboot-v2'.
> +
> +Example
> +-------
> +
> +.. code-block:: dts
> +
> +    ibm,secureboot {
> +        compatible = "ibm,secureboot-v2";
> +        secure-enabled;
> +        trusted-enabled;
> +        hw-key-hash-size = <0x40>;
> +        hw-key-hash = <0x40d487ff 0x7380ed6a 0xd54775d5 0x795fea0d 0xe2f541fe
> +                       0xa9db06b8 0x466a42a3 0x20e65f75 0xb4866546 0x0017d907
> +                       0x515dc2a5 0xf9fc5095 0x4d6ee0c9 0xb67d219d 0xfb708535
> +                       0x1d01d6d1>;
> +        phandle = <0x100000fd>;
> +        linux,phandle = <0x100000fd>;
> +    };
> diff --git a/Documentation/devicetree/bindings/powerpc/secvar.rst b/Documentation/devicetree/bindings/powerpc/secvar.rst
> new file mode 100644
> index 000000000000..47793ab9c2a7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/powerpc/secvar.rst
> @@ -0,0 +1,89 @@
> +# SPDX-License-Identifier: GPL-2.0
> +*** NOTE ***
> +This document is copied from OPAL firmware
> +(skiboot/doc/device-tree/secvar.rst)
> +************
> +.. _device-tree/ibm,secureboot/secvar:
> +
> +secvar
> +======
> +
> +The ``secvar`` node provides secure variable information for the secure
> +boot of the target OS.
> +
> +Required properties
> +-------------------
> +
> +.. code-block:: none
> +
> +    compatible:         this property is set based on the current secure
> +                        variable scheme as set by the platform.
> +
> +    status:             set to "fail" if the secure variables could not
> +                        be initialized, validated, or some other
> +                        catastrophic failure.
> +
> +    update-status:      contains the return code of the update queue
> +                        process run during initialization. Signifies if
> +                        updates were processed or not, and if there was
> +                        an error. See table below
> +
> +    secure-mode:        a u64 bitfield set by the backend to determine
> +                        what secure mode we should be in, and if host
> +                        secure boot should be enforced.
> +
> +Example
> +-------
> +
> +.. code-block:: dts
> +
> +    secvar {
> +        compatible = "ibm,edk2-compat-v1";
> +        status = "okay";
> +        secure-mode = "1";
> +    };
> +
> +Update Status
> +-------------
> +
> +The update status property should be set by the backend driver to a value
> +that best fits its error condtion. The following table defines the
> +general intent of each error code, check backend specific documentation
> +for more detail.
> +
> ++-----------------+-----------------------------------------------+
> +| update-status   | Generic Reason                                |
> ++-----------------|-----------------------------------------------+
> +| OPAL_SUCCESS    | Updates were found and processed successfully |
> ++-----------------|-----------------------------------------------+
> +| OPAL_EMPTY      | No updates were found, none processed         |
> ++-----------------|-----------------------------------------------+
> +| OPAL_PARAMETER  | Unable to parse data in the update section    |
> ++-----------------|-----------------------------------------------+
> +| OPAL_PERMISSION | Update failed to apply, possible auth failure |
> ++-----------------|-----------------------------------------------+
> +| OPAL_HARDWARE   | Misc. storage-related error                   |
> ++-----------------|-----------------------------------------------+
> +| OPAL_RESOURCE   | Out of space (somewhere)                      |
> ++-----------------|-----------------------------------------------+
> +| OPAL_NO_MEM     | Out of memory                                 |
> ++-----------------+-----------------------------------------------+
> +
> +Secure Mode
> +-----------
> +
> ++-----------------------+------------------------+
> +| backend specific-bits |      generic mode bits |
> ++-----------------------+------------------------+
> +64                     32                        0
> +
> +The secure mode property should be set by the backend driver. The least
> +significant 32 bits are reserved for generic modes, shared across all
> +possible backends. The other 32 bits are open for backends to determine
> +their own modes. Any kernel must be made aware of any custom modes.
> +
> +At the moment, only one general-purpose bit is defined:
> +
> +``#define SECVAR_SECURE_MODE_ENFORCING  0x1``
> +
> +which signals that a kernel should enforce host secure boot.
> -- 
> 2.20.1
> 

Re: [PATCH v6 3/9] powerpc: add support to initialize ima policy rules

[Nayna]

On 09/30/2019 09:04 PM, Thiago Jung Bauermann wrote:
> Hello,

Hi,

>
>> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
>> new file mode 100644
>> index 000000000000..39401b67f19e
>> --- /dev/null
>> +++ b/arch/powerpc/kernel/ima_arch.c
>> @@ -0,0 +1,33 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 IBM Corporation
>> + * Author: Nayna Jain
>> + */
>> +
>> +#include <linux/ima.h>
>> +#include <asm/secure_boot.h>
>> +
>> +bool arch_ima_get_secureboot(void)
>> +{
>> +	return is_powerpc_os_secureboot_enabled();
>> +}
>> +
>> +/* Defines IMA appraise rules for secureboot */
>> +static const char *const arch_rules[] = {
>> +	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
>> +#if !IS_ENABLED(CONFIG_MODULE_SIG)
>> +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
>> +#endif
>> +	NULL
>> +};
>> +
>> +/*
>> + * Returns the relevant IMA arch policies based on the system secureboot state.
>> + */
>> +const char *const *arch_get_ima_policy(void)
>> +{
>> +	if (is_powerpc_os_secureboot_enabled())
>> +		return arch_rules;
>> +
>> +	return NULL;
>> +}
> If CONFIG_MODULE_SIG is enabled but module signatures aren't enforced,
> then IMA won't enforce module signature either. x86's
> arch_get_ima_policy() calls set_module_sig_enforced(). Doesn't the
> powerpc version need to do that as well?
>
> On the flip side, if module signatures are enforced by the module
> subsystem then IMA will verify the signature a second time since there's
> no sharing of signature verification results between the module
> subsystem and IMA (this was observed by Mimi).
>
> IMHO this is a minor issue, since module loading isn't a hot path and
> the duplicate work shouldn't impact anything. But it could be avoided by
> having a NULL entry in arch_rules, which arch_get_ima_policy() would
> dynamically update with the "appraise func=MODULE_CHECK" rule if
> is_module_sig_enforced() is true.

Thanks Thiago for reviewing.  I am wondering that this will give two 
meanings for NULL. Can we do something like below, there are possibly 
two options ?

1. Set IMA_APPRAISED in the iint->flags if is_module_sig_enforced().

OR

2. Let ima_get_action() check for is_module_sig_enforced() when policy 
is appraise and func is MODULE_CHECK.

Thanks & Regards,
    - Nayna

Re: [PATCH v6 1/9] dt-bindings: ibm, secureboot: secure boot specific properties for PowerNV

[Nayna]

On 10/01/2019 09:33 AM, Rob Herring wrote:
> On Fri, Sep 27, 2019 at 10:25:52AM -0400, Nayna Jain wrote:
>> PowerNV represents both the firmware and Host OS secureboot state of the
>> system via device tree. This patch adds the documentation to give
>> the definition of the nodes and the properties.
>>
>> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
>> ---
>>   .../bindings/powerpc/ibm,secureboot.rst       | 76 ++++++++++++++++
>>   .../devicetree/bindings/powerpc/secvar.rst    | 89 +++++++++++++++++++
>>   2 files changed, 165 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/powerpc/ibm,secureboot.rst
>>   create mode 100644 Documentation/devicetree/bindings/powerpc/secvar.rst
>>
>> diff --git a/Documentation/devicetree/bindings/powerpc/ibm,secureboot.rst b/Documentation/devicetree/bindings/powerpc/ibm,secureboot.rst
>> new file mode 100644
>> index 000000000000..03d32099d2eb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/powerpc/ibm,secureboot.rst
>> @@ -0,0 +1,76 @@
>> +# SPDX-License-Identifier: GPL-2.0
> Not the right form for reST files.
>
>> +*** NOTE ***
>> +This document is copied from OPAL firmware
>> +(skiboot/doc/device-tree/ibm,secureboot.rst)
> Why copy into the kernel?

Do you mean we do not need the device-tree documentation in the kernel 
when it already exists in the skiboot tree ?

I think I am ok with that. Michael, what do you think ?

Thanks & Regards,
      - Nayna

Re: [PATCH v6 3/9] powerpc: add support to initialize ima policy rules

[Thiago Jung Bauermann]

Hi Nayna,

Nayna <nayna@linux.vnet.ibm.com> writes:

> On 09/30/2019 09:04 PM, Thiago Jung Bauermann wrote:
>>> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
>>> new file mode 100644
>>> index 000000000000..39401b67f19e
>>> --- /dev/null
>>> +++ b/arch/powerpc/kernel/ima_arch.c
>>> @@ -0,0 +1,33 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (C) 2019 IBM Corporation
>>> + * Author: Nayna Jain
>>> + */
>>> +
>>> +#include <linux/ima.h>
>>> +#include <asm/secure_boot.h>
>>> +
>>> +bool arch_ima_get_secureboot(void)
>>> +{
>>> +	return is_powerpc_os_secureboot_enabled();
>>> +}
>>> +
>>> +/* Defines IMA appraise rules for secureboot */
>>> +static const char *const arch_rules[] = {
>>> +	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
>>> +#if !IS_ENABLED(CONFIG_MODULE_SIG)
>>> +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
>>> +#endif
>>> +	NULL
>>> +};
>>> +
>>> +/*
>>> + * Returns the relevant IMA arch policies based on the system secureboot state.
>>> + */
>>> +const char *const *arch_get_ima_policy(void)
>>> +{
>>> +	if (is_powerpc_os_secureboot_enabled())
>>> +		return arch_rules;
>>> +
>>> +	return NULL;
>>> +}
>> If CONFIG_MODULE_SIG is enabled but module signatures aren't enforced,
>> then IMA won't enforce module signature either. x86's
>> arch_get_ima_policy() calls set_module_sig_enforced(). Doesn't the
>> powerpc version need to do that as well?
>>
>> On the flip side, if module signatures are enforced by the module
>> subsystem then IMA will verify the signature a second time since there's
>> no sharing of signature verification results between the module
>> subsystem and IMA (this was observed by Mimi).
>>
>> IMHO this is a minor issue, since module loading isn't a hot path and
>> the duplicate work shouldn't impact anything. But it could be avoided by
>> having a NULL entry in arch_rules, which arch_get_ima_policy() would
>> dynamically update with the "appraise func=MODULE_CHECK" rule if
>> is_module_sig_enforced() is true.
>
> Thanks Thiago for reviewing.  I am wondering that this will give two meanings
> for NULL.

What are the two meanings? My understanding is that it only means "end
of array". The additional NULL just allows arch_get_ima_policy() to
dynamically append one item to the array.

But I hadn't thought of your other alternatives. They should work just
as well. Among those, I think option 1 is cleaner.

This addresses the second issue I mentioned, but not the first.

Also, one other thing I just noticed is that x86's arch policy has
measure rules but powerpc's policy doesn't. What is different in our
case?

> Can we do something like below, there are possibly two options ?
>
> 1. Set IMA_APPRAISED in the iint->flags if is_module_sig_enforced().
>
> OR
>
> 2. Let ima_get_action() check for is_module_sig_enforced() when policy is
> appraise and func is MODULE_CHECK.
>
> Thanks & Regards,
>    - Nayna


--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v6 7/9] ima: check against blacklisted hashes for files with modsig

[Mimi Zohar]

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 <nayna@linux.ibm.com>
> ---
>  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 <linux/magic.h>
>  #include <linux/ima.h>
>  #include <linux/evm.h>
> +#include <keys/system_keyring.h>
>  
>  #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_blacklisted(), as a wrapper for
is_hash_blacklisted().


> +		if ((rc == -EKEYREJECTED) && (iint->flags & IMA_MEASURE))
> +			process_buffer_measurement(digest, digestsize,
> +						   "blacklisted-hash", pcr,
> +						   template_desc);

For appended signatures, the IMA policy measurement rule would
normally be "template=ima-modsig".  Shouldn't the "template_desc" for
blacklisted binaries be "template=ima-buf"?

> +	}
> +
> +	return rc;
> +}
> +
>  /*
>   * ima_appraise_measurement - appraise file measurement
>   *
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index ae0c1bdc4eaf..92c446045637 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -336,8 +336,12 @@ static int process_measurement(struct file *file, const struct cred *cred,
>  				      template_desc);
>  	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
>  		inode_lock(inode);

The lock is needed for updating the extended attributes (in IMA fix
mode).  Please limit taking the lock to ima_appraise_measurement().

> -		rc = ima_appraise_measurement(func, iint, file, pathname,
> -					      xattr_value, xattr_len, modsig);
> +		rc = ima_blacklist_measurement(iint, modsig, action, pcr,
> +					       template_desc);

process_measurement() calls a number of functions: ima_collect_measurement(), ima_store_measurement(), ima_appraise_measurement() and ima_audit_measurement().  The action is contained within the name (eg. collect, store, appraise, audit).   "blacklist" implies the function is blacklisting a file hash, as opposed to checking whether the file hash is already black listed.  Changing the tense from "blacklist" to "blacklisted" would help.

Renaming the function to "ima_is_binary_blacklisted" would be even better.

Mimi

> +		if (rc != -EKEYREJECTED)
> +			rc = ima_appraise_measurement(func, iint, file,
> +						      pathname, xattr_value,
> +						      xattr_len, modsig);
>  		inode_unlock(inode);
>  		if (!rc)
>  			rc = mmap_violation_check(func, file, &pathbuf,
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 4badc4fcda98..ad3b3af69460 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -765,8 +765,8 @@ enum {
>  	Opt_fsuuid, Opt_uid_eq, Opt_euid_eq, Opt_fowner_eq,
>  	Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
>  	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
> -	Opt_appraise_type, Opt_permit_directio,
> -	Opt_pcr, Opt_template, Opt_err
> +	Opt_appraise_type, Opt_appraise_flag,
> +	Opt_permit_directio, Opt_pcr, Opt_template, Opt_err
>  };
>  
>  static const match_table_t policy_tokens = {
> @@ -798,6 +798,7 @@ static const match_table_t policy_tokens = {
>  	{Opt_euid_lt, "euid<%s"},
>  	{Opt_fowner_lt, "fowner<%s"},
>  	{Opt_appraise_type, "appraise_type=%s"},
> +	{Opt_appraise_flag, "appraise_flag=%s"},
>  	{Opt_permit_directio, "permit_directio"},
>  	{Opt_pcr, "pcr=%s"},
>  	{Opt_template, "template=%s"},
> @@ -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;
>  		case Opt_permit_directio:
>  			entry->flags |= IMA_PERMIT_DIRECTIO;
>  			break;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index d9323d31a3a8..73fc286834d7 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -32,6 +32,7 @@
>  #define EVM_IMMUTABLE_DIGSIG	0x08000000
>  #define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
>  #define IMA_MODSIG_ALLOWED	0x20000000
> +#define IMA_CHECK_BLACKLIST	0x40000000
>  
>  #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
>  				 IMA_HASH | IMA_APPRAISE_SUBMASK)

Re: [PATCH v6 8/9] ima: deprecate permit_directio, instead use appraise_flag

[Mimi Zohar]

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

[Mimi Zohar]

On Tue, 2019-10-01 at 12:07 -0400, Nayna wrote:
> 
> On 09/30/2019 09:04 PM, Thiago Jung Bauermann wrote:
> > Hello,
> 
> Hi,
> 
> >
> >> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
> >> new file mode 100644
> >> index 000000000000..39401b67f19e
> >> --- /dev/null
> >> +++ b/arch/powerpc/kernel/ima_arch.c
> >> @@ -0,0 +1,33 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (C) 2019 IBM Corporation
> >> + * Author: Nayna Jain
> >> + */
> >> +
> >> +#include <linux/ima.h>
> >> +#include <asm/secure_boot.h>
> >> +
> >> +bool arch_ima_get_secureboot(void)
> >> +{
> >> +	return is_powerpc_os_secureboot_enabled();
> >> +}
> >> +
> >> +/* Defines IMA appraise rules for secureboot */
> >> +static const char *const arch_rules[] = {
> >> +	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> >> +#if !IS_ENABLED(CONFIG_MODULE_SIG)
> >> +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> >> +#endif
> >> +	NULL
> >> +};
> >> +
> >> +/*
> >> + * Returns the relevant IMA arch policies based on the system secureboot state.
> >> + */
> >> +const char *const *arch_get_ima_policy(void)
> >> +{
> >> +	if (is_powerpc_os_secureboot_enabled())
> >> +		return arch_rules;
> >> +
> >> +	return NULL;
> >> +}
> > If CONFIG_MODULE_SIG is enabled but module signatures aren't enforced,
> > then IMA won't enforce module signature either. x86's
> > arch_get_ima_policy() calls set_module_sig_enforced(). Doesn't the
> > powerpc version need to do that as well?
> >
> > On the flip side, if module signatures are enforced by the module
> > subsystem then IMA will verify the signature a second time since there's
> > no sharing of signature verification results between the module
> > subsystem and IMA (this was observed by Mimi).
> >
> > IMHO this is a minor issue, since module loading isn't a hot path and
> > the duplicate work shouldn't impact anything. But it could be avoided by
> > having a NULL entry in arch_rules, which arch_get_ima_policy() would
> > dynamically update with the "appraise func=MODULE_CHECK" rule if
> > is_module_sig_enforced() is true.
> 
> Thanks Thiago for reviewing.  I am wondering that this will give two 
> meanings for NULL. Can we do something like below, there are possibly 
> two options ?
> 
> 1. Set IMA_APPRAISED in the iint->flags if is_module_sig_enforced().
> 
> OR
> 
> 2. Let ima_get_action() check for is_module_sig_enforced() when policy 
> is appraise and func is MODULE_CHECK.

I'm a bit hesitant about mixing the module subsystem signature
verification method with the IMA measure "template=ima-modsig" rules.
 Does it actually work?

We can at least limit verifying the same appended signature twice to
when "module.sig_enforce" is specified on the boot command line, by
changing "!IS_ENABLED(CONFIG_MODULE_SIG)" to test
"CONFIG_MODULE_SIG_FORCE".

Mimi

Re: [PATCH v6 6/9] ima: make process_buffer_measurement() non static

[Mimi Zohar]

[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 <nayna@linux.ibm.com>

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 v6 3/9] powerpc: add support to initialize ima policy rules

[Nayna]

On 10/02/2019 05:49 PM, Mimi Zohar wrote:
> On Tue, 2019-10-01 at 12:07 -0400, Nayna wrote:
>> On 09/30/2019 09:04 PM, Thiago Jung Bauermann wrote:
>>> Hello,
>> Hi,
>>
>>>> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
>>>> new file mode 100644
>>>> index 000000000000..39401b67f19e
>>>> --- /dev/null
>>>> +++ b/arch/powerpc/kernel/ima_arch.c
>>>> @@ -0,0 +1,33 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (C) 2019 IBM Corporation
>>>> + * Author: Nayna Jain
>>>> + */
>>>> +
>>>> +#include <linux/ima.h>
>>>> +#include <asm/secure_boot.h>
>>>> +
>>>> +bool arch_ima_get_secureboot(void)
>>>> +{
>>>> +	return is_powerpc_os_secureboot_enabled();
>>>> +}
>>>> +
>>>> +/* Defines IMA appraise rules for secureboot */
>>>> +static const char *const arch_rules[] = {
>>>> +	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
>>>> +#if !IS_ENABLED(CONFIG_MODULE_SIG)
>>>> +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
>>>> +#endif
>>>> +	NULL
>>>> +};
>>>> +
>>>> +/*
>>>> + * Returns the relevant IMA arch policies based on the system secureboot state.
>>>> + */
>>>> +const char *const *arch_get_ima_policy(void)
>>>> +{
>>>> +	if (is_powerpc_os_secureboot_enabled())
>>>> +		return arch_rules;
>>>> +
>>>> +	return NULL;
>>>> +}
>>> If CONFIG_MODULE_SIG is enabled but module signatures aren't enforced,
>>> then IMA won't enforce module signature either. x86's
>>> arch_get_ima_policy() calls set_module_sig_enforced(). Doesn't the
>>> powerpc version need to do that as well?
>>>
>>> On the flip side, if module signatures are enforced by the module
>>> subsystem then IMA will verify the signature a second time since there's
>>> no sharing of signature verification results between the module
>>> subsystem and IMA (this was observed by Mimi).
>>>
>>> IMHO this is a minor issue, since module loading isn't a hot path and
>>> the duplicate work shouldn't impact anything. But it could be avoided by
>>> having a NULL entry in arch_rules, which arch_get_ima_policy() would
>>> dynamically update with the "appraise func=MODULE_CHECK" rule if
>>> is_module_sig_enforced() is true.
>> Thanks Thiago for reviewing.  I am wondering that this will give two
>> meanings for NULL. Can we do something like below, there are possibly
>> two options ?
>>
>> 1. Set IMA_APPRAISED in the iint->flags if is_module_sig_enforced().
>>
>> OR
>>
>> 2. Let ima_get_action() check for is_module_sig_enforced() when policy
>> is appraise and func is MODULE_CHECK.
> I'm a bit hesitant about mixing the module subsystem signature
> verification method with the IMA measure "template=ima-modsig" rules.
>   Does it actually work?
>
> We can at least limit verifying the same appended signature twice to
> when "module.sig_enforce" is specified on the boot command line, by
> changing "!IS_ENABLED(CONFIG_MODULE_SIG)" to test
> "CONFIG_MODULE_SIG_FORCE".

Yes this seems to be a better idea. I have implemented this in the v7 
version of the ima_arch version.

Thanks & Regards,
      - Nayna