Messages in this thread | | | Subject | Re: [PATCH v2 2/5 RFC] use event name instead of enum to make the call generic | From | prsriva <> | Date | Thu, 25 Apr 2019 10:19:19 -0700 |
| |
On 2019-04-25 4:48 a.m., Nayna wrote: > > > On 04/23/2019 08:15 PM, Prakhar Srivastava wrote: >> From: Prakhar Srivastava <prsriva02@gmail.com> >> >> Signed-off-by: Prakhar Srivastava <prsriva@microsoft.com> >> --- >> >> 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; >> }
| |