diff mbox series

[v6,8/8] ima: add FIRMWARE_PARTIAL_READ support

Message ID 20200605225959.12424-9-scott.branden@broadcom.com (mailing list archive)
State New, archived
Headers show
Series firmware: add partial read support in request_firmware_into_buf | expand

Commit Message

Scott Branden June 5, 2020, 10:59 p.m. UTC 
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(-)

Comments

Mimi Zohar June 5, 2020, 11:19 p.m. UTC | #1 
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)) {
Scott Branden June 5, 2020, 11:31 p.m. UTC | #2 
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.
>
> 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


>
> Mimi
>
>> +
>>   	if (!file && read_id == READING_FIRMWARE) {
>>   		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
>>   		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
Mimi Zohar June 5, 2020, 11:48 p.m. UTC | #3 
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 mbox series

Patch

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)) {