IMA-appraisal is mostly being used in the embedded or single purpose
closed system environments. In these environments, both the Kconfig
options and the userspace tools can be modified appropriately to limit
syscalls. For stock kernels, userspace applications need to continue to
work with older kernels as well as with newer kernels.
In this environment, the customer needs the ability to define a system
wide IMA policy, such as requiring all kexec'ed images, firmware, kernel
modules to be signed, without being dependent on either the Kconfig
options or the userspace tools.[1]
This patch set allows the customer to define a policy which requires
the kexec'ed kernel images, firmware, and/or kernel modules to be
signed.
In addition, this patch set includes the ability to configure a build
time IMA policy, which is automatically loaded at run time without
needing to specify it on the boot command line and persists after
loading a custom kernel policy.
[1] kexec-tools suupports the new syscall based on a flag (-s).
Changelog v5:
- Shared kernel_load_data_id and kernel_read_file_id enumerations.
The previous version of this patch set defined a new LSM hook named
security_kernel_load_data and an associated enumeration named
kernel_load_data_id, independent of kernel_read_file_id. In this
version, the kernel_load_data_id and kernel_read_file_id values are
shared, simplifying Loadpin's and other LSMs calling one LSM hook from
the other.
- Warn about loading firmware from pre-shared memory.
Previous versions of this patch set prevented loading firmware, based on
policy, from pre-allocated (DMA) memory, introduced in commit
a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer").
Based on discussions, calling dma_alloc_coherent() is unnecessary and
confusing. This version of the patch set allows loading the firmware,
but emits a warning.
Changelog v4:
- Define a new LSM hook named security_kernel_load_data().
- Define kernel_load_data_id enumeration.
- Replace the existing LSM hook in init_module syscall.
Changelog v3:
Based on James' feedback:
- Renamed security_kernel_read_file() to security_kernel_read_data().
- Defined new kernel_load_data_id enumeration.
- Cleaned up ima_read_data(), replacing if's with switch.
Changelog v2:
- combined "kexec: limit kexec_load syscall" and "firmware: kernel
signature verification" patch sets.
- add support for build time policy.
- defined generic security_kernel_read_blob() wrapper for
security_kernel_read_file(). Suggested by Luis
Mimi Zohar (8):
security: define new LSM hook named security_kernel_load_data
kexec: add call to LSM hook in original kexec_load syscall
ima: based on policy require signed kexec kernel images
firmware: add call to LSM hook before firmware sysfs fallback
ima: based on policy require signed firmware (sysfs fallback)
ima: add build time policy
ima: based on policy warn about loading firmware (pre-allocated
buffer)
module: replace the existing LSM hook in init_module
drivers/base/firmware_loader/fallback.c | 7 +++
include/linux/ima.h | 7 +++
include/linux/lsm_hooks.h | 6 +++
include/linux/security.h | 27 ++++++++++++
kernel/kexec.c | 8 ++++
kernel/module.c | 2 +-
security/integrity/ima/Kconfig | 58 ++++++++++++++++++++++++
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_main.c | 78 ++++++++++++++++++++++++---------
security/integrity/ima/ima_policy.c | 48 ++++++++++++++++++--
security/loadpin/loadpin.c | 6 +++
security/security.c | 10 +++++
security/selinux/hooks.c | 15 +++++++
13 files changed, 249 insertions(+), 24 deletions(-)
--
2.7.5
IMA by default does not measure, appraise or audit files, but can be
enabled at runtime by specifying a builtin policy on the boot command line
or by loading a custom policy.
This patch defines a build time policy, which verifies kernel modules,
firmware, kexec image, and/or the IMA policy signatures. This build time
policy is automatically enabled at runtime and persists after loading a
custom policy.
Signed-off-by: Mimi Zohar <[email protected]>
---
security/integrity/ima/Kconfig | 58 +++++++++++++++++++++++++++++++++++++
security/integrity/ima/ima_policy.c | 46 +++++++++++++++++++++++++++--
2 files changed, 101 insertions(+), 3 deletions(-)
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 94c2151331aa..13b446328dda 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -157,6 +157,64 @@ config IMA_APPRAISE
<http://linux-ima.sourceforge.net>
If unsure, say N.
+config IMA_APPRAISE_BUILD_POLICY
+ bool "IMA build time configured policy rules"
+ depends on IMA_APPRAISE && INTEGRITY_ASYMMETRIC_KEYS
+ default n
+ help
+ This option defines an IMA appraisal policy at build time, which
+ is enforced at run time without having to specify a builtin
+ policy name on the boot command line. The build time appraisal
+ policy rules persist after loading a custom policy.
+
+ Depending on the rules configured, this policy may require kernel
+ modules, firmware, the kexec kernel image, and/or the IMA policy
+ to be signed. Unsigned files might prevent the system from
+ booting or applications from working properly.
+
+config IMA_APPRAISE_REQUIRE_FIRMWARE_SIGS
+ bool "Appraise firmware signatures"
+ depends on IMA_APPRAISE_BUILD_POLICY
+ default n
+ help
+ This option defines a policy requiring all firmware to be signed,
+ including the regulatory.db. If both this option and
+ CFG80211_REQUIRE_SIGNED_REGDB are enabled, then both signature
+ verification methods are necessary.
+
+config IMA_APPRAISE_REQUIRE_KEXEC_SIGS
+ bool "Appraise kexec kernel image signatures"
+ depends on IMA_APPRAISE_BUILD_POLICY
+ default n
+ help
+ Enabling this rule will require all kexec'ed kernel images to
+ be signed and verified by a public key on the trusted IMA
+ keyring.
+
+ Kernel image signatures can not be verified by the original
+ kexec_load syscall. Enabling this rule will prevent its
+ usage.
+
+config IMA_APPRAISE_REQUIRE_MODULE_SIGS
+ bool "Appraise kernel modules signatures"
+ depends on IMA_APPRAISE_BUILD_POLICY
+ default n
+ help
+ Enabling this rule will require all kernel modules to be signed
+ and verified by a public key on the trusted IMA keyring.
+
+ Kernel module signatures can only be verified by IMA-appraisal,
+ via the finit_module syscall. Enabling this rule will prevent
+ the usage of the init_module syscall.
+
+config IMA_APPRAISE_REQUIRE_POLICY_SIGS
+ bool "Appraise IMA policy signature"
+ depends on IMA_APPRAISE_BUILD_POLICY
+ default n
+ help
+ Enabling this rule will require the IMA policy to be signed and
+ and verified by a key on the trusted IMA keyring.
+
config IMA_APPRAISE_BOOTPARAM
bool "ima_appraise boot parameter"
depends on IMA_APPRAISE
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index ebfb389b79df..8c9499867c91 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -49,6 +49,7 @@
int ima_policy_flag;
static int temp_ima_appraise;
+static int build_ima_appraise __ro_after_init;
#define MAX_LSM_RULES 6
enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
@@ -162,6 +163,25 @@ static struct ima_rule_entry default_appraise_rules[] __ro_after_init = {
#endif
};
+static struct ima_rule_entry build_appraise_rules[] __ro_after_init = {
+#ifdef CONFIG_IMA_APPRAISE_REQUIRE_MODULE_SIGS
+ {.action = APPRAISE, .func = MODULE_CHECK,
+ .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+#endif
+#ifdef CONFIG_IMA_APPRAISE_REQUIRE_FIRMWARE_SIGS
+ {.action = APPRAISE, .func = FIRMWARE_CHECK,
+ .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+#endif
+#ifdef CONFIG_IMA_APPRAISE_REQUIRE_KEXEC_SIGS
+ {.action = APPRAISE, .func = KEXEC_KERNEL_CHECK,
+ .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+#endif
+#ifdef CONFIG_IMA_APPRAISE_REQUIRE_POLICY_SIGS
+ {.action = APPRAISE, .func = POLICY_CHECK,
+ .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+#endif
+};
+
static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
{.action = APPRAISE, .func = MODULE_CHECK,
.flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
@@ -435,7 +455,7 @@ void ima_update_policy_flag(void)
ima_policy_flag |= entry->action;
}
- ima_appraise |= temp_ima_appraise;
+ ima_appraise |= (build_ima_appraise | temp_ima_appraise);
if (!ima_appraise)
ima_policy_flag &= ~IMA_APPRAISE;
}
@@ -488,8 +508,8 @@ void __init ima_init_policy(void)
}
/*
- * Insert the appraise rules requiring file signatures, prior to
- * any other appraise rules.
+ * Insert the builtin "secure_boot" policy rules requiring file
+ * signatures, prior to any other appraise rules.
*/
for (i = 0; i < secure_boot_entries; i++) {
list_add_tail(&secure_boot_rules[i].list, &ima_default_rules);
@@ -497,6 +517,26 @@ void __init ima_init_policy(void)
ima_appraise_flag(secure_boot_rules[i].func);
}
+ /*
+ * Insert the build time appraise rules requiring file signatures
+ * for both the initial and custom policies, prior to other appraise
+ * rules.
+ */
+ for (i = 0; i < ARRAY_SIZE(build_appraise_rules); i++) {
+ struct ima_rule_entry *entry;
+
+ if (!secure_boot_entries)
+ list_add_tail(&build_appraise_rules[i].list,
+ &ima_default_rules);
+
+ entry = kmemdup(&build_appraise_rules[i], sizeof(*entry),
+ GFP_KERNEL);
+ if (entry)
+ list_add_tail(&entry->list, &ima_policy_rules);
+ build_ima_appraise |=
+ ima_appraise_flag(build_appraise_rules[i].func);
+ }
+
for (i = 0; i < appraise_entries; i++) {
list_add_tail(&default_appraise_rules[i].list,
&ima_default_rules);
--
2.7.5
With an IMA policy requiring signed firmware, this patch prevents
the sysfs fallback method of loading firmware.
Signed-off-by: Mimi Zohar <[email protected]>
Cc: Luis R. Rodriguez <[email protected]>
Cc: David Howells <[email protected]>
Cc: Matthew Garrett <[email protected]>
---
security/integrity/ima/ima_main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 71fecfef0939..e467664965e7 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -472,8 +472,10 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
if (!file && read_id == READING_FIRMWARE) {
if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
- (ima_appraise & IMA_APPRAISE_ENFORCE))
+ (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+ pr_err("Prevent firmware loading_store.\n");
return -EACCES; /* INTEGRITY_UNKNOWN */
+ }
return 0;
}
@@ -517,6 +519,12 @@ int ima_load_data(enum kernel_load_data_id id)
pr_err("impossible to appraise a kernel image without a file descriptor; try using kexec_file_load syscall.\n");
return -EACCES; /* INTEGRITY_UNKNOWN */
}
+ break;
+ case LOADING_FIRMWARE:
+ if (ima_appraise & IMA_APPRAISE_FIRMWARE) {
+ pr_err("Prevent firmware sysfs fallback loading.\n");
+ return -EACCES; /* INTEGRITY_UNKNOWN */
+ }
default:
break;
}
--
2.7.5
Both the init_module and finit_module syscalls call either directly
or indirectly the security_kernel_read_file LSM hook. This patch
replaces the direct call in init_module with a call to the new
security_kernel_load_data hook and makes the corresponding changes
in SELinux, LoadPin, and IMA.
Signed-off-by: Mimi Zohar <[email protected]>
Cc: Jeff Vander Stoep <[email protected]>
Cc: Casey Schaufler <[email protected]>
Cc: Kees Cook <[email protected]>
Acked-by: Jessica Yu <[email protected]>
Acked-by: Paul Moore <[email protected]>
---
Changelog v5:
- For SELinux, have both the security_kernel_read_file and
security_kernel_load_data LSM hooks call selinux_kernel_read_file().
- LoadPin: replace existing init_module LSM hook support with
new security_kernel_load_data hook.
kernel/module.c | 2 +-
security/integrity/ima/ima_main.c | 24 ++++++++++--------------
security/loadpin/loadpin.c | 6 ++++++
security/selinux/hooks.c | 15 +++++++++++++++
4 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index f475f30eed8c..a7615d661910 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2876,7 +2876,7 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
if (info->len < sizeof(*(info->hdr)))
return -ENOEXEC;
- err = security_kernel_read_file(NULL, READING_MODULE);
+ err = security_kernel_load_data(LOADING_MODULE);
if (err)
return err;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 7da123d980ea..166670e418ca 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -438,17 +438,6 @@ static int read_idmap[READING_MAX_ID] = {
*/
int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
{
- bool sig_enforce = is_module_sig_enforced();
-
- if (!file && read_id == READING_MODULE) {
- if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
- (ima_appraise & IMA_APPRAISE_ENFORCE)) {
- pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
- return -EACCES; /* INTEGRITY_UNKNOWN */
- }
- return 0; /* We rely on module signature checking */
- }
-
if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
(ima_appraise & IMA_APPRAISE_ENFORCE)) {
@@ -486,9 +475,6 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
return 0;
}
- if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */
- return 0;
-
/* permit signed certs */
if (!file && read_id == READING_X509_CERTIFICATE)
return 0;
@@ -517,6 +503,8 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
*/
int ima_load_data(enum kernel_load_data_id id)
{
+ bool sig_enforce;
+
if ((ima_appraise & IMA_APPRAISE_ENFORCE) != IMA_APPRAISE_ENFORCE)
return 0;
@@ -532,6 +520,14 @@ int ima_load_data(enum kernel_load_data_id id)
pr_err("Prevent firmware sysfs fallback loading.\n");
return -EACCES; /* INTEGRITY_UNKNOWN */
}
+ break;
+ case LOADING_MODULE:
+ sig_enforce = is_module_sig_enforced();
+
+ if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES)) {
+ pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
+ return -EACCES; /* INTEGRITY_UNKNOWN */
+ }
default:
break;
}
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index 5fa191252c8f..c5dfd0dcbb2b 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -173,9 +173,15 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
return 0;
}
+static int loadpin_load_data(enum kernel_load_data_id id)
+{
+ return loadpin_read_file(NULL, id);
+}
+
static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
+ LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
};
void __init loadpin_add_hooks(void)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2b5ee5fbd652..a8bf324130f5 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4073,6 +4073,20 @@ static int selinux_kernel_read_file(struct file *file,
return rc;
}
+static int selinux_kernel_load_data(enum kernel_load_data_id id)
+{
+ int rc = 0;
+
+ switch (id) {
+ case LOADING_MODULE:
+ rc = selinux_kernel_module_from_file(NULL);
+ default:
+ break;
+ }
+
+ return rc;
+}
+
static int selinux_task_setpgid(struct task_struct *p, pid_t pgid)
{
return avc_has_perm(&selinux_state,
@@ -6972,6 +6986,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
+ LSM_HOOK_INIT(kernel_load_data, selinux_kernel_load_data),
LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid),
LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid),
--
2.7.5
Some systems are memory constrained but they need to load very large
firmwares. The firmware subsystem allows drivers to request this
firmware be loaded from the filesystem, but this requires that the
entire firmware be loaded into kernel memory first before it's provided
to the driver. This can lead to a situation where we map the firmware
twice, once to load the firmware into kernel memory and once to copy the
firmware into the final resting place.
To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
into a pre-allocated buffer") introduced request_firmware_into_buf() API
that allows drivers to request firmware be loaded directly into a
pre-allocated buffer. (Based on the mailing list discussions, calling
dma_alloc_coherent() is unnecessary and confusing.)
(Very broken/buggy) devices using pre-allocated memory run the risk of
the firmware being accessible to the device prior to the completion of
IMA's signature verification. For the time being, this patch emits a
warning, but does not prevent the loading of the firmware.
Signed-off-by: Mimi Zohar <[email protected]>
Cc: Luis R. Rodriguez <[email protected]>
Cc: David Howells <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Serge E. Hallyn <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: Bjorn Andersson <[email protected]>
---
Changelog v5:
- Instead of preventing loading firmware from a pre-allocate buffer,
emit a warning.
security/integrity/ima/ima_main.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index e467664965e7..7da123d980ea 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -416,6 +416,15 @@ void ima_post_path_mknod(struct dentry *dentry)
iint->flags |= IMA_NEW_FILE;
}
+static int read_idmap[READING_MAX_ID] = {
+ [READING_FIRMWARE] = FIRMWARE_CHECK,
+ [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
+ [READING_MODULE] = MODULE_CHECK,
+ [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
+ [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
+ [READING_POLICY] = POLICY_CHECK
+};
+
/**
* ima_read_file - pre-measure/appraise hook decision based on policy
* @file: pointer to the file to be measured/appraised/audit
@@ -439,18 +448,16 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
}
return 0; /* We rely on module signature checking */
}
+
+ if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
+ if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
+ (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+ pr_warn("device might be able to access firmware prior to signature verification completion.\n");
+ }
+ }
return 0;
}
-static int read_idmap[READING_MAX_ID] = {
- [READING_FIRMWARE] = FIRMWARE_CHECK,
- [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
- [READING_MODULE] = MODULE_CHECK,
- [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
- [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
- [READING_POLICY] = POLICY_CHECK
-};
-
/**
* ima_post_read_file - in memory collect/appraise/audit measurement
* @file: pointer to the file to be measured/appraised/audit
--
2.7.5
The original kexec_load syscall can not verify file signatures, nor can
the kexec image be measured. Based on policy, deny the kexec_load
syscall.
Signed-off-by: Mimi Zohar <[email protected]>
Cc: Eric Biederman <[email protected]>
Cc: Luis R. Rodriguez <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: David Howells <[email protected]>
---
Changelog v3:
- use switch/case
include/linux/ima.h | 7 +++++++
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_main.c | 27 +++++++++++++++++++++++++++
security/integrity/ima/ima_policy.c | 2 ++
security/security.c | 7 ++++++-
5 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 0e4647e0eb60..84806b54b50a 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -11,6 +11,7 @@
#define _LINUX_IMA_H
#include <linux/fs.h>
+#include <linux/security.h>
#include <linux/kexec.h>
struct linux_binprm;
@@ -19,6 +20,7 @@ extern int ima_bprm_check(struct linux_binprm *bprm);
extern int ima_file_check(struct file *file, int mask, int opened);
extern void ima_file_free(struct file *file);
extern int ima_file_mmap(struct file *file, unsigned long prot);
+extern int ima_load_data(enum kernel_load_data_id id);
extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
@@ -49,6 +51,11 @@ static inline int ima_file_mmap(struct file *file, unsigned long prot)
return 0;
}
+static inline int ima_load_data(enum kernel_load_data_id id)
+{
+ return 0;
+}
+
static inline int ima_read_file(struct file *file, enum kernel_read_file_id id)
{
return 0;
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 2ab1affffa36..588e4813370c 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -232,6 +232,7 @@ int ima_policy_show(struct seq_file *m, void *v);
#define IMA_APPRAISE_MODULES 0x08
#define IMA_APPRAISE_FIRMWARE 0x10
#define IMA_APPRAISE_POLICY 0x20
+#define IMA_APPRAISE_KEXEC 0x40
#ifdef CONFIG_IMA_APPRAISE
int ima_appraise_measurement(enum ima_hooks func,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index dca44cf7838e..71fecfef0939 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -496,6 +496,33 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
MAY_READ, func, 0);
}
+/**
+ * ima_load_data - appraise decision based on policy
+ * @id: kernel load data caller identifier
+ *
+ * Callers of this LSM hook can not measure, appraise, or audit the
+ * data provided by userspace. Enforce policy rules requring a file
+ * signature (eg. kexec'ed kernel image).
+ *
+ * For permission return 0, otherwise return -EACCES.
+ */
+int ima_load_data(enum kernel_load_data_id id)
+{
+ if ((ima_appraise & IMA_APPRAISE_ENFORCE) != IMA_APPRAISE_ENFORCE)
+ return 0;
+
+ switch (id) {
+ case LOADING_KEXEC_IMAGE:
+ if (ima_appraise & IMA_APPRAISE_KEXEC) {
+ pr_err("impossible to appraise a kernel image without a file descriptor; try using kexec_file_load syscall.\n");
+ return -EACCES; /* INTEGRITY_UNKNOWN */
+ }
+ default:
+ break;
+ }
+ return 0;
+}
+
static int __init init_ima(void)
{
int error;
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 7f4a4de7e831..ebfb389b79df 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -448,6 +448,8 @@ static int ima_appraise_flag(enum ima_hooks func)
return IMA_APPRAISE_FIRMWARE;
else if (func == POLICY_CHECK)
return IMA_APPRAISE_POLICY;
+ else if (func == KEXEC_KERNEL_CHECK)
+ return IMA_APPRAISE_KEXEC;
return 0;
}
diff --git a/security/security.c b/security/security.c
index 05fe5b1932d7..7b870df0a335 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1063,7 +1063,12 @@ EXPORT_SYMBOL_GPL(security_kernel_post_read_file);
int security_kernel_load_data(enum kernel_load_data_id id)
{
- return call_int_hook(kernel_load_data, 0, id);
+ int ret;
+
+ ret = call_int_hook(kernel_load_data, 0, id);
+ if (ret)
+ return ret;
+ return ima_load_data(id);
}
int security_task_fix_setuid(struct cred *new, const struct cred *old,
--
2.7.5
Add an LSM hook prior to allowing firmware sysfs fallback loading.
Signed-off-by: Mimi Zohar <[email protected]>
Cc: Luis R. Rodriguez <[email protected]>
Cc: David Howells <[email protected]>
Cc: Kees Cook <[email protected]>
Acked-by: Luis R. Rodriguez <[email protected]>
---
Changelog v4:
- call new LSM security_kernel_load_data hook
Changelog v2:
- call security_kernel_read_blob()
- rename the READING_FIRMWARE_FALLBACK kernel_read_file_id enumeration to
READING_FIRMWARE_FALLBACK_SYSFS.
drivers/base/firmware_loader/fallback.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 7f732744f0d3..202324291542 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -651,6 +651,8 @@ static bool fw_force_sysfs_fallback(enum fw_opt opt_flags)
static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
{
+ int ret;
+
if (fw_fallback_config.ignore_sysfs_fallback) {
pr_info_once("Ignoring firmware sysfs fallback due to sysctl knob\n");
return false;
@@ -659,6 +661,11 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
if ((opt_flags & FW_OPT_NOFALLBACK))
return false;
+ /* Also permit LSMs and IMA to fail firmware sysfs fallback */
+ ret = security_kernel_load_data(LOADING_FIRMWARE);
+ if (ret < 0)
+ return ret;
+
return fw_force_sysfs_fallback(opt_flags);
}
--
2.7.5
In order for LSMs and IMA-appraisal to differentiate between kexec_load
and kexec_file_load syscalls, both the original and new syscalls must
call an LSM hook. This patch adds a call to security_kernel_load_data()
in the original kexec_load syscall.
Signed-off-by: Mimi Zohar <[email protected]>
Cc: Eric Biederman <[email protected]>
Cc: Luis R. Rodriguez <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: David Howells <[email protected]>
Acked-by: Serge Hallyn <[email protected]>
---
kernel/kexec.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/kernel/kexec.c b/kernel/kexec.c
index aed8fb2564b3..68559808fdfa 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -11,6 +11,7 @@
#include <linux/capability.h>
#include <linux/mm.h>
#include <linux/file.h>
+#include <linux/security.h>
#include <linux/kexec.h>
#include <linux/mutex.h>
#include <linux/list.h>
@@ -195,10 +196,17 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments,
static inline int kexec_load_check(unsigned long nr_segments,
unsigned long flags)
{
+ int result;
+
/* We only trust the superuser with rebooting the system. */
if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
return -EPERM;
+ /* Permit LSMs and IMA to fail the kexec */
+ result = security_kernel_load_data(LOADING_KEXEC_IMAGE);
+ if (result < 0)
+ return result;
+
/*
* Verify we have a legal set of flags
* This leaves us room for future extensions.
--
2.7.5
Differentiate between the kernel reading a file from the kernel loading
data provided by userspace. This patch defines a new LSM hook named
security_kernel_load_data.
Signed-off-by: Mimi Zohar <[email protected]>
Cc: Eric Biederman <[email protected]>
Cc: Luis R. Rodriguez <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: David Howells <[email protected]>
Cc: Casey Schaufler <[email protected]>
Acked-by: Serge Hallyn <[email protected]>
---
Changelog v5:
- Share the kernel_load_data_id and kernel_read_file_id values,
simplifying Loadpin's and other LSMs calling one LSM hook from the
other.
Changelog v4:
- Define new LSM hook named security_kernel_load_data.
Changelog v3:
- Rename security_kernel_read_file to security_kernel_read_data().
Changelog v2:
- Define a generic wrapper named security_kernel_read_blob() for
security_kernel_read_file().
Changelog v1:
- Define and call security_kexec_load(), a wrapper for
security_kernel_read_file().
include/linux/lsm_hooks.h | 6 ++++++
include/linux/security.h | 27 +++++++++++++++++++++++++++
security/security.c | 5 +++++
3 files changed, 38 insertions(+)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 8f1131c8dd54..a08bc2587b96 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -576,6 +576,10 @@
* userspace to load a kernel module with the given name.
* @kmod_name name of the module requested by the kernel
* Return 0 if successful.
+ * @kernel_load_data:
+ * Load data provided by userspace.
+ * @id kernel load data identifier
+ * Return 0 if permission is granted.
* @kernel_read_file:
* Read a file specified by userspace.
* @file contains the file structure pointing to the file being read
@@ -1582,6 +1586,7 @@ union security_list_options {
int (*kernel_act_as)(struct cred *new, u32 secid);
int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
int (*kernel_module_request)(char *kmod_name);
+ int (*kernel_load_data)(enum kernel_load_data_id id);
int (*kernel_read_file)(struct file *file, enum kernel_read_file_id id);
int (*kernel_post_read_file)(struct file *file, char *buf, loff_t size,
enum kernel_read_file_id id);
@@ -1872,6 +1877,7 @@ struct security_hook_heads {
struct hlist_head cred_getsecid;
struct hlist_head kernel_act_as;
struct hlist_head kernel_create_files_as;
+ struct hlist_head kernel_load_data;
struct hlist_head kernel_read_file;
struct hlist_head kernel_post_read_file;
struct hlist_head kernel_module_request;
diff --git a/include/linux/security.h b/include/linux/security.h
index 63030c85ee19..3410acfe139c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -159,6 +159,27 @@ extern int mmap_min_addr_handler(struct ctl_table *table, int write,
typedef int (*initxattrs) (struct inode *inode,
const struct xattr *xattr_array, void *fs_data);
+
+/* Keep the kernel_load_data_id enum in sync with kernel_read_file_id */
+#define __data_id_enumify(ENUM, dummy) LOADING_ ## ENUM,
+#define __data_id_stringify(dummy, str) #str,
+
+enum kernel_load_data_id {
+ __kernel_read_file_id(__data_id_enumify)
+};
+
+static const char * const kernel_load_data_str[] = {
+ __kernel_read_file_id(__data_id_stringify)
+};
+
+static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
+{
+ if ((unsigned)id >= LOADING_MAX_ID)
+ return kernel_load_data_str[LOADING_UNKNOWN];
+
+ return kernel_load_data_str[id];
+}
+
#ifdef CONFIG_SECURITY
struct security_mnt_opts {
@@ -320,6 +341,7 @@ void security_cred_getsecid(const struct cred *c, u32 *secid);
int security_kernel_act_as(struct cred *new, u32 secid);
int security_kernel_create_files_as(struct cred *new, struct inode *inode);
int security_kernel_module_request(char *kmod_name);
+int security_kernel_load_data(enum kernel_load_data_id id);
int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
enum kernel_read_file_id id);
@@ -909,6 +931,11 @@ static inline int security_kernel_module_request(char *kmod_name)
return 0;
}
+static inline int security_kernel_load_data(enum kernel_load_data_id id)
+{
+ return 0;
+}
+
static inline int security_kernel_read_file(struct file *file,
enum kernel_read_file_id id)
{
diff --git a/security/security.c b/security/security.c
index e7d76a8000a5..05fe5b1932d7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1061,6 +1061,11 @@ int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
}
EXPORT_SYMBOL_GPL(security_kernel_post_read_file);
+int security_kernel_load_data(enum kernel_load_data_id id)
+{
+ return call_int_hook(kernel_load_data, 0, id);
+}
+
int security_task_fix_setuid(struct cred *new, const struct cred *old,
int flags)
{
--
2.7.5
On 2 July 2018 at 16:38, Mimi Zohar <[email protected]> wrote:
> Some systems are memory constrained but they need to load very large
> firmwares. The firmware subsystem allows drivers to request this
> firmware be loaded from the filesystem, but this requires that the
> entire firmware be loaded into kernel memory first before it's provided
> to the driver. This can lead to a situation where we map the firmware
> twice, once to load the firmware into kernel memory and once to copy the
> firmware into the final resting place.
>
> To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
> into a pre-allocated buffer") introduced request_firmware_into_buf() API
> that allows drivers to request firmware be loaded directly into a
> pre-allocated buffer. (Based on the mailing list discussions, calling
> dma_alloc_coherent() is unnecessary and confusing.)
>
> (Very broken/buggy) devices using pre-allocated memory run the risk of
> the firmware being accessible to the device prior to the completion of
> IMA's signature verification. For the time being, this patch emits a
> warning, but does not prevent the loading of the firmware.
>
As I attempted to explain in the exchange with Luis, this has nothing
to do with broken or buggy devices, but is simply the reality we have
to deal with on platforms that lack IOMMUs.
Even if you load into one buffer, carry out the signature verification
and *only then* copy it to another buffer, a bus master could
potentially read it from the first buffer as well. Mapping for DMA
does *not* mean 'making the memory readable by the device' unless
IOMMUs are being used. Otherwise, a bus master can read it from the
first buffer, or even patch the code that performs the security check
in the first place. For such platforms, copying the data around to
prevent the device from reading it is simply pointless, as well as any
other mitigation in software to protect yourself from misbehaving bus
masters.
So issuing a warning in this particular case is rather arbitrary. On
these platforms, all bus masters can read (and modify) all of your
memory all of the time, and as long as the firmware loader code takes
care not to provide the DMA address to the device until after the
verification is complete, it really has done all it reasonably can in
the environment that it is expected to operate in.
(The use of dma_alloc_coherent() is a bit of a red herring here, as it
incorporates the DMA map operation. However, DMA map is a no-op on
systems with cache coherent 1:1 DMA [iow, all PCs and most arm64
platforms unless they have IOMMUs], and so there is not much
difference between memory allocated with kmalloc() or with
dma_alloc_coherent() in terms of whether the device can access it
freely)
> Signed-off-by: Mimi Zohar <[email protected]>
> Cc: Luis R. Rodriguez <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Serge E. Hallyn <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
>
> ---
> Changelog v5:
> - Instead of preventing loading firmware from a pre-allocate buffer,
> emit a warning.
>
> security/integrity/ima/ima_main.c | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index e467664965e7..7da123d980ea 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -416,6 +416,15 @@ void ima_post_path_mknod(struct dentry *dentry)
> iint->flags |= IMA_NEW_FILE;
> }
>
> +static int read_idmap[READING_MAX_ID] = {
> + [READING_FIRMWARE] = FIRMWARE_CHECK,
> + [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
> + [READING_MODULE] = MODULE_CHECK,
> + [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
> + [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
> + [READING_POLICY] = POLICY_CHECK
> +};
> +
> /**
> * ima_read_file - pre-measure/appraise hook decision based on policy
> * @file: pointer to the file to be measured/appraised/audit
> @@ -439,18 +448,16 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
> }
> return 0; /* We rely on module signature checking */
> }
> +
> + if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
> + if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> + (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> + pr_warn("device might be able to access firmware prior to signature verification completion.\n");
> + }
> + }
> return 0;
> }
>
> -static int read_idmap[READING_MAX_ID] = {
> - [READING_FIRMWARE] = FIRMWARE_CHECK,
> - [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
> - [READING_MODULE] = MODULE_CHECK,
> - [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
> - [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
> - [READING_POLICY] = POLICY_CHECK
> -};
> -
> /**
> * ima_post_read_file - in memory collect/appraise/audit measurement
> * @file: pointer to the file to be measured/appraised/audit
> --
> 2.7.5
>
On 7/2/18 7:37 AM, Mimi Zohar wrote:
> The original kexec_load syscall can not verify file signatures, nor can
> the kexec image be measured. Based on policy, deny the kexec_load
> syscall.
Curiosity question: I thought kexec_load() syscall was used to load a
crashdump? If this is true, how would this work if kexec_load() is
being denied? I don't think I'd want to be hindered in cases where I'm
trying to diagnose a crash.
Thanks,
Jay
> Signed-off-by: Mimi Zohar <[email protected]>
> Cc: Eric Biederman <[email protected]>
> Cc: Luis R. Rodriguez <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: David Howells <[email protected]>
>
> ---
> Changelog v3:
> - use switch/case
>
> include/linux/ima.h | 7 +++++++
> security/integrity/ima/ima.h | 1 +
> security/integrity/ima/ima_main.c | 27 +++++++++++++++++++++++++++
> security/integrity/ima/ima_policy.c | 2 ++
> security/security.c | 7 ++++++-
> 5 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 0e4647e0eb60..84806b54b50a 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -11,6 +11,7 @@
> #define _LINUX_IMA_H
>
> #include <linux/fs.h>
> +#include <linux/security.h>
> #include <linux/kexec.h>
> struct linux_binprm;
>
> @@ -19,6 +20,7 @@ extern int ima_bprm_check(struct linux_binprm *bprm);
> extern int ima_file_check(struct file *file, int mask, int opened);
> extern void ima_file_free(struct file *file);
> extern int ima_file_mmap(struct file *file, unsigned long prot);
> +extern int ima_load_data(enum kernel_load_data_id id);
> extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
> extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> enum kernel_read_file_id id);
> @@ -49,6 +51,11 @@ static inline int ima_file_mmap(struct file *file, unsigned long prot)
> return 0;
> }
>
> +static inline int ima_load_data(enum kernel_load_data_id id)
> +{
> + return 0;
> +}
> +
> static inline int ima_read_file(struct file *file, enum kernel_read_file_id id)
> {
> return 0;
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 2ab1affffa36..588e4813370c 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -232,6 +232,7 @@ int ima_policy_show(struct seq_file *m, void *v);
> #define IMA_APPRAISE_MODULES 0x08
> #define IMA_APPRAISE_FIRMWARE 0x10
> #define IMA_APPRAISE_POLICY 0x20
> +#define IMA_APPRAISE_KEXEC 0x40
>
> #ifdef CONFIG_IMA_APPRAISE
> int ima_appraise_measurement(enum ima_hooks func,
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index dca44cf7838e..71fecfef0939 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -496,6 +496,33 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
> MAY_READ, func, 0);
> }
>
> +/**
> + * ima_load_data - appraise decision based on policy
> + * @id: kernel load data caller identifier
> + *
> + * Callers of this LSM hook can not measure, appraise, or audit the
> + * data provided by userspace. Enforce policy rules requring a file
> + * signature (eg. kexec'ed kernel image).
> + *
> + * For permission return 0, otherwise return -EACCES.
> + */
> +int ima_load_data(enum kernel_load_data_id id)
> +{
> + if ((ima_appraise & IMA_APPRAISE_ENFORCE) != IMA_APPRAISE_ENFORCE)
> + return 0;
> +
> + switch (id) {
> + case LOADING_KEXEC_IMAGE:
> + if (ima_appraise & IMA_APPRAISE_KEXEC) {
> + pr_err("impossible to appraise a kernel image without a file descriptor; try using kexec_file_load syscall.\n");
> + return -EACCES; /* INTEGRITY_UNKNOWN */
> + }
> + default:
> + break;
> + }
> + return 0;
> +}
> +
> static int __init init_ima(void)
> {
> int error;
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 7f4a4de7e831..ebfb389b79df 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -448,6 +448,8 @@ static int ima_appraise_flag(enum ima_hooks func)
> return IMA_APPRAISE_FIRMWARE;
> else if (func == POLICY_CHECK)
> return IMA_APPRAISE_POLICY;
> + else if (func == KEXEC_KERNEL_CHECK)
> + return IMA_APPRAISE_KEXEC;
> return 0;
> }
>
> diff --git a/security/security.c b/security/security.c
> index 05fe5b1932d7..7b870df0a335 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1063,7 +1063,12 @@ EXPORT_SYMBOL_GPL(security_kernel_post_read_file);
>
> int security_kernel_load_data(enum kernel_load_data_id id)
> {
> - return call_int_hook(kernel_load_data, 0, id);
> + int ret;
> +
> + ret = call_int_hook(kernel_load_data, 0, id);
> + if (ret)
> + return ret;
> + return ima_load_data(id);
> }
>
> int security_task_fix_setuid(struct cred *new, const struct cred *old,
On 7/2/18 7:37 AM, Mimi Zohar wrote:
> Differentiate between the kernel reading a file from the kernel loading
> data provided by userspace. This patch defines a new LSM hook named
> security_kernel_load_data.
If this patch series is re-done, can we tweak the description here
please? From what I understood of the code in this patch, I'd tweak it as:
"Differentiate between the kernel reading a file specified by userspace
and the kernel loading a block of data provided by userspace. This
patch defines a new LSM hook named security_kernel_load_data()."
From the description, I got a tad confused if the the kernel reading a file was also provided by userspace (I know it may be 2nd-nature to people on this list, I'm still learning this kernel module).
Thanks,
Jay
> Signed-off-by: Mimi Zohar <[email protected]>
> Cc: Eric Biederman <[email protected]>
> Cc: Luis R. Rodriguez <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: Casey Schaufler <[email protected]>
> Acked-by: Serge Hallyn <[email protected]>
>
> ---
> Changelog v5:
> - Share the kernel_load_data_id and kernel_read_file_id values,
> simplifying Loadpin's and other LSMs calling one LSM hook from the
> other.
>
> Changelog v4:
> - Define new LSM hook named security_kernel_load_data.
>
> Changelog v3:
> - Rename security_kernel_read_file to security_kernel_read_data().
>
> Changelog v2:
> - Define a generic wrapper named security_kernel_read_blob() for
> security_kernel_read_file().
>
> Changelog v1:
> - Define and call security_kexec_load(), a wrapper for
> security_kernel_read_file().
>
> include/linux/lsm_hooks.h | 6 ++++++
> include/linux/security.h | 27 +++++++++++++++++++++++++++
> security/security.c | 5 +++++
> 3 files changed, 38 insertions(+)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 8f1131c8dd54..a08bc2587b96 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -576,6 +576,10 @@
> * userspace to load a kernel module with the given name.
> * @kmod_name name of the module requested by the kernel
> * Return 0 if successful.
> + * @kernel_load_data:
> + * Load data provided by userspace.
> + * @id kernel load data identifier
> + * Return 0 if permission is granted.
> * @kernel_read_file:
> * Read a file specified by userspace.
> * @file contains the file structure pointing to the file being read
> @@ -1582,6 +1586,7 @@ union security_list_options {
> int (*kernel_act_as)(struct cred *new, u32 secid);
> int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
> int (*kernel_module_request)(char *kmod_name);
> + int (*kernel_load_data)(enum kernel_load_data_id id);
> int (*kernel_read_file)(struct file *file, enum kernel_read_file_id id);
> int (*kernel_post_read_file)(struct file *file, char *buf, loff_t size,
> enum kernel_read_file_id id);
> @@ -1872,6 +1877,7 @@ struct security_hook_heads {
> struct hlist_head cred_getsecid;
> struct hlist_head kernel_act_as;
> struct hlist_head kernel_create_files_as;
> + struct hlist_head kernel_load_data;
> struct hlist_head kernel_read_file;
> struct hlist_head kernel_post_read_file;
> struct hlist_head kernel_module_request;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 63030c85ee19..3410acfe139c 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -159,6 +159,27 @@ extern int mmap_min_addr_handler(struct ctl_table *table, int write,
> typedef int (*initxattrs) (struct inode *inode,
> const struct xattr *xattr_array, void *fs_data);
>
> +
> +/* Keep the kernel_load_data_id enum in sync with kernel_read_file_id */
> +#define __data_id_enumify(ENUM, dummy) LOADING_ ## ENUM,
> +#define __data_id_stringify(dummy, str) #str,
> +
> +enum kernel_load_data_id {
> + __kernel_read_file_id(__data_id_enumify)
> +};
> +
> +static const char * const kernel_load_data_str[] = {
> + __kernel_read_file_id(__data_id_stringify)
> +};
> +
> +static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
> +{
> + if ((unsigned)id >= LOADING_MAX_ID)
> + return kernel_load_data_str[LOADING_UNKNOWN];
> +
> + return kernel_load_data_str[id];
> +}
> +
> #ifdef CONFIG_SECURITY
>
> struct security_mnt_opts {
> @@ -320,6 +341,7 @@ void security_cred_getsecid(const struct cred *c, u32 *secid);
> int security_kernel_act_as(struct cred *new, u32 secid);
> int security_kernel_create_files_as(struct cred *new, struct inode *inode);
> int security_kernel_module_request(char *kmod_name);
> +int security_kernel_load_data(enum kernel_load_data_id id);
> int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
> int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
> enum kernel_read_file_id id);
> @@ -909,6 +931,11 @@ static inline int security_kernel_module_request(char *kmod_name)
> return 0;
> }
>
> +static inline int security_kernel_load_data(enum kernel_load_data_id id)
> +{
> + return 0;
> +}
> +
> static inline int security_kernel_read_file(struct file *file,
> enum kernel_read_file_id id)
> {
> diff --git a/security/security.c b/security/security.c
> index e7d76a8000a5..05fe5b1932d7 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1061,6 +1061,11 @@ int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
> }
> EXPORT_SYMBOL_GPL(security_kernel_post_read_file);
>
> +int security_kernel_load_data(enum kernel_load_data_id id)
> +{
> + return call_int_hook(kernel_load_data, 0, id);
> +}
> +
> int security_task_fix_setuid(struct cred *new, const struct cred *old,
> int flags)
> {
Hi Mimi,
I love your patch! Perhaps something to improve:
[auto build test WARNING on integrity/next-integrity]
[also build test WARNING on v4.18-rc3 next-20180702]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Mimi-Zohar/kexec-firmware-support-system-wide-policy-requiring-signatures/20180703-011114
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
>> security/loadpin/loadpin.c:178:40: sparse: mixing different enum types
security/loadpin/loadpin.c:178:40: int enum kernel_load_data_id versus
security/loadpin/loadpin.c:178:40: int enum kernel_read_file_id
vim +178 security/loadpin/loadpin.c
175
176 static int loadpin_load_data(enum kernel_load_data_id id)
177 {
> 178 return loadpin_read_file(NULL, id);
179 }
180
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Mimi,
I love your patch! Yet something to improve:
[auto build test ERROR on integrity/next-integrity]
[also build test ERROR on v4.18-rc3 next-20180702]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Mimi-Zohar/kexec-firmware-support-system-wide-policy-requiring-signatures/20180703-011114
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
config: x86_64-randconfig-ws0-07031240 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
>> ERROR: "security_kernel_load_data" [drivers/base/firmware_loader/firmware_class.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Mon, 2018-07-02 at 11:45 -0700, J Freyensee wrote:
>
> On 7/2/18 7:37 AM, Mimi Zohar wrote:
> > Differentiate between the kernel reading a file from the kernel loading
> > data provided by userspace. This patch defines a new LSM hook named
> > security_kernel_load_data.
>
> If this patch series is re-done, can we tweak the description here
> please? From what I understood of the code in this patch, I'd tweak it as:
>
> "Differentiate between the kernel reading a file specified by userspace
> and the kernel loading a block of data provided by userspace. This
> patch defines a new LSM hook named security_kernel_load_data()."
>
> From the description, I got a tad confused if the the kernel
> reading a file was also provided by userspace (I know it may be 2nd-
> nature to people on this list, I'm still learning this kernel
> module).
Could we replace "a block of data provided by userspace" with just
"data provided by userspace"?
Mimi
On Mon, 2018-07-02 at 11:31 -0700, J Freyensee wrote:
>
> On 7/2/18 7:37 AM, Mimi Zohar wrote:
> > The original kexec_load syscall can not verify file signatures, nor can
> > the kexec image be measured. Based on policy, deny the kexec_load
> > syscall.
>
>
> Curiosity question: I thought kexec_load() syscall was used to load a
> crashdump?
kexec is used to collect the memory used to analyze the crash dump.
> If this is true, how would this work if kexec_load() is
> being denied? I don't think I'd want to be hindered in cases where I'm
> trying to diagnose a crash.
For trusted & secure boot, we need a full measurement list and
signature chain of trust rooted in HW. Permitting kexec_load would
break these chains of trust.
Permitting/denying kexec_load is based on a runtime IMA policy. Patch
6/8 "ima: add build time policy", in this patch set, introduces the
concept of a build time policy. With these patches, you could
configure your kernel and/or load an IMA policy permitting kexec_load.
Mimi
On Mon, 2018-07-02 at 17:30 +0200, Ard Biesheuvel wrote:
> On 2 July 2018 at 16:38, Mimi Zohar <[email protected]> wrote:
> > Some systems are memory constrained but they need to load very large
> > firmwares. The firmware subsystem allows drivers to request this
> > firmware be loaded from the filesystem, but this requires that the
> > entire firmware be loaded into kernel memory first before it's provided
> > to the driver. This can lead to a situation where we map the firmware
> > twice, once to load the firmware into kernel memory and once to copy the
> > firmware into the final resting place.
> >
> > To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
> > into a pre-allocated buffer") introduced request_firmware_into_buf() API
> > that allows drivers to request firmware be loaded directly into a
> > pre-allocated buffer. (Based on the mailing list discussions, calling
> > dma_alloc_coherent() is unnecessary and confusing.)
> >
> > (Very broken/buggy) devices using pre-allocated memory run the risk of
> > the firmware being accessible to the device prior to the completion of
> > IMA's signature verification. For the time being, this patch emits a
> > warning, but does not prevent the loading of the firmware.
> >
>
> As I attempted to explain in the exchange with Luis, this has nothing
> to do with broken or buggy devices, but is simply the reality we have
> to deal with on platforms that lack IOMMUs.
> Even if you load into one buffer, carry out the signature verification
> and *only then* copy it to another buffer, a bus master could
> potentially read it from the first buffer as well. Mapping for DMA
> does *not* mean 'making the memory readable by the device' unless
> IOMMUs are being used. Otherwise, a bus master can read it from the
> first buffer, or even patch the code that performs the security check
> in the first place. For such platforms, copying the data around to
> prevent the device from reading it is simply pointless, as well as any
> other mitigation in software to protect yourself from misbehaving bus
> masters.
Thank you for taking the time to explain this again.
> So issuing a warning in this particular case is rather arbitrary. On
> these platforms, all bus masters can read (and modify) all of your
> memory all of the time, and as long as the firmware loader code takes
> care not to provide the DMA address to the device until after the
> verification is complete, it really has done all it reasonably can in
> the environment that it is expected to operate in.
So for the non-IOMMU system case, differentiating between pre-
allocated buffers vs. using two buffers doesn't make sense.
>
> (The use of dma_alloc_coherent() is a bit of a red herring here, as it
> incorporates the DMA map operation. However, DMA map is a no-op on
> systems with cache coherent 1:1 DMA [iow, all PCs and most arm64
> platforms unless they have IOMMUs], and so there is not much
> difference between memory allocated with kmalloc() or with
> dma_alloc_coherent() in terms of whether the device can access it
> freely)
What about systems with an IOMMU?
Mimi
On 9 July 2018 at 21:41, Mimi Zohar <[email protected]> wrote:
> On Mon, 2018-07-02 at 17:30 +0200, Ard Biesheuvel wrote:
>> On 2 July 2018 at 16:38, Mimi Zohar <[email protected]> wrote:
>> > Some systems are memory constrained but they need to load very large
>> > firmwares. The firmware subsystem allows drivers to request this
>> > firmware be loaded from the filesystem, but this requires that the
>> > entire firmware be loaded into kernel memory first before it's provided
>> > to the driver. This can lead to a situation where we map the firmware
>> > twice, once to load the firmware into kernel memory and once to copy the
>> > firmware into the final resting place.
>> >
>> > To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
>> > into a pre-allocated buffer") introduced request_firmware_into_buf() API
>> > that allows drivers to request firmware be loaded directly into a
>> > pre-allocated buffer. (Based on the mailing list discussions, calling
>> > dma_alloc_coherent() is unnecessary and confusing.)
>> >
>> > (Very broken/buggy) devices using pre-allocated memory run the risk of
>> > the firmware being accessible to the device prior to the completion of
>> > IMA's signature verification. For the time being, this patch emits a
>> > warning, but does not prevent the loading of the firmware.
>> >
>>
>> As I attempted to explain in the exchange with Luis, this has nothing
>> to do with broken or buggy devices, but is simply the reality we have
>> to deal with on platforms that lack IOMMUs.
>
>> Even if you load into one buffer, carry out the signature verification
>> and *only then* copy it to another buffer, a bus master could
>> potentially read it from the first buffer as well. Mapping for DMA
>> does *not* mean 'making the memory readable by the device' unless
>> IOMMUs are being used. Otherwise, a bus master can read it from the
>> first buffer, or even patch the code that performs the security check
>> in the first place. For such platforms, copying the data around to
>> prevent the device from reading it is simply pointless, as well as any
>> other mitigation in software to protect yourself from misbehaving bus
>> masters.
>
> Thank you for taking the time to explain this again.
>
>> So issuing a warning in this particular case is rather arbitrary. On
>> these platforms, all bus masters can read (and modify) all of your
>> memory all of the time, and as long as the firmware loader code takes
>> care not to provide the DMA address to the device until after the
>> verification is complete, it really has done all it reasonably can in
>> the environment that it is expected to operate in.
>
> So for the non-IOMMU system case, differentiating between pre-
> allocated buffers vs. using two buffers doesn't make sense.
>
>>
>> (The use of dma_alloc_coherent() is a bit of a red herring here, as it
>> incorporates the DMA map operation. However, DMA map is a no-op on
>> systems with cache coherent 1:1 DMA [iow, all PCs and most arm64
>> platforms unless they have IOMMUs], and so there is not much
>> difference between memory allocated with kmalloc() or with
>> dma_alloc_coherent() in terms of whether the device can access it
>> freely)
>
> What about systems with an IOMMU?
>
On systems with an IOMMU, performing the DMA map will create an entry
in the IOMMU page tables for the physical region associated with the
buffer, making the region accessible to the device. For platforms in
this category, using dma_alloc_coherent() for allocating a buffer to
pass firmware to the device does open a window where the device could
theoretically access this data while the validation is still in
progress.
Note that the device still needs to be informed about the address of
the buffer: just calling dma_alloc_coherent() will not allow the
device to find the firmware image in its memory space, and arbitrary
DMA accesses performed by the device will trigger faults that are
reported to the OS. So the window between DMA map (or
dma_alloc_coherent()) and the device specific command to pass the DMA
buffer address to the device is not inherently unsafe IMO, but I do
understand the need to cover this scenario.
As I pointed out before, using coherent DMA buffers to perform
streaming DMA is generally a bad idea, since they may be allocated
from a finite pool, and may use uncached mappings, making the access
slower than necessary (while streaming DMA can use any kmalloc'ed
buffer and will just flush the contents of the caches to main memory
when the DMA map is performed).
So to summarize again: in my opinion, using a single buffer is not a
problem as long as the validation completes before the DMA map is
performed. This will provide the expected guarantees on systems with
IOMMUs, and will not complicate matters on systems where there is no
point in obsessing about this anyway given that devices can access all
of memory whenever they want to.
As for the Qualcomm case: dma_alloc_coherent() is not needed here but
simply ends up being used because it was already wired up in the
qualcomm specific secure world API, which amounts to doing syscalls
into a higher privilege level than the one the kernel itself runs at.
So again, reasoning about whether the secure world will look at your
data before you checked the sig is rather pointless, and adding
special cases to the IMA api to cater for this use case seems like a
waste of engineering and review effort to me. If we have to do
something to tie up this loose end, let's try switching it to the
streaming DMA api instead.
--
Ard.
On 10 July 2018 at 08:51, Ard Biesheuvel <[email protected]> wrote:
> On 9 July 2018 at 21:41, Mimi Zohar <[email protected]> wrote:
>> On Mon, 2018-07-02 at 17:30 +0200, Ard Biesheuvel wrote:
>>> On 2 July 2018 at 16:38, Mimi Zohar <[email protected]> wrote:
>>> > Some systems are memory constrained but they need to load very large
>>> > firmwares. The firmware subsystem allows drivers to request this
>>> > firmware be loaded from the filesystem, but this requires that the
>>> > entire firmware be loaded into kernel memory first before it's provided
>>> > to the driver. This can lead to a situation where we map the firmware
>>> > twice, once to load the firmware into kernel memory and once to copy the
>>> > firmware into the final resting place.
>>> >
>>> > To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
>>> > into a pre-allocated buffer") introduced request_firmware_into_buf() API
>>> > that allows drivers to request firmware be loaded directly into a
>>> > pre-allocated buffer. (Based on the mailing list discussions, calling
>>> > dma_alloc_coherent() is unnecessary and confusing.)
>>> >
>>> > (Very broken/buggy) devices using pre-allocated memory run the risk of
>>> > the firmware being accessible to the device prior to the completion of
>>> > IMA's signature verification. For the time being, this patch emits a
>>> > warning, but does not prevent the loading of the firmware.
>>> >
>>>
>>> As I attempted to explain in the exchange with Luis, this has nothing
>>> to do with broken or buggy devices, but is simply the reality we have
>>> to deal with on platforms that lack IOMMUs.
>>
>>> Even if you load into one buffer, carry out the signature verification
>>> and *only then* copy it to another buffer, a bus master could
>>> potentially read it from the first buffer as well. Mapping for DMA
>>> does *not* mean 'making the memory readable by the device' unless
>>> IOMMUs are being used. Otherwise, a bus master can read it from the
>>> first buffer, or even patch the code that performs the security check
>>> in the first place. For such platforms, copying the data around to
>>> prevent the device from reading it is simply pointless, as well as any
>>> other mitigation in software to protect yourself from misbehaving bus
>>> masters.
>>
>> Thank you for taking the time to explain this again.
>>
>>> So issuing a warning in this particular case is rather arbitrary. On
>>> these platforms, all bus masters can read (and modify) all of your
>>> memory all of the time, and as long as the firmware loader code takes
>>> care not to provide the DMA address to the device until after the
>>> verification is complete, it really has done all it reasonably can in
>>> the environment that it is expected to operate in.
>>
>> So for the non-IOMMU system case, differentiating between pre-
>> allocated buffers vs. using two buffers doesn't make sense.
>>
>>>
>>> (The use of dma_alloc_coherent() is a bit of a red herring here, as it
>>> incorporates the DMA map operation. However, DMA map is a no-op on
>>> systems with cache coherent 1:1 DMA [iow, all PCs and most arm64
>>> platforms unless they have IOMMUs], and so there is not much
>>> difference between memory allocated with kmalloc() or with
>>> dma_alloc_coherent() in terms of whether the device can access it
>>> freely)
>>
>> What about systems with an IOMMU?
>>
>
> On systems with an IOMMU, performing the DMA map will create an entry
> in the IOMMU page tables for the physical region associated with the
> buffer, making the region accessible to the device. For platforms in
> this category, using dma_alloc_coherent() for allocating a buffer to
> pass firmware to the device does open a window where the device could
> theoretically access this data while the validation is still in
> progress.
>
> Note that the device still needs to be informed about the address of
> the buffer: just calling dma_alloc_coherent() will not allow the
> device to find the firmware image in its memory space, and arbitrary
> DMA accesses performed by the device will trigger faults that are
> reported to the OS. So the window between DMA map (or
> dma_alloc_coherent()) and the device specific command to pass the DMA
> buffer address to the device is not inherently unsafe IMO, but I do
> understand the need to cover this scenario.
>
> As I pointed out before, using coherent DMA buffers to perform
> streaming DMA is generally a bad idea, since they may be allocated
> from a finite pool, and may use uncached mappings, making the access
> slower than necessary (while streaming DMA can use any kmalloc'ed
> buffer and will just flush the contents of the caches to main memory
> when the DMA map is performed).
>
> So to summarize again: in my opinion, using a single buffer is not a
> problem as long as the validation completes before the DMA map is
> performed. This will provide the expected guarantees on systems with
> IOMMUs, and will not complicate matters on systems where there is no
> point in obsessing about this anyway given that devices can access all
> of memory whenever they want to.
>
> As for the Qualcomm case: dma_alloc_coherent() is not needed here but
> simply ends up being used because it was already wired up in the
> qualcomm specific secure world API, which amounts to doing syscalls
> into a higher privilege level than the one the kernel itself runs at.
> So again, reasoning about whether the secure world will look at your
> data before you checked the sig is rather pointless, and adding
> special cases to the IMA api to cater for this use case seems like a
> waste of engineering and review effort to me. If we have to do
> something to tie up this loose end, let's try switching it to the
> streaming DMA api instead.
>
Forgot to mention: the Qualcomm case is about passing data to the CPU
running at another privilege level, so IOMMU vs !IOMMU is not a factor
here.
On Tue, 2018-07-10 at 08:56 +0200, Ard Biesheuvel wrote:
> On 10 July 2018 at 08:51, Ard Biesheuvel <[email protected]> wrote:
> > On 9 July 2018 at 21:41, Mimi Zohar <[email protected]> wrote:
> >> On Mon, 2018-07-02 at 17:30 +0200, Ard Biesheuvel wrote:
> >>> On 2 July 2018 at 16:38, Mimi Zohar <[email protected]> wrote:
> >>> > Some systems are memory constrained but they need to load very large
> >>> > firmwares. The firmware subsystem allows drivers to request this
> >>> > firmware be loaded from the filesystem, but this requires that the
> >>> > entire firmware be loaded into kernel memory first before it's provided
> >>> > to the driver. This can lead to a situation where we map the firmware
> >>> > twice, once to load the firmware into kernel memory and once to copy the
> >>> > firmware into the final resting place.
> >>> >
> >>> > To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
> >>> > into a pre-allocated buffer") introduced request_firmware_into_buf() API
> >>> > that allows drivers to request firmware be loaded directly into a
> >>> > pre-allocated buffer. (Based on the mailing list discussions, calling
> >>> > dma_alloc_coherent() is unnecessary and confusing.)
> >>> >
> >>> > (Very broken/buggy) devices using pre-allocated memory run the risk of
> >>> > the firmware being accessible to the device prior to the completion of
> >>> > IMA's signature verification. For the time being, this patch emits a
> >>> > warning, but does not prevent the loading of the firmware.
> >>> >
> >>>
> >>> As I attempted to explain in the exchange with Luis, this has nothing
> >>> to do with broken or buggy devices, but is simply the reality we have
> >>> to deal with on platforms that lack IOMMUs.
> >>
> >>> Even if you load into one buffer, carry out the signature verification
> >>> and *only then* copy it to another buffer, a bus master could
> >>> potentially read it from the first buffer as well. Mapping for DMA
> >>> does *not* mean 'making the memory readable by the device' unless
> >>> IOMMUs are being used. Otherwise, a bus master can read it from the
> >>> first buffer, or even patch the code that performs the security check
> >>> in the first place. For such platforms, copying the data around to
> >>> prevent the device from reading it is simply pointless, as well as any
> >>> other mitigation in software to protect yourself from misbehaving bus
> >>> masters.
> >>
> >> Thank you for taking the time to explain this again.
> >>
> >>> So issuing a warning in this particular case is rather arbitrary. On
> >>> these platforms, all bus masters can read (and modify) all of your
> >>> memory all of the time, and as long as the firmware loader code takes
> >>> care not to provide the DMA address to the device until after the
> >>> verification is complete, it really has done all it reasonably can in
> >>> the environment that it is expected to operate in.
> >>
> >> So for the non-IOMMU system case, differentiating between pre-
> >> allocated buffers vs. using two buffers doesn't make sense.
> >>
> >>>
> >>> (The use of dma_alloc_coherent() is a bit of a red herring here, as it
> >>> incorporates the DMA map operation. However, DMA map is a no-op on
> >>> systems with cache coherent 1:1 DMA [iow, all PCs and most arm64
> >>> platforms unless they have IOMMUs], and so there is not much
> >>> difference between memory allocated with kmalloc() or with
> >>> dma_alloc_coherent() in terms of whether the device can access it
> >>> freely)
> >>
> >> What about systems with an IOMMU?
> >>
> >
> > On systems with an IOMMU, performing the DMA map will create an entry
> > in the IOMMU page tables for the physical region associated with the
> > buffer, making the region accessible to the device. For platforms in
> > this category, using dma_alloc_coherent() for allocating a buffer to
> > pass firmware to the device does open a window where the device could
> > theoretically access this data while the validation is still in
> > progress.
> >
> > Note that the device still needs to be informed about the address of
> > the buffer: just calling dma_alloc_coherent() will not allow the
> > device to find the firmware image in its memory space, and arbitrary
> > DMA accesses performed by the device will trigger faults that are
> > reported to the OS. So the window between DMA map (or
> > dma_alloc_coherent()) and the device specific command to pass the DMA
> > buffer address to the device is not inherently unsafe IMO, but I do
> > understand the need to cover this scenario.
> >
> > As I pointed out before, using coherent DMA buffers to perform
> > streaming DMA is generally a bad idea, since they may be allocated
> > from a finite pool, and may use uncached mappings, making the access
> > slower than necessary (while streaming DMA can use any kmalloc'ed
> > buffer and will just flush the contents of the caches to main memory
> > when the DMA map is performed).
> >
> > So to summarize again: in my opinion, using a single buffer is not a
> > problem as long as the validation completes before the DMA map is
> > performed. This will provide the expected guarantees on systems with
> > IOMMUs, and will not complicate matters on systems where there is no
> > point in obsessing about this anyway given that devices can access all
> > of memory whenever they want to.
It sound like as long as the pre-allocated buffer is not being re-
used, either by being mapped to multiple devices or used to load
multiple firmware blobs, it is safe.
> >
> > As for the Qualcomm case: dma_alloc_coherent() is not needed here but
> > simply ends up being used because it was already wired up in the
> > qualcomm specific secure world API, which amounts to doing syscalls
> > into a higher privilege level than the one the kernel itself runs at.
> > So again, reasoning about whether the secure world will look at your
> > data before you checked the sig is rather pointless, and adding
> > special cases to the IMA api to cater for this use case seems like a
> > waste of engineering and review effort to me. If we have to do
> > something to tie up this loose end, let's try switching it to the
> > streaming DMA api instead.
> >
>
> Forgot to mention: the Qualcomm case is about passing data to the CPU
> running at another privilege level, so IOMMU vs !IOMMU is not a factor
> here.
Agreed. It sounds like the dependency would be on whether the buffer
has been DMA mapped.
Mimi
On Mon 09 Jul 23:56 PDT 2018, Ard Biesheuvel wrote:
> On 10 July 2018 at 08:51, Ard Biesheuvel <[email protected]> wrote:
> > On 9 July 2018 at 21:41, Mimi Zohar <[email protected]> wrote:
> >> On Mon, 2018-07-02 at 17:30 +0200, Ard Biesheuvel wrote:
> >>> On 2 July 2018 at 16:38, Mimi Zohar <[email protected]> wrote:
[..]
> > So to summarize again: in my opinion, using a single buffer is not a
> > problem as long as the validation completes before the DMA map is
> > performed. This will provide the expected guarantees on systems with
> > IOMMUs, and will not complicate matters on systems where there is no
> > point in obsessing about this anyway given that devices can access all
> > of memory whenever they want to.
> >
> > As for the Qualcomm case: dma_alloc_coherent() is not needed here but
> > simply ends up being used because it was already wired up in the
> > qualcomm specific secure world API, which amounts to doing syscalls
> > into a higher privilege level than the one the kernel itself runs at.
As I said before, the dma_alloc_coherent() referred to in this
discussion holds parameters for the Trustzone call, i.e. it will hold
the address to the buffer that the firmware was loaded into - it won't
hold any data that comes from the actual firmware.
> > So again, reasoning about whether the secure world will look at your
> > data before you checked the sig is rather pointless, and adding
> > special cases to the IMA api to cater for this use case seems like a
> > waste of engineering and review effort to me.
Forgive me if I'm missing something in the implementation here, but
aren't the IMA checks done before request_firmware*() returns?
> > If we have to do
> > something to tie up this loose end, let's try switching it to the
> > streaming DMA api instead.
> >
>
> Forgot to mention: the Qualcomm case is about passing data to the CPU
> running at another privilege level, so IOMMU vs !IOMMU is not a factor
> here.
Further more, all scenarios we've look at so far is completely
sequential, so if the firmware request fails we won't invoke the
Trustzone operation that would consume the memory or we won't turn on
the power to the CPU that would execute the firmware.
Tbh the only case I can think of where there would be a "race condition"
here is if we have a device that is polling the last byte of a
predefined firmware memory area for the firmware loader to read some
specific data into it. In cases where the firmware request is followed
by some explicit signalling to the device (or a power on sequence) I'm
unable to see the issue discussed here.
Regards,
Bjorn
Hi Eric,
Can I get your Ack on this patch?
Mimi
On Mon, 2018-07-02 at 10:37 -0400, Mimi Zohar wrote:
> In order for LSMs and IMA-appraisal to differentiate between kexec_load
> and kexec_file_load syscalls, both the original and new syscalls must
> call an LSM hook. This patch adds a call to security_kernel_load_data()
> in the original kexec_load syscall.
>
> Signed-off-by: Mimi Zohar <[email protected]>
> Cc: Eric Biederman <[email protected]>
> Cc: Luis R. Rodriguez <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: David Howells <[email protected]>
> Acked-by: Serge Hallyn <[email protected]>
> ---
> kernel/kexec.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index aed8fb2564b3..68559808fdfa 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -11,6 +11,7 @@
> #include <linux/capability.h>
> #include <linux/mm.h>
> #include <linux/file.h>
> +#include <linux/security.h>
> #include <linux/kexec.h>
> #include <linux/mutex.h>
> #include <linux/list.h>
> @@ -195,10 +196,17 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments,
> static inline int kexec_load_check(unsigned long nr_segments,
> unsigned long flags)
> {
> + int result;
> +
> /* We only trust the superuser with rebooting the system. */
> if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> return -EPERM;
>
> + /* Permit LSMs and IMA to fail the kexec */
> + result = security_kernel_load_data(LOADING_KEXEC_IMAGE);
> + if (result < 0)
> + return result;
> +
> /*
> * Verify we have a legal set of flags
> * This leaves us room for future extensions.
On 10 July 2018 at 21:19, Bjorn Andersson <[email protected]> wrote:
> On Mon 09 Jul 23:56 PDT 2018, Ard Biesheuvel wrote:
>
>> On 10 July 2018 at 08:51, Ard Biesheuvel <[email protected]> wrote:
>> > On 9 July 2018 at 21:41, Mimi Zohar <[email protected]> wrote:
>> >> On Mon, 2018-07-02 at 17:30 +0200, Ard Biesheuvel wrote:
>> >>> On 2 July 2018 at 16:38, Mimi Zohar <[email protected]> wrote:
> [..]
>> > So to summarize again: in my opinion, using a single buffer is not a
>> > problem as long as the validation completes before the DMA map is
>> > performed. This will provide the expected guarantees on systems with
>> > IOMMUs, and will not complicate matters on systems where there is no
>> > point in obsessing about this anyway given that devices can access all
>> > of memory whenever they want to.
>> >
>> > As for the Qualcomm case: dma_alloc_coherent() is not needed here but
>> > simply ends up being used because it was already wired up in the
>> > qualcomm specific secure world API, which amounts to doing syscalls
>> > into a higher privilege level than the one the kernel itself runs at.
>
> As I said before, the dma_alloc_coherent() referred to in this
> discussion holds parameters for the Trustzone call, i.e. it will hold
> the address to the buffer that the firmware was loaded into - it won't
> hold any data that comes from the actual firmware.
>
Ah yes, I forgot that detail. Thanks for reminding me.
>> > So again, reasoning about whether the secure world will look at your
>> > data before you checked the sig is rather pointless, and adding
>> > special cases to the IMA api to cater for this use case seems like a
>> > waste of engineering and review effort to me.
>
> Forgive me if I'm missing something in the implementation here, but
> aren't the IMA checks done before request_firmware*() returns?
>
The issue under discussion is whether calling request_firmware() to
load firmware into a buffer that may be readable by the device while
the IMA checks are in progress constitutes a security hazard.
>> > If we have to do
>> > something to tie up this loose end, let's try switching it to the
>> > streaming DMA api instead.
>> >
>>
>> Forgot to mention: the Qualcomm case is about passing data to the CPU
>> running at another privilege level, so IOMMU vs !IOMMU is not a factor
>> here.
>
> Further more, all scenarios we've look at so far is completely
> sequential, so if the firmware request fails we won't invoke the
> Trustzone operation that would consume the memory or we won't turn on
> the power to the CPU that would execute the firmware.
>
>
> Tbh the only case I can think of where there would be a "race condition"
> here is if we have a device that is polling the last byte of a
> predefined firmware memory area for the firmware loader to read some
> specific data into it. In cases where the firmware request is followed
> by some explicit signalling to the device (or a power on sequence) I'm
> unable to see the issue discussed here.
>
I agree. But the latter part is platform specific, and so it requires
some degree of trust in the driver author on the part of the IMA
routines that request_firmware() is called at an appropriate time.
The point I am trying to make in this thread is that there are cases
where it makes no sense for the kernel to reason about these things,
given that higher privilege levels such as the TrustZone secure world
own the kernel's execution context entirely already, and given that
masters that are not behind an IOMMU can read and write all of memory
all of the time anyway.
The bottom line is that reality does not respect the layering that IMA
assumes, and so the only meaningful way to treat some of the use cases
is simply to ignore them entirely. So we should still perform all the
checks, but we will have to live with the limited utility of doing so
in some scenarios (and not print nasty warnings to the kernel log for
such cases)
--
Ard.
On Wed, 2018-07-11 at 08:24 +0200, Ard Biesheuvel wrote:
> On 10 July 2018 at 21:19, Bjorn Andersson <[email protected]> wrote:
> > Tbh the only case I can think of where there would be a "race condition"
> > here is if we have a device that is polling the last byte of a
> > predefined firmware memory area for the firmware loader to read some
> > specific data into it. In cases where the firmware request is followed
> > by some explicit signalling to the device (or a power on sequence) I'm
> > unable to see the issue discussed here.
> >
>
> I agree. But the latter part is platform specific, and so it requires
> some degree of trust in the driver author on the part of the IMA
> routines that request_firmware() is called at an appropriate time.
Exactly. Qualcomm could be using the pre-allocated buffer
appropriately, but that doesn't guarantee how it will be used in the
future.
> The point I am trying to make in this thread is that there are cases
> where it makes no sense for the kernel to reason about these things,
> given that higher privilege levels such as the TrustZone secure world
> own the kernel's execution context entirely already, and given that
> masters that are not behind an IOMMU can read and write all of memory
> all of the time anyway.
> The bottom line is that reality does not respect the layering that IMA
> assumes, and so the only meaningful way to treat some of the use cases
> is simply to ignore them entirely. So we should still perform all the
> checks, but we will have to live with the limited utility of doing so
> in some scenarios (and not print nasty warnings to the kernel log for
> such cases)
You have convinced me that the warning shouldn't be emitted in either
the non IOMMU or in the IOMMU case, assuming the buffer has not been
DMA mapped.
The remaining concern is using the same buffer mapped to multiple
devices or re-using the same buffer to load multiple firmware blobs.
I'm not sure how easy that would be to detect.
I need to stage the rest of the patch set to be upstreamed. Could we
just add a comment in the code reflecting this discussion?
Mimi
On Thu 12 Jul 13:03 PDT 2018, Mimi Zohar wrote:
> On Wed, 2018-07-11 at 08:24 +0200, Ard Biesheuvel wrote:
> > On 10 July 2018 at 21:19, Bjorn Andersson <[email protected]> wrote:
>
> > > Tbh the only case I can think of where there would be a "race condition"
> > > here is if we have a device that is polling the last byte of a
> > > predefined firmware memory area for the firmware loader to read some
> > > specific data into it. In cases where the firmware request is followed
> > > by some explicit signalling to the device (or a power on sequence) I'm
> > > unable to see the issue discussed here.
> > >
> >
> > I agree. But the latter part is platform specific, and so it requires
> > some degree of trust in the driver author on the part of the IMA
> > routines that request_firmware() is called at an appropriate time.
>
> Exactly. ?Qualcomm could be using the pre-allocated buffer
> appropriately, but that doesn't guarantee how it will be used in the
> future.
>
Agreed.
But a device that starts operate on the firmware memory before it's
fully loaded (and verified) will likely hit other nasty issues. Using a
traditional request_firmware() and memcpy() or simply mapping the buffer
into the remote piecemeal would have the same issue.
So I think that regardless of using IMA, if you don't have the ability
to control your device's view of the entire firmware buffer atomically
you must write your device drivers accordingly.
Regards,
Bjorn