Re: [PATCH v2 2/5 RFC] use event name instead of enum to make the call generic

From: prsriva
Date: Thu Apr 25 2019 - 13:19:25 EST

On 2019-04-25 4:48 a.m., Nayna wrote:


On 04/23/2019 08:15 PM, Prakhar Srivastava wrote:
From: Prakhar Srivastava <prsriva02@xxxxxxxxx>

Signed-off-by: Prakhar Srivastava <prsriva@xxxxxxxxxxxxx>
---

Currently for soft reboot(kexec_file_load) the kernel file and
signature is measured by IMA. The cmdline args used to load the kernel
is not measured.
The boot aggregate that gets calculated will have no change since the
EFI loader has not been triggered.
Adding the kexec cmdline args measure and kernel version will add some
attestable criteria.


Any reason for including the whole commit message after "---"

Anything after "---" is not included in the patch description when patch is applied.

This comment applies to all the patches in this patchset.
I will fix the comments and send out the patchset with a cover letter. Thankyou for pointing this out.

remove enums to control type of buffers entries, instead pass the event name to be used.

Is the last statement meant to be a Changelog from v1-> v2 ? Only the changelog has to be after "---"

Also, If posting more than one patch, it is preferrable to add a cover-letter.
I will add a cover letter alongside fixing the comments. Thankyou!


 include/linux/ima.h | 10 ++--------
 kernel/kexec_file.c | 3 +++
 security/integrity/ima/ima.h | 2 +-
 security/integrity/ima/ima_main.c | 30 ++++++++++--------------------
 4 files changed, 16 insertions(+), 29 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 733d0cb9dedc..5e41507c57e5 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -14,12 +14,6 @@
 #include <linux/kexec.h>
 struct linux_binprm;

-enum __buffer_id {
-ÂÂÂ KERNEL_VERSION,
-ÂÂÂ KEXEC_CMDLINE,
-ÂÂÂ MAX_BUFFER_ID = KEXEC_CMDLINE
-} buffer_id;
-

Is the v2 version created on top of the v1 version that was posted ?

v2 is based off the HEAD of the repo.
The v2 version has to be on top of the HEAD of the repository itself, and not on the v1 version. Only the final reviewed and tested version makes to the upstream.

Btw, which repository and its branch are you using ?

I am basing my changes off IMA branch:
git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
Thanks & Regards,
ÂÂÂÂÂ - Nayna




 #ifdef CONFIG_IMA
 extern int ima_bprm_check(struct linux_binprm *bprm);
 extern int ima_file_check(struct file *file, int mask, int opened);
@@ -29,7 +23,7 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
 extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id);
 extern void ima_post_path_mknod(struct dentry *dentry);
-extern void ima_buffer_check(const void *buff, int size, enum buffer_id id);
+extern void ima_buffer_check(const void *buff, int size, char *eventname);
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);
 #endif
@@ -72,7 +66,7 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
 }

 static inline void ima_buffer_check(const void *buff, int size,
-ÂÂÂÂÂÂÂÂÂÂÂ enum buffer_id id)
+ÂÂÂÂÂÂÂÂÂÂÂ char *eventname)
 {
ÂÂÂÂÂ return;
 }
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index b118735fea9d..2a5234eb4b28 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -182,6 +182,9 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
ÂÂÂÂÂÂÂÂÂÂÂÂÂ ret = -EINVAL;
ÂÂÂÂÂÂÂÂÂÂÂÂÂ goto out;
ÂÂÂÂÂÂÂÂÂ }
+
+ÂÂÂÂÂÂÂ ima_buffer_check(image->cmdline_buf, cmdline_len - 1,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "kexec_cmdline");
ÂÂÂÂÂ }

ÂÂÂÂÂ /* Call arch image load handlers */
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index b71f2f6f7421..fcade3c103ed 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -181,8 +181,8 @@ enum ima_hooks {
ÂÂÂÂÂ FIRMWARE_CHECK,
ÂÂÂÂÂ KEXEC_KERNEL_CHECK,
ÂÂÂÂÂ KEXEC_INITRAMFS_CHECK,
-ÂÂÂ BUFFER_CHECK,
ÂÂÂÂÂ POLICY_CHECK,
+ÂÂÂ BUFFER_CHECK,
ÂÂÂÂÂ MAX_CHECK
 };

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 6408cadaadbb..da82c705a5ed 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -160,8 +160,7 @@ void ima_file_free(struct file *file)
ÂÂ * (Instead of using the file hash the buffer hash is used).
ÂÂ * @buff - The buffer that needs to be added to the log
ÂÂ * @size - size of buffer(in bytes)
- * @id - buffer id, this is differentiator for the various buffers
- * that can be measured.
+ * @id - eventname, event name to be used for buffer measurement.
ÂÂ *
ÂÂ * The buffer passed is added to the ima logs.
ÂÂ * If the sig template is used, then the sig field contains the buffer.
@@ -170,7 +169,7 @@ void ima_file_free(struct file *file)
ÂÂ * On error cases surface errors from ima calls.
ÂÂ */
 static int process_buffer_measurement(const void *buff, int size,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum buffer_id id)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ char *eventname)
 {
ÂÂÂÂÂ int ret = -EINVAL;
ÂÂÂÂÂ struct ima_template_entry *entry = NULL;
@@ -185,23 +184,13 @@ static int process_buffer_measurement(const void *buff, int size,
ÂÂÂÂÂ int violation = 0;
ÂÂÂÂÂ int pcr = CONFIG_IMA_MEASURE_PCR_IDX;

-ÂÂÂ if (!buff || size ==Â 0)
+ÂÂÂ if (!buff || size ==Â 0 || !eventname)
ÂÂÂÂÂÂÂÂÂ goto err_out;

ÂÂÂÂÂ if (ima_get_action(NULL, 0, BUFFER_CHECK, &pcr) != IMA_MEASURE)
ÂÂÂÂÂÂÂÂÂ goto err_out;

-ÂÂÂ switch (buffer_id) {
-ÂÂÂ case KERNEL_VERSION:
-ÂÂÂÂÂÂÂ name = "Kernel-version";
-ÂÂÂÂÂÂÂ break;
-ÂÂÂ case KEXEC_CMDLINE:
-ÂÂÂÂÂÂÂ name = "Kexec-cmdline";
-ÂÂÂÂÂÂÂ break;
-ÂÂÂ default:
-ÂÂÂÂÂÂÂ goto err_out;
-ÂÂÂ }
-
+ÂÂÂ name = eventname;
ÂÂÂÂÂ memset(iint, 0, sizeof(*iint));
ÂÂÂÂÂ memset(&hash, 0, sizeof(hash));

@@ -452,15 +441,16 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
ÂÂ * ima_buffer_check - based on policy, collect & store buffer measurement
ÂÂ * @buf: pointer to buffer
ÂÂ * @size: size of buffer
- * @buffer_id: caller identifier
+ * @eventname: caller identifier
ÂÂ *
 * Buffers can only be measured, not appraised. The buffer identifier
- * is used as the measurement list entry name (eg. boot_cmdline).
+ * is used as the measurement list entry name (eg. boot_cmdline,
+ * kernel_version).
ÂÂ */
-void ima_buffer_check(const void *buf, int size, enum buffer_id id)
+void ima_buffer_check(const void *buf, int size, char *eventname)
 {
-ÂÂÂ if (buf && size != 0)
-ÂÂÂÂÂÂÂ process_buffer_measurement(buf, size, id);
+ÂÂÂ if (buf && size != 0 && eventname)
+ÂÂÂÂÂÂÂ process_buffer_measurement(buf, size, eventname);

ÂÂÂÂÂ return;
 }