| Message ID | 20200605225959.12424-9-scott.branden@broadcom.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v6,1/8] fs: introduce kernel_pread_file* support | expand |
Hi Scott, On Fri, 2020-06-05 at 15:59 -0700, Scott Branden wrote: > > @@ -648,6 +667,9 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size, > enum ima_hooks func; > u32 secid; > > + if (!file && read_id == READING_FIRMWARE_PARTIAL_READ) > + return 0; The file should be measured on the pre security hook, not here on the post security hook. Here, whether "file" is defined or not, is irrelevant. The test should just check "read_id". Have you tested measuring the firmware by booting a system with "ima_policy=tcb" specified on the boot command line and compared the measurement entry in the IMA measurement list with the file hash (eg. sha1sum, sha256sum)? Mimi > + > if (!file && read_id == READING_FIRMWARE) { > if ((ima_appraise & IMA_APPRAISE_FIRMWARE) && > (ima_appraise & IMA_APPRAISE_ENFORCE)) {
On Fri, 2020-06-05 at 16:31 -0700, Scott Branden wrote: > Hi Mimi, > > On 2020-06-05 4:19 p.m., Mimi Zohar wrote: > > Hi Scott, > > > > On Fri, 2020-06-05 at 15:59 -0700, Scott Branden wrote: > >> @@ -648,6 +667,9 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size, > >> enum ima_hooks func; > >> u32 secid; > >> > >> + if (!file && read_id == READING_FIRMWARE_PARTIAL_READ) > >> + return 0; > > The file should be measured on the pre security hook, not here on the > > post security hook. Here, whether "file" is defined or not, is > > irrelevant. The test should just check "read_id". > OK, will remove the !file from here. thanks! > > > > Have you tested measuring the firmware by booting a system with > > "ima_policy=tcb" specified on the boot command line and compared the > > measurement entry in the IMA measurement list with the file hash (eg. > > sha1sum, sha256sum)? > Yes, I enabled IMA in my kernel and added ima_policy=tsb to the boot > command line, > > Here are the entries from > /sys/kernel/security/ima/ascii_runtime_measurements of the files I am > accessing. > Please let me know if I am doing anything incorrectly. > > 10 4612bce355b2dbc45ecd95e17001636be8832c7f ima-ng > sha1:fddd9a28c2b15acf3b0fc9ec0cf187cb2153d7f2 > /lib/firmware/vk-boot1-bcm958401m2.ecdsa.bin > 10 4c0eb0fc30eb7ac3a30a27f05c1d2a8d28d6a9ec ima-ng > sha1:b16d343dd63352d10309690c71b110762a9444c3 > /lib/firmware/vk-boot2-bcm958401m2_a72.ecdsn > > The sha1 sum matches: > root@genericx86-64:/sys/kernel/security/ima# sha1sum > /lib/firmware/vk-boot1-bcm958401m2.ecdsa.bin > fddd9a28c2b15acf3b0fc9ec0cf187cb2153d7f2 > /lib/firmware/vk-boot1-bcm958401m2.ecdsa.bin > > root@genericx86-64:/sys/kernel/security/ima# sha1sum > /lib/firmware/vk-boot2-bcm958401m2_a72.ecdsa.bin > b16d343dd63352d10309690c71b110762a9444c3 > /lib/firmware/vk-boot2-bcm958401m2_a72.ecdsa.bin Looks good! (FYI, a larger hash algorithm can be specified in the Kconfig or "ima_hash=" on the boot command line.) Mimi
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index 93e7fee42cd4..d0c42194af17 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -483,7 +483,11 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv, /* Already populated data member means we're loading into a buffer */ if (!decompress && fw_priv->data) { buffer = fw_priv->data; - id = READING_FIRMWARE_PREALLOC_BUFFER; + if (fw_priv->opt == KERNEL_PREAD_PART) + id = READING_FIRMWARE_PARTIAL_READ; + else + id = READING_FIRMWARE_PREALLOC_BUFFER; + msize = fw_priv->allocated_size; } diff --git a/fs/exec.c b/fs/exec.c index e5c241c07b75..3fbc2fee909f 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -968,7 +968,8 @@ int kernel_pread_file(struct file *file, void **buf, loff_t *size, goto out; } - if (id != READING_FIRMWARE_PREALLOC_BUFFER) + if ((id != READING_FIRMWARE_PARTIAL_READ) && + (id != READING_FIRMWARE_PREALLOC_BUFFER)) *buf = vmalloc(alloc_size); if (!*buf) { ret = -ENOMEM; @@ -1000,7 +1001,8 @@ int kernel_pread_file(struct file *file, void **buf, loff_t *size, out_free: if (ret < 0) { - if (id != READING_FIRMWARE_PREALLOC_BUFFER) { + if ((id != READING_FIRMWARE_PARTIAL_READ) && + (id != READING_FIRMWARE_PREALLOC_BUFFER)) { vfree(*buf); *buf = NULL; } diff --git a/include/linux/fs.h b/include/linux/fs.h index 76d463e4a628..3affcaa7c7b2 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -3020,6 +3020,7 @@ extern int do_pipe_flags(int *, int); #define __kernel_read_file_id(id) \ id(UNKNOWN, unknown) \ id(FIRMWARE, firmware) \ + id(FIRMWARE_PARTIAL_READ, firmware) \ id(FIRMWARE_PREALLOC_BUFFER, firmware) \ id(FIRMWARE_EFI_EMBEDDED, firmware) \ id(MODULE, kernel-module) \ diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 800fb3bba418..982debd59cc4 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -609,6 +609,9 @@ void ima_post_path_mknod(struct dentry *dentry) */ int ima_read_file(struct file *file, enum kernel_read_file_id read_id) { + enum ima_hooks func; + u32 secid; + /* * READING_FIRMWARE_PREALLOC_BUFFER * @@ -617,11 +620,27 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id) * of IMA's signature verification any more than when using two * buffers? */ - return 0; + if (read_id != READING_FIRMWARE_PARTIAL_READ) + return 0; + + if (!file) { + if ((ima_appraise & IMA_APPRAISE_FIRMWARE) && + (ima_appraise & IMA_APPRAISE_ENFORCE)) { + pr_err("Prevent firmware loading_store.\n"); + return -EACCES; /* INTEGRITY_UNKNOWN */ + } + return 0; + } + + func = read_idmap[read_id] ?: FILE_CHECK; + security_task_getsecid(current, &secid); + return process_measurement(file, current_cred(), secid, NULL, + 0, MAY_READ, func); } const int read_idmap[READING_MAX_ID] = { [READING_FIRMWARE] = FIRMWARE_CHECK, + [READING_FIRMWARE_PARTIAL_READ] = FIRMWARE_CHECK, [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK, [READING_MODULE] = MODULE_CHECK, [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK, @@ -648,6 +667,9 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size, enum ima_hooks func; u32 secid; + if (!file && read_id == READING_FIRMWARE_PARTIAL_READ) + return 0; + if (!file && read_id == READING_FIRMWARE) { if ((ima_appraise & IMA_APPRAISE_FIRMWARE) && (ima_appraise & IMA_APPRAISE_ENFORCE)) {
Add FIRMWARE_PARTIAL_READ support for integrity measurement on partial reads of firmware files. Signed-off-by: Scott Branden <scott.branden@broadcom.com> --- drivers/base/firmware_loader/main.c | 6 +++++- fs/exec.c | 6 ++++-- include/linux/fs.h | 1 + security/integrity/ima/ima_main.c | 24 +++++++++++++++++++++++- 4 files changed, 33 insertions(+), 4 deletions(-)