Re: [PATCH v4 10/17] firmware_loader: Use security_post_load_data()
From: Mimi Zohar <zohar@linux.ibm.com>
Date: 2020-08-06 22:08:15
Also in:
linux-efi, linux-integrity, linux-kselftest, lkml, selinux
On Wed, 2020-07-29 at 10:58 -0700, Kees Cook wrote:
Now that security_post_load_data() is wired up, use it instead
of the NULL file argument style of security_post_read_file(),
and update the security_kernel_load_data() call to indicate that a
security_kernel_post_load_data() call is expected.
Wire up the IMA check to match earlier logic. Perhaps a generalized
change to ima_post_load_data() might look something like this:
return process_buffer_measurement(buf, size,
kernel_load_data_id_str(load_id),
read_idmap[load_id] ?: FILE_CHECK,
0, NULL);
Signed-off-by: Kees Cook <redacted>Other than one change and one question below, it looks good. Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> <snip>
quoted hunk ↗ jump to hunk
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 85000dc8595c..1a7bc4c7437d 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c
quoted hunk ↗ jump to hunk
@@ -706,7 +697,7 @@ int ima_load_data(enum kernel_load_data_id id, bool contents) } break; case LOADING_FIRMWARE: - if (ima_enforce && (ima_appraise & IMA_APPRAISE_FIRMWARE)) { + if (ima_enforce && (ima_appraise & IMA_APPRAISE_FIRMWARE) && !contents) { pr_err("Prevent firmware sysfs fallback loading.\n");
Appended signatures are limited to kernel modules and, more recently, to the kexec kernel image, not firmware. Without a file descriptor, file signatures stored as an xattrs are not applicable either. We might as well fail earlier, rather than later. Adding "!contents" is unnecessary.
quoted hunk ↗ jump to hunk
return -EACCES; /* INTEGRITY_UNKNOWN */ }@@ -739,6 +730,15 @@ int ima_load_data(enum kernel_load_data_id id, bool contents) */ int ima_post_load_data(char *buf, loff_t size, enum kernel_load_data_id load_id) { + if (load_id == LOADING_FIRMWARE) { + if ((ima_appraise & IMA_APPRAISE_FIRMWARE) && + (ima_appraise & IMA_APPRAISE_ENFORCE)) { + pr_err("Prevent firmware loading_store.\n"); + return -EACCES; /* INTEGRITY_UNKNOWN */ + } + return 0; + }
Even with failing LOADING_FIRMWARE early in ima_load_data(), is this still needed for fw_sysfs_loading()? thanks, Mimi
+ return 0; }