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 <keesc...@chromium.org>
Other than one change and one question below, it looks good. Reviewed-by: Mimi Zohar <zo...@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; > } >