Re: [PATCH v4 10/17] firmware_loader: Use security_post_load_data()

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 keescook@chromium.org

Other than one change and one question below, it looks good.

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

<snip>

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

@@ -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.

	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;

}