Linux-Integrity Archive on lore.kernel.org
 help / Atom feed
* [PATCH 0/3 v5] Kexec cmdline bufffer measure
@ 2019-05-10 22:37 Prakhar Srivastava
  2019-05-10 22:37 ` [PATCH 1/3 v5] add a new ima hook and policy to measure the cmdline Prakhar Srivastava
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Prakhar Srivastava @ 2019-05-10 22:37 UTC (permalink / raw)
  To: linux-integrity, inux-security-module, linux-kernel
  Cc: zohar, ebiederm, vgoyal, prsriva, Prakhar Srivastava

From: Prakhar Srivastava <prsriva02@gmail.com>

The motive behind the patch series is to measure the cmdline args
used for soft reboot/kexec case.

For secure boot attestation, it is necessary to measure the kernel
command line and the kernel version. For cold boot, the boot loader
can be enhanced to measure these parameters.
(https://mjg59.dreamwidth.org/48897.html)
However, for attestation across soft reboot boundary, these values 
also need to be measured during kexec.

Currently for Kexec(kexec_file_load)/soft reboot scenario the cmdline
args that are used to boot the next kernel is not measured. For 
normal case of boot/hardreboot the cmdline args are measured into the TPM.

The hash of boot command line is calculated and added to the current 
running kernel's measurement list.  On a soft reboot like kexec, the PCRs
are not reset to zero.  Refer to commit 94c3aac567a9 ("ima: on soft 
reboot, restore the measurement list") patch description.

To achive the above the patch series does the following
  -adds a new ima hook: ima_kexec_cmdline which measures the cmdline args
   into the ima log, behind a new ima policy entry KEXEC_CMDLINE.
  -since the cmldine args cannot be appraised, a new template field(buf) is
   added. The template field contains the buffer passed(cmldine args), which
   can be used to appraise/attest at a later stage.
  -call the ima_kexec_cmdline(...) hook from kexec_file_load call.

The ima logs need to carried over to the next kernel, which will be followed
up by other patchsets for x86_64 and arm64.


Changelog:
v5:
  -add a new ima hook and policy to measure the cmdline
    args(ima_kexec_cmdline)
  -add a new template field buf to contain the buffer measured.
    [suggested by Mimi Zohar]
  -call ima_kexec_cmdline from kexec_file_load path

v4:
  - per feedback from LSM community, removed the LSM hook and renamed the
    IMA policy to KEXEC_CMDLINE

v3: (rebase changes to next-general)
  - Add policy checks for buffer[suggested by Mimi Zohar]
  - use the IMA_XATTR to add buffer
  - Add kexec_cmdline used for kexec file load
  - Add an LSM hook to allow usage by other LSM.[suggestd by Mimi Zohar]

v2:
  - Add policy checks for buffer[suggested by Mimi Zohar]
  - Add an LSM hook to allow usage by other LSM.[suggestd by Mimi Zohar]
  - use the IMA_XATTR to add buffer instead of sig template

v1:
  -Add kconfigs to control the ima_buffer_check
  -measure the cmdline args suffixed with the kernel file name
  -add the buffer to the template sig field.


Prakhar Srivastava (3):
  add a new ima hook and policy to measure the cmdline
  add a new template field buf to contain the buffer
  call ima_kexec_cmdline from kexec_file_load path

 Documentation/ABI/testing/ima_policy      |   1 +
 include/linux/ima.h                       |   2 +
 kernel/kexec_file.c                       |   2 +
 security/integrity/ima/ima.h              |   1 +
 security/integrity/ima/ima_api.c          |   1 +
 security/integrity/ima/ima_main.c         | 107 ++++++++++++++++++++++
 security/integrity/ima/ima_policy.c       |   9 ++
 security/integrity/ima/ima_template.c     |   2 +
 security/integrity/ima/ima_template_lib.c |  21 +++++
 security/integrity/ima/ima_template_lib.h |   4 +
 security/integrity/integrity.h            |   1 +
 11 files changed, 151 insertions(+)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/3 v5] add a new ima hook and policy to measure the cmdline
  2019-05-10 22:37 [PATCH 0/3 v5] Kexec cmdline bufffer measure Prakhar Srivastava
@ 2019-05-10 22:37 ` Prakhar Srivastava
  2019-05-13 16:56   ` Mimi Zohar
  2019-05-10 22:37 ` [PATCH 2/3 v5] add a new template field buf to contain the buffer Prakhar Srivastava
  2019-05-10 22:37 ` [PATCH 3/3 v5] call ima_kexec_cmdline from kexec_file_load path Prakhar Srivastava
  2 siblings, 1 reply; 14+ messages in thread
From: Prakhar Srivastava @ 2019-05-10 22:37 UTC (permalink / raw)
  To: linux-integrity, inux-security-module, linux-kernel
  Cc: zohar, ebiederm, vgoyal, prsriva, Prakhar Srivastava

From: Prakhar Srivastava <prsriva02@gmail.com>

For secure boot attestation, it is necessary to measure the kernel
command line and the kernel version. For cold boot, the boot loader
can be enhanced to measure these parameters. However, for attestation
across soft reboot boundary, these values also need to be measured
during kexec.

For this reason, this patch adds support for measuring these
parameters during kexec. To achive this, a new ima policy and
hook id, defined KEXEC_CMDLINE and ima_kexec_cmdline respectively,
are added.

Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
---
 Documentation/ABI/testing/ima_policy |  1 +
 include/linux/ima.h                  |  2 +
 security/integrity/ima/ima.h         |  1 +
 security/integrity/ima/ima_api.c     |  1 +
 security/integrity/ima/ima_main.c    | 84 ++++++++++++++++++++++++++++
 security/integrity/ima/ima_policy.c  |  9 +++
 6 files changed, 98 insertions(+)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 74c6702de74e..62e7cd687e9c 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -29,6 +29,7 @@ Description:
 		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
+				[KEXEC_CMDLINE]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
 			       [[^]MAY_EXEC]
 			fsmagic:= hex value
diff --git a/include/linux/ima.h b/include/linux/ima.h
index dc12fbcf484c..2e2c77280be8 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -26,6 +26,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_kexec_cmdline(const void *buf, int size);
 
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);
@@ -92,6 +93,7 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
 	return;
 }
 
+static inline void ima_kexec_cmdline(const void *buf, int size) {}
 #endif /* CONFIG_IMA */
 
 #ifndef CONFIG_IMA_KEXEC
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d213e835c498..226a26d8de09 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -184,6 +184,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
 	hook(KEXEC_KERNEL_CHECK)	\
 	hook(KEXEC_INITRAMFS_CHECK)	\
 	hook(POLICY_CHECK)		\
+	hook(KEXEC_CMDLINE)		\
 	hook(MAX_CHECK)
 #define __ima_hook_enumify(ENUM)	ENUM,
 
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c7505fb122d4..800d965232e5 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -169,6 +169,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  *		subj=, obj=, type=, func=, mask=, fsmagic=
  *	subj,obj, and type: are LSM specific.
  *	func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
+ *	| KEXEC_CMDLINE
  *	mask: contains the permission mask
  *	fsmagic: hex value
  *
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 357edd140c09..1d186bda25fe 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -576,6 +576,90 @@ int ima_load_data(enum kernel_load_data_id id)
 	return 0;
 }
 
+/*
+ * process_buffer_measurement - Measure the buffer passed to ima log.
+ * (Instead of using the file hash use the buffer hash).
+ * @buf - The buffer that needs to be added to the log
+ * @size - size of buffer(in bytes)
+ * @eventname - event name to be used for buffer.
+ *
+ * The buffer passed is added to the ima log.
+ *
+ * On success return 0.
+ * On error cases surface errors from ima calls.
+ */
+static int process_buffer_measurement(const void *buf, int size,
+				const char *eventname, const struct cred *cred,
+				u32 secid)
+{
+	int ret = 0;
+	struct ima_template_entry *entry = NULL;
+	struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
+	struct ima_event_data event_data = {iint, NULL, NULL,
+						NULL, 0, NULL};
+	struct {
+		struct ima_digest_data hdr;
+		char digest[IMA_MAX_DIGEST_SIZE];
+	} hash;
+	int violation = 0;
+	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
+	int action = 0;
+
+	action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr);
+	if (!(action & IMA_AUDIT) && !(action & IMA_MEASURE))
+		goto out;
+
+	memset(iint, 0, sizeof(*iint));
+	memset(&hash, 0, sizeof(hash));
+
+	event_data.filename = eventname;
+
+	iint->ima_hash = &hash.hdr;
+	iint->ima_hash->algo = ima_hash_algo;
+	iint->ima_hash->length = hash_digest_size[ima_hash_algo];
+
+	ret = ima_calc_buffer_hash(buf, size, iint->ima_hash);
+	if (ret < 0)
+		goto out;
+
+	ret = ima_alloc_init_template(&event_data, &entry);
+	if (ret < 0)
+		goto out;
+
+	if (action & IMA_MEASURE)
+		ret = ima_store_template(entry, violation, NULL, buf, pcr);
+
+	if (action & IMA_AUDIT)
+		ima_audit_measurement(iint, event_data.filename);
+
+	if (ret < 0) {
+		ima_free_template_entry(entry);
+		goto out;
+	}
+
+out:
+	return ret;
+}
+
+/**
+ * ima_kexec_cmdline - based on policy, store kexec cmdline args
+ * @buf: pointer to buffer
+ * @size: size of buffer
+ *
+ * Buffers can only be measured, not appraised.
+ */
+void ima_kexec_cmdline(const void *buf, int size)
+{
+	u32 secid;
+
+	if (buf && size != 0) {
+		security_task_getsecid(current, &secid);
+		process_buffer_measurement(buf, size, "kexec-cmdline",
+				current_cred(), secid);
+	}
+}
+
+
 static int __init init_ima(void)
 {
 	int error;
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e0cc323f948f..413e5921b248 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -291,6 +291,13 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 {
 	int i;
 
+	/* only incase of KEXEC_CMDLINE, inode is NULL */
+	if (func == KEXEC_CMDLINE) {
+		if ((rule->flags & IMA_FUNC) &&
+			(rule->func == func) && (!inode))
+			return true;
+		return false;
+	}
 	if ((rule->flags & IMA_FUNC) &&
 	    (rule->func != func && func != POST_SETATTR))
 		return false;
@@ -869,6 +876,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				entry->func = KEXEC_INITRAMFS_CHECK;
 			else if (strcmp(args[0].from, "POLICY_CHECK") == 0)
 				entry->func = POLICY_CHECK;
+			else if (strcmp(args[0].from, "KEXEC_CMDLINE") == 0)
+				entry->func = KEXEC_CMDLINE;
 			else
 				result = -EINVAL;
 			if (!result)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/3 v5] add a new template field buf to contain the buffer
  2019-05-10 22:37 [PATCH 0/3 v5] Kexec cmdline bufffer measure Prakhar Srivastava
  2019-05-10 22:37 ` [PATCH 1/3 v5] add a new ima hook and policy to measure the cmdline Prakhar Srivastava
@ 2019-05-10 22:37 ` Prakhar Srivastava
  2019-05-13 13:48   ` Roberto Sassu
  2019-05-10 22:37 ` [PATCH 3/3 v5] call ima_kexec_cmdline from kexec_file_load path Prakhar Srivastava
  2 siblings, 1 reply; 14+ messages in thread
From: Prakhar Srivastava @ 2019-05-10 22:37 UTC (permalink / raw)
  To: linux-integrity, inux-security-module, linux-kernel
  Cc: zohar, ebiederm, vgoyal, prsriva, Prakhar Srivastava

From: Prakhar Srivastava <prsriva02@gmail.com>

The buffer(cmdline args) added to the ima log cannot be attested
without having the actual buffer. Thus to make the measured buffer 
available to stroe/read a new ima temaplate (buf) is added. 
The cmdline args used for soft reboot can then be read and attested
later.

The patch adds a new template field buf to store/read the buffer
used while measuring kexec_cmdline args in the 
[PATCH 1/2 v5]: "add a new ima hook and policy to measure the cmdline".
Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
---
 security/integrity/ima/ima_main.c         | 23 +++++++++++++++++++++++
 security/integrity/ima/ima_template.c     |  2 ++
 security/integrity/ima/ima_template_lib.c | 21 +++++++++++++++++++++
 security/integrity/ima/ima_template_lib.h |  4 ++++
 security/integrity/integrity.h            |  1 +
 5 files changed, 51 insertions(+)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 1d186bda25fe..ca12885ca241 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -605,10 +605,32 @@ static int process_buffer_measurement(const void *buf, int size,
 	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
 	int action = 0;
 
+	struct buffer_xattr {
+		enum evm_ima_xattr_type type;
+		u16 buf_length;
+		unsigned char buf[0];
+	};
+	struct buffer_xattr *buffer_event_data = NULL;
+	int alloc_length = 0;
+
 	action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr);
 	if (!(action & IMA_AUDIT) && !(action & IMA_MEASURE))
 		goto out;
 
+	alloc_length = sizeof(struct buffer_xattr) + size;
+	buffer_event_data = kzalloc(alloc_length, GFP_KERNEL);
+	if (!buffer_event_data) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	buffer_event_data->type = IMA_XATTR_BUFFER;
+	buffer_event_data->buf_length = size;
+	memcpy(buffer_event_data->buf, buf, size);
+
+	event_data.xattr_value = (struct evm_ima_xattr_data *)buffer_event_data;
+	event_data.xattr_len = alloc_length;
+
 	memset(iint, 0, sizeof(*iint));
 	memset(&hash, 0, sizeof(hash));
 
@@ -638,6 +660,7 @@ static int process_buffer_measurement(const void *buf, int size,
 	}
 
 out:
+	kfree(buffer_event_data);
 	return ret;
 }
 
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index b631b8bc7624..a76d1c04162a 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -43,6 +43,8 @@ static const struct ima_template_field supported_fields[] = {
 	 .field_show = ima_show_template_string},
 	{.field_id = "sig", .field_init = ima_eventsig_init,
 	 .field_show = ima_show_template_sig},
+	{.field_id = "buf", .field_init = ima_eventbuf_init,
+	 .field_show = ima_show_template_buf},
 };
 #define MAX_TEMPLATE_NAME_LEN 15
 
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 513b457ae900..95a827f42c18 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -162,6 +162,11 @@ void ima_show_template_sig(struct seq_file *m, enum ima_show_type show,
 	ima_show_template_field_data(m, show, DATA_FMT_HEX, field_data);
 }
 
+void ima_show_template_buf(struct seq_file *m, enum ima_show_type show,
+				struct ima_field_data *field_data)
+{
+	ima_show_template_field_data(m, show, DATA_FMT_HEX, field_data);
+}
 /**
  * ima_parse_buf() - Parses lengths and data from an input buffer
  * @bufstartp:       Buffer start address.
@@ -389,3 +394,19 @@ int ima_eventsig_init(struct ima_event_data *event_data,
 	return ima_write_template_field_data(xattr_value, event_data->xattr_len,
 					     DATA_FMT_HEX, field_data);
 }
+
+/*
+ *  ima_eventbuf_init - include the buffer(kexec-cmldine) as part of the
+ *  template data.
+ */
+int ima_eventbuf_init(struct ima_event_data *event_data,
+				struct ima_field_data *field_data)
+{
+	struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
+
+	if ((!xattr_value) || (xattr_value->type != IMA_XATTR_BUFFER))
+		return 0;
+
+	return ima_write_template_field_data(xattr_value, event_data->xattr_len,
+					DATA_FMT_HEX, field_data);
+}
diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h
index 6a3d8b831deb..12f1a8578b31 100644
--- a/security/integrity/ima/ima_template_lib.h
+++ b/security/integrity/ima/ima_template_lib.h
@@ -29,6 +29,8 @@ void ima_show_template_string(struct seq_file *m, enum ima_show_type show,
 			      struct ima_field_data *field_data);
 void ima_show_template_sig(struct seq_file *m, enum ima_show_type show,
 			   struct ima_field_data *field_data);
+void ima_show_template_buf(struct seq_file *m, enum ima_show_type show,
+			   struct ima_field_data *field_data);
 int ima_parse_buf(void *bufstartp, void *bufendp, void **bufcurp,
 		  int maxfields, struct ima_field_data *fields, int *curfields,
 		  unsigned long *len_mask, int enforce_mask, char *bufname);
@@ -42,4 +44,6 @@ int ima_eventname_ng_init(struct ima_event_data *event_data,
 			  struct ima_field_data *field_data);
 int ima_eventsig_init(struct ima_event_data *event_data,
 		      struct ima_field_data *field_data);
+int ima_eventbuf_init(struct ima_event_data *event_data,
+		      struct ima_field_data *field_data);
 #endif /* __LINUX_IMA_TEMPLATE_LIB_H */
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 7de59f44cba3..14ef904f091d 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -74,6 +74,7 @@ enum evm_ima_xattr_type {
 	EVM_IMA_XATTR_DIGSIG,
 	IMA_XATTR_DIGEST_NG,
 	EVM_XATTR_PORTABLE_DIGSIG,
+	IMA_XATTR_BUFFER,
 	IMA_XATTR_LAST
 };
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 3/3 v5] call ima_kexec_cmdline from kexec_file_load path
  2019-05-10 22:37 [PATCH 0/3 v5] Kexec cmdline bufffer measure Prakhar Srivastava
  2019-05-10 22:37 ` [PATCH 1/3 v5] add a new ima hook and policy to measure the cmdline Prakhar Srivastava
  2019-05-10 22:37 ` [PATCH 2/3 v5] add a new template field buf to contain the buffer Prakhar Srivastava
@ 2019-05-10 22:37 ` Prakhar Srivastava
  2019-05-14 14:46   ` Mimi Zohar
  2 siblings, 1 reply; 14+ messages in thread
From: Prakhar Srivastava @ 2019-05-10 22:37 UTC (permalink / raw)
  To: linux-integrity, inux-security-module, linux-kernel
  Cc: zohar, ebiederm, vgoyal, prsriva, Prakhar Srivastava

From: Prakhar Srivastava <prsriva02@gmail.com>

To measure the cmldine args used in case of soft reboot. Call the 
ima hook defined in [PATCH 1/3 v5]:"add a new ima hook and policy to measure the cmdline"

Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
---
 kernel/kexec_file.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index f1d0e00a3971..e779bcf674a0 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -241,6 +241,8 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 			ret = -EINVAL;
 			goto out;
 		}
+
+		ima_kexec_cmdline(image->cmdline_buf, image->cmdline_buf_len - 1);
 	}
 
 	/* Call arch image load handlers */
-- 
2.20.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3 v5] add a new template field buf to contain the buffer
  2019-05-10 22:37 ` [PATCH 2/3 v5] add a new template field buf to contain the buffer Prakhar Srivastava
@ 2019-05-13 13:48   ` Roberto Sassu
  2019-05-14  5:07     ` prakhar srivastava
  0 siblings, 1 reply; 14+ messages in thread
From: Roberto Sassu @ 2019-05-13 13:48 UTC (permalink / raw)
  To: Prakhar Srivastava, linux-integrity, inux-security-module, linux-kernel
  Cc: zohar, ebiederm, vgoyal, prsriva

On 5/11/2019 12:37 AM, Prakhar Srivastava wrote:
> From: Prakhar Srivastava <prsriva02@gmail.com>
> 
> The buffer(cmdline args) added to the ima log cannot be attested
> without having the actual buffer. Thus to make the measured buffer
> available to stroe/read a new ima temaplate (buf) is added.

Hi Prakhar

please fix the typos. More comments below.


> The cmdline args used for soft reboot can then be read and attested
> later.
> 
> The patch adds a new template field buf to store/read the buffer
> used while measuring kexec_cmdline args in the
> [PATCH 1/2 v5]: "add a new ima hook and policy to measure the cmdline".
> Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
> ---
>   security/integrity/ima/ima_main.c         | 23 +++++++++++++++++++++++
>   security/integrity/ima/ima_template.c     |  2 ++
>   security/integrity/ima/ima_template_lib.c | 21 +++++++++++++++++++++
>   security/integrity/ima/ima_template_lib.h |  4 ++++
>   security/integrity/integrity.h            |  1 +
>   5 files changed, 51 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 1d186bda25fe..ca12885ca241 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -605,10 +605,32 @@ static int process_buffer_measurement(const void *buf, int size,
>   	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
>   	int action = 0;
>   
> +	struct buffer_xattr {
> +		enum evm_ima_xattr_type type;
> +		u16 buf_length;
> +		unsigned char buf[0];
> +	};
> +	struct buffer_xattr *buffer_event_data = NULL;
> +	int alloc_length = 0;
> +
>   	action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr);
>   	if (!(action & IMA_AUDIT) && !(action & IMA_MEASURE))
>   		goto out;
>   
> +	alloc_length = sizeof(struct buffer_xattr) + size;
> +	buffer_event_data = kzalloc(alloc_length, GFP_KERNEL);
> +	if (!buffer_event_data) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	buffer_event_data->type = IMA_XATTR_BUFFER;
> +	buffer_event_data->buf_length = size;
> +	memcpy(buffer_event_data->buf, buf, size);
> +
> +	event_data.xattr_value = (struct evm_ima_xattr_data *)buffer_event_data;
> +	event_data.xattr_len = alloc_length;

I would prefer that you introduce two new fields in the ima_event_data
structure. You can initialize them directly with the parameters of
process_buffer_measurement(). ima_write_template_field_data() will make
a copy.


> +
>   	memset(iint, 0, sizeof(*iint));
>   	memset(&hash, 0, sizeof(hash));
>   
> @@ -638,6 +660,7 @@ static int process_buffer_measurement(const void *buf, int size,
>   	}
>   
>   out:
> +	kfree(buffer_event_data);
>   	return ret;
>   }
>   
> diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
> index b631b8bc7624..a76d1c04162a 100644
> --- a/security/integrity/ima/ima_template.c
> +++ b/security/integrity/ima/ima_template.c
> @@ -43,6 +43,8 @@ static const struct ima_template_field supported_fields[] = {
>   	 .field_show = ima_show_template_string},
>   	{.field_id = "sig", .field_init = ima_eventsig_init,
>   	 .field_show = ima_show_template_sig},
> +	{.field_id = "buf", .field_init = ima_eventbuf_init,
> +	 .field_show = ima_show_template_buf},

Please update Documentation/security/IMA-templates.rst

Thanks

Roberto


>   };
>   #define MAX_TEMPLATE_NAME_LEN 15
>   
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index 513b457ae900..95a827f42c18 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -162,6 +162,11 @@ void ima_show_template_sig(struct seq_file *m, enum ima_show_type show,
>   	ima_show_template_field_data(m, show, DATA_FMT_HEX, field_data);
>   }
>   
> +void ima_show_template_buf(struct seq_file *m, enum ima_show_type show,
> +				struct ima_field_data *field_data)
> +{
> +	ima_show_template_field_data(m, show, DATA_FMT_HEX, field_data);
> +}
>   /**
>    * ima_parse_buf() - Parses lengths and data from an input buffer
>    * @bufstartp:       Buffer start address.
> @@ -389,3 +394,19 @@ int ima_eventsig_init(struct ima_event_data *event_data,
>   	return ima_write_template_field_data(xattr_value, event_data->xattr_len,
>   					     DATA_FMT_HEX, field_data);
>   }
> +
> +/*
> + *  ima_eventbuf_init - include the buffer(kexec-cmldine) as part of the
> + *  template data.
> + */
> +int ima_eventbuf_init(struct ima_event_data *event_data,
> +				struct ima_field_data *field_data)
> +{
> +	struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
> +
> +	if ((!xattr_value) || (xattr_value->type != IMA_XATTR_BUFFER))
> +		return 0;
> +
> +	return ima_write_template_field_data(xattr_value, event_data->xattr_len,
> +					DATA_FMT_HEX, field_data);
> +}
> diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h
> index 6a3d8b831deb..12f1a8578b31 100644
> --- a/security/integrity/ima/ima_template_lib.h
> +++ b/security/integrity/ima/ima_template_lib.h
> @@ -29,6 +29,8 @@ void ima_show_template_string(struct seq_file *m, enum ima_show_type show,
>   			      struct ima_field_data *field_data);
>   void ima_show_template_sig(struct seq_file *m, enum ima_show_type show,
>   			   struct ima_field_data *field_data);
> +void ima_show_template_buf(struct seq_file *m, enum ima_show_type show,
> +			   struct ima_field_data *field_data);
>   int ima_parse_buf(void *bufstartp, void *bufendp, void **bufcurp,
>   		  int maxfields, struct ima_field_data *fields, int *curfields,
>   		  unsigned long *len_mask, int enforce_mask, char *bufname);
> @@ -42,4 +44,6 @@ int ima_eventname_ng_init(struct ima_event_data *event_data,
>   			  struct ima_field_data *field_data);
>   int ima_eventsig_init(struct ima_event_data *event_data,
>   		      struct ima_field_data *field_data);
> +int ima_eventbuf_init(struct ima_event_data *event_data,
> +		      struct ima_field_data *field_data);
>   #endif /* __LINUX_IMA_TEMPLATE_LIB_H */
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 7de59f44cba3..14ef904f091d 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -74,6 +74,7 @@ enum evm_ima_xattr_type {
>   	EVM_IMA_XATTR_DIGSIG,
>   	IMA_XATTR_DIGEST_NG,
>   	EVM_XATTR_PORTABLE_DIGSIG,
> +	IMA_XATTR_BUFFER,
>   	IMA_XATTR_LAST
>   };
>   
> 

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3 v5] add a new ima hook and policy to measure the cmdline
  2019-05-10 22:37 ` [PATCH 1/3 v5] add a new ima hook and policy to measure the cmdline Prakhar Srivastava
@ 2019-05-13 16:56   ` Mimi Zohar
  2019-05-14  4:53     ` prakhar srivastava
  0 siblings, 1 reply; 14+ messages in thread
From: Mimi Zohar @ 2019-05-13 16:56 UTC (permalink / raw)
  To: Prakhar Srivastava, linux-integrity, inux-security-module, linux-kernel
  Cc: ebiederm, vgoyal, prsriva

On Fri, 2019-05-10 at 15:37 -0700, Prakhar Srivastava wrote:

> +/*
> + * process_buffer_measurement - Measure the buffer passed to ima log.

"passed to ima log" is unnecessary.

> + * (Instead of using the file hash use the buffer hash).

This comment, if needed, belongs in the text description area below,
not here.

> + * @buf - The buffer that needs to be added to the log
> + * @size - size of buffer(in bytes)
> + * @eventname - event name to be used for buffer.

Missing are the other fields.

> + *
> + * The buffer passed is added to the ima log.
> + *
> + * On success return 0.
> + * On error cases surface errors from ima calls.

Only IMA-appraise returns errors to the caller.  There's no point in
returning an error.


> + */
> +static int process_buffer_measurement(const void *buf, int size,
> +				const char *eventname, const struct cred *cred,
> +				u32 secid)

This should be "static void".

> +{
> +	int ret = 0;
> +	struct ima_template_entry *entry = NULL;
> +	struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
> +	struct ima_event_data event_data = {iint, NULL, NULL,
> +						NULL, 0, NULL};
> +	struct {
> +		struct ima_digest_data hdr;
> +		char digest[IMA_MAX_DIGEST_SIZE];
> +	} hash;
> +	int violation = 0;
> +	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> +	int action = 0;
> +
> +	action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr);
> +	if (!(action & IMA_AUDIT) && !(action & IMA_MEASURE))
> +		goto out;
> +
> +	memset(iint, 0, sizeof(*iint));
> +	memset(&hash, 0, sizeof(hash));
> +
> +	event_data.filename = eventname;
> +
> +	iint->ima_hash = &hash.hdr;
> +	iint->ima_hash->algo = ima_hash_algo;
> +	iint->ima_hash->length = hash_digest_size[ima_hash_algo];
> +
> +	ret = ima_calc_buffer_hash(buf, size, iint->ima_hash);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = ima_alloc_init_template(&event_data, &entry);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (action & IMA_MEASURE)
> +		ret = ima_store_template(entry, violation, NULL, buf, pcr);
> +
> +	if (action & IMA_AUDIT)
> +		ima_audit_measurement(iint, event_data.filename);

The cover letter and patch description say this patch set is limited
to measuring the boot command line - IMA-measurement.
 ima_audit_measurement() adds file hashes in the audit log, which can
be used for security analytics and/or forensics.  This is part of IMA-
audit.  The call to ima_audit_measurement() is inappropriate.

Mimi


> +
> +	if (ret < 0) {
> +		ima_free_template_entry(entry);
> +		goto out;
> +	}
> +
> +out:
> +	return ret;
> +}
> +


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3 v5] add a new ima hook and policy to measure the cmdline
  2019-05-13 16:56   ` Mimi Zohar
@ 2019-05-14  4:53     ` prakhar srivastava
  2019-05-14 14:36       ` Mimi Zohar
  0 siblings, 1 reply; 14+ messages in thread
From: prakhar srivastava @ 2019-05-14  4:53 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: linux-integrity, inux-security-module, linux-kernel, ebiederm,
	vgoyal, Prakhar Srivastava

On Mon, May 13, 2019 at 9:56 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Fri, 2019-05-10 at 15:37 -0700, Prakhar Srivastava wrote:
>
> > +/*
> > + * process_buffer_measurement - Measure the buffer passed to ima log.
>
> "passed to ima log" is unnecessary.
>
> > + * (Instead of using the file hash use the buffer hash).
>
> This comment, if needed, belongs in the text description area below,
> not here.
>
> > + * @buf - The buffer that needs to be added to the log
> > + * @size - size of buffer(in bytes)
> > + * @eventname - event name to be used for buffer.
>
> Missing are the other fields.
>
> > + *
> > + * The buffer passed is added to the ima log.
> > + *
> > + * On success return 0.
> > + * On error cases surface errors from ima calls.
>
> Only IMA-appraise returns errors to the caller.  There's no point in
> returning an error.
>
>
> > + */
> > +static int process_buffer_measurement(const void *buf, int size,
> > +                             const char *eventname, const struct cred *cred,
> > +                             u32 secid)
>
> This should be "static void".
>
> > +{
> > +
> > +     if (action & IMA_MEASURE)
> > +             ret = ima_store_template(entry, violation, NULL, buf, pcr);
> > +
> > +     if (action & IMA_AUDIT)
> > +             ima_audit_measurement(iint, event_data.filename);
>
> The cover letter and patch description say this patch set is limited
> to measuring the boot command line - IMA-measurement.
>  ima_audit_measurement() adds file hashes in the audit log, which can
> be used for security analytics and/or forensics.  This is part of IMA-
> audit.  The call to ima_audit_measurement() is inappropriate.
>
To clarify, in one of the previous versions you mentioned it
might be helpful to add audit.
I might have misunderstood you, but i will remove the
audit_measurement and make other corrections.
Thankyou for your feedback.

- Thanks,
Prakhar Srivastava
 Mimi

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3 v5] add a new template field buf to contain the buffer
  2019-05-13 13:48   ` Roberto Sassu
@ 2019-05-14  5:07     ` prakhar srivastava
  2019-05-14 13:22       ` Roberto Sassu
  0 siblings, 1 reply; 14+ messages in thread
From: prakhar srivastava @ 2019-05-14  5:07 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: linux-integrity, inux-security-module, linux-kernel, Mimi Zohar,
	ebiederm, vgoyal, Prakhar Srivastava

On Mon, May 13, 2019 at 6:48 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> On 5/11/2019 12:37 AM, Prakhar Srivastava wrote:
> > From: Prakhar Srivastava <prsriva02@gmail.com>
> >
> > The buffer(cmdline args) added to the ima log cannot be attested
> > without having the actual buffer. Thus to make the measured buffer
> > available to store/read a new ima template (buf) is added.
>
> Hi Prakhar
>
> please fix the typos. More comments below.
>
>
> > +     buffer_event_data->type = IMA_XATTR_BUFFER;
> > +     buffer_event_data->buf_length = size;
> > +     memcpy(buffer_event_data->buf, buf, size);
> > +
> > +     event_data.xattr_value = (struct evm_ima_xattr_data *)buffer_event_data;
> > +     event_data.xattr_len = alloc_length;
>
> I would prefer that you introduce two new fields in the ima_event_data
> structure. You can initialize them directly with the parameters of
> process_buffer_measurement().
I will make the edits, this will definitely save the kzalloc in this code
path.
>
> ima_write_template_field_data() will make
> a copy.
>
Since event_data->type is used to distinguish what the template field
 should contain.
Removing the type and subsequent check in the template_init,
 buf template fmt will result in the whole event_Data structure
being added to the log, which is not the expected output.
For buffer entries, the buf templet fmt will contains the buffer itself.

>
> > +      .field_show = ima_show_template_buf},
>
> Please update Documentation/security/IMA-templates.rst
Will update the documentation.

Thanks,
Prakhar Srivastava
>
> Thanks
>
> Roberto

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3 v5] add a new template field buf to contain the buffer
  2019-05-14  5:07     ` prakhar srivastava
@ 2019-05-14 13:22       ` Roberto Sassu
  2019-05-17 23:32         ` prakhar srivastava
  0 siblings, 1 reply; 14+ messages in thread
From: Roberto Sassu @ 2019-05-14 13:22 UTC (permalink / raw)
  To: prakhar srivastava
  Cc: linux-integrity, inux-security-module, linux-kernel, Mimi Zohar,
	ebiederm, vgoyal, Prakhar Srivastava

On 5/14/2019 7:07 AM, prakhar srivastava wrote:
> On Mon, May 13, 2019 at 6:48 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>>
>> On 5/11/2019 12:37 AM, Prakhar Srivastava wrote:
>>> From: Prakhar Srivastava <prsriva02@gmail.com>
>>>
>>> The buffer(cmdline args) added to the ima log cannot be attested
>>> without having the actual buffer. Thus to make the measured buffer
>>> available to store/read a new ima template (buf) is added.
>>
>> Hi Prakhar
>>
>> please fix the typos. More comments below.
>>
>>
>>> +     buffer_event_data->type = IMA_XATTR_BUFFER;
>>> +     buffer_event_data->buf_length = size;
>>> +     memcpy(buffer_event_data->buf, buf, size);
>>> +
>>> +     event_data.xattr_value = (struct evm_ima_xattr_data *)buffer_event_data;
>>> +     event_data.xattr_len = alloc_length;
>>
>> I would prefer that you introduce two new fields in the ima_event_data
>> structure. You can initialize them directly with the parameters of
>> process_buffer_measurement().
> I will make the edits, this will definitely save the kzalloc in this code
> path.
>>
>> ima_write_template_field_data() will make
>> a copy.
>>
> Since event_data->type is used to distinguish what the template field
>   should contain.
> Removing the type and subsequent check in the template_init,
>   buf template fmt will result in the whole event_Data structure
> being added to the log, which is not the expected output.
> For buffer entries, the buf templet fmt will contains the buffer itself.

The purpose of ima_event_data is to pass data to the init method of
template fields. Each method takes the data it needs.

If you pass event_data->buf and event_data->buf_len to
ima_write_template_field_data() this should be fine.

Roberto


>>> +      .field_show = ima_show_template_buf},
>>
>> Please update Documentation/security/IMA-templates.rst
> Will update the documentation.
> 
> Thanks,
> Prakhar Srivastava
>>
>> Thanks
>>
>> Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3 v5] add a new ima hook and policy to measure the cmdline
  2019-05-14  4:53     ` prakhar srivastava
@ 2019-05-14 14:36       ` Mimi Zohar
  0 siblings, 0 replies; 14+ messages in thread
From: Mimi Zohar @ 2019-05-14 14:36 UTC (permalink / raw)
  To: prakhar srivastava
  Cc: linux-integrity, inux-security-module, linux-kernel, ebiederm,
	vgoyal, Prakhar Srivastava


> > > +{
> > > +
> > > +     if (action & IMA_MEASURE)
> > > +             ret = ima_store_template(entry, violation, NULL, buf, pcr);
> > > +
> > > +     if (action & IMA_AUDIT)
> > > +             ima_audit_measurement(iint, event_data.filename);
> >
> > The cover letter and patch description say this patch set is limited
> > to measuring the boot command line - IMA-measurement.
> >  ima_audit_measurement() adds file hashes in the audit log, which can
> > be used for security analytics and/or forensics.  This is part of IMA-
> > audit.  The call to ima_audit_measurement() is inappropriate.
> >
> To clarify, in one of the previous versions you mentioned it
> might be helpful to add audit.

The original question was whether the kexec command line should ONLY
be measured.  That decision impacts whether a new function
(process_buffer_measurement) is needed or whether you should still
call process_measurement().

> I might have misunderstood you, but i will remove the
> audit_measurement and make other corrections.
> Thankyou for your feedback.

Even if it was agreed that you were adding the ability to measure and
audit the kexec boot command line, the cover letter, the patch
descriptions and the patches themselves would need to reflect that.
 The call to ima_audit_measurement() would not be hidden like this,
but included as a separate patch.

Mimi





^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3 v5] call ima_kexec_cmdline from kexec_file_load path
  2019-05-10 22:37 ` [PATCH 3/3 v5] call ima_kexec_cmdline from kexec_file_load path Prakhar Srivastava
@ 2019-05-14 14:46   ` Mimi Zohar
  0 siblings, 0 replies; 14+ messages in thread
From: Mimi Zohar @ 2019-05-14 14:46 UTC (permalink / raw)
  To: Prakhar Srivastava, linux-integrity, inux-security-module, linux-kernel
  Cc: ebiederm, vgoyal, prsriva, Dave Young

[Cc'ing Dave Young]

On Fri, 2019-05-10 at 15:37 -0700, Prakhar Srivastava wrote:
> From: Prakhar Srivastava <prsriva02@gmail.com>

The "From" line above should only appear when the patch author and the
sender differ.  You can create the patches under one id and post them
from another id.  Something is still wrong.

> 
> To measure the cmldine args used in case of soft reboot. Call the 
> ima hook defined in [PATCH 1/3 v5]:"add a new ima hook and policy to measure the cmdline"
> 
> Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>

> ---
>  kernel/kexec_file.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index f1d0e00a3971..e779bcf674a0 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -241,6 +241,8 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  			ret = -EINVAL;
>  			goto out;
>  		}
> +
> +		ima_kexec_cmdline(image->cmdline_buf, image->cmdline_buf_len - 1);
>  	}
>  
>  	/* Call arch image load handlers */

Much better!

Mimi


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3 v5] add a new template field buf to contain the buffer
  2019-05-14 13:22       ` Roberto Sassu
@ 2019-05-17 23:32         ` prakhar srivastava
  2019-05-20 12:18           ` Roberto Sassu
  0 siblings, 1 reply; 14+ messages in thread
From: prakhar srivastava @ 2019-05-17 23:32 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: linux-integrity, linux-kernel, Mimi Zohar, ebiederm, vgoyal,
	Prakhar Srivastava

On Tue, May 14, 2019 at 6:22 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> On 5/14/2019 7:07 AM, prakhar srivastava wrote:
> > On Mon, May 13, 2019 at 6:48 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
> >>
> >> On 5/11/2019 12:37 AM, Prakhar Srivastava wrote:
> >>> From: Prakhar Srivastava <prsriva02@gmail.com>
> >>>
> >>> The buffer(cmdline args) added to the ima log cannot be attested
> >>> without having the actual buffer. Thus to make the measured buffer
> >>> available to store/read a new ima template (buf) is added.
> >>
> >> Hi Prakhar
> >>
> >> please fix the typos. More comments below.
> >>
> >>
> >>> +     buffer_event_data->type = IMA_XATTR_BUFFER;
> >>> +     buffer_event_data->buf_length = size;
> >>> +     memcpy(buffer_event_data->buf, buf, size);
> >>> +
> >>> +     event_data.xattr_value = (struct evm_ima_xattr_data *)buffer_event_data;
> >>> +     event_data.xattr_len = alloc_length;
> >>
> >> I would prefer that you introduce two new fields in the ima_event_data
> >> structure. You can initialize them directly with the parameters of
> >> process_buffer_measurement().
> > I will make the edits, this will definitely save the kzalloc in this code
> > path.
> >>
> >> ima_write_template_field_data() will make
> >> a copy.
> >>
> > Since event_data->type is used to distinguish what the template field
> >   should contain.
> > Removing the type and subsequent check in the template_init,
> >   buf template fmt will result in the whole event_Data structure
> > being added to the log, which is not the expected output.
> > For buffer entries, the buf template fmt will contains the buffer itself.

>
> The purpose of ima_event_data is to pass data to the init method of
> template fields. Each method takes the data it needs.
>
> If you pass event_data->buf and event_data->buf_len to
> ima_write_template_field_data() this should be fine.

Hi Roberto,
I did some testing after making the needed code changes,
the output is as expected the buf template field only contains
the buf when the ima_event_data.buf is set.

However i just want to double check if adding two new fields to
the struct ima_event_data is approach you want me to take?
Mimi any concerns?

what all tests do i need to run to confirm i am not
in-inadvertently breaking some thing else?

Thanks,
Prakhar Srivastava
>
> Roberto
>
>
> >>> +      .field_show = ima_show_template_buf},
> >>
> >> Please update Documentation/security/IMA-templates.rst
> > Will update the documentation.
> >
> > Thanks,
> > Prakhar Srivastava
> >>
> >> Thanks
> >>
> >> Roberto
>
> --
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Bo PENG, Jian LI, Yanli SHI

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3 v5] add a new template field buf to contain the buffer
  2019-05-17 23:32         ` prakhar srivastava
@ 2019-05-20 12:18           ` Roberto Sassu
  0 siblings, 0 replies; 14+ messages in thread
From: Roberto Sassu @ 2019-05-20 12:18 UTC (permalink / raw)
  To: prakhar srivastava
  Cc: linux-integrity, linux-kernel, Mimi Zohar, ebiederm, vgoyal,
	Prakhar Srivastava

On 5/18/2019 1:32 AM, prakhar srivastava wrote:
> On Tue, May 14, 2019 at 6:22 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>>
>> On 5/14/2019 7:07 AM, prakhar srivastava wrote:
>>> On Mon, May 13, 2019 at 6:48 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>>>>
>>>> On 5/11/2019 12:37 AM, Prakhar Srivastava wrote:
>>>>> From: Prakhar Srivastava <prsriva02@gmail.com>
>>>>>
>>>>> The buffer(cmdline args) added to the ima log cannot be attested
>>>>> without having the actual buffer. Thus to make the measured buffer
>>>>> available to store/read a new ima template (buf) is added.
>>>>
>>>> Hi Prakhar
>>>>
>>>> please fix the typos. More comments below.
>>>>
>>>>
>>>>> +     buffer_event_data->type = IMA_XATTR_BUFFER;
>>>>> +     buffer_event_data->buf_length = size;
>>>>> +     memcpy(buffer_event_data->buf, buf, size);
>>>>> +
>>>>> +     event_data.xattr_value = (struct evm_ima_xattr_data *)buffer_event_data;
>>>>> +     event_data.xattr_len = alloc_length;
>>>>
>>>> I would prefer that you introduce two new fields in the ima_event_data
>>>> structure. You can initialize them directly with the parameters of
>>>> process_buffer_measurement().
>>> I will make the edits, this will definitely save the kzalloc in this code
>>> path.
>>>>
>>>> ima_write_template_field_data() will make
>>>> a copy.
>>>>
>>> Since event_data->type is used to distinguish what the template field
>>>    should contain.
>>> Removing the type and subsequent check in the template_init,
>>>    buf template fmt will result in the whole event_Data structure
>>> being added to the log, which is not the expected output.
>>> For buffer entries, the buf template fmt will contains the buffer itself.
> 
>>
>> The purpose of ima_event_data is to pass data to the init method of
>> template fields. Each method takes the data it needs.
>>
>> If you pass event_data->buf and event_data->buf_len to
>> ima_write_template_field_data() this should be fine.
> 
> Hi Roberto,
> I did some testing after making the needed code changes,
> the output is as expected the buf template field only contains
> the buf when the ima_event_data.buf is set.
> 
> However i just want to double check if adding two new fields to
> the struct ima_event_data is approach you want me to take?
> Mimi any concerns?

I think it should not be a problem. ima_event_data was introduced to
pass more information to a function for a new template field, without
changing the definition of existing functions.


> what all tests do i need to run to confirm i am not
> in-inadvertently breaking some thing else?

ima_event_data is not used for marshaling/unmarshaling. Adding two new
members to the structure won't change the behavior of existing code.

Roberto


> Thanks,
> Prakhar Srivastava
>>
>> Roberto
>>
>>
>>>>> +      .field_show = ima_show_template_buf},
>>>>
>>>> Please update Documentation/security/IMA-templates.rst
>>> Will update the documentation.
>>>
>>> Thanks,
>>> Prakhar Srivastava
>>>>
>>>> Thanks
>>>>
>>>> Roberto
>>
>> --
>> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
>> Managing Director: Bo PENG, Jian LI, Yanli SHI

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/3 v5] add a new ima hook and policy to measure the cmdline
  2019-05-10 22:32 [PATCH 0/3 v5] Kexec cmdline bufffer measure Prakhar Srivastava
@ 2019-05-10 22:32 ` Prakhar Srivastava
  0 siblings, 0 replies; 14+ messages in thread
From: Prakhar Srivastava @ 2019-05-10 22:32 UTC (permalink / raw)
  To: linux-integrity, inux-security-module, linux-kernel
  Cc: zohar, ebiederm, vgoyal, prsriva, Prakhar Srivastava

From: Prakhar Srivastava <prsriva02@gmail.com>

For this reason, this patch adds support for measuring these
parameters during kexec. To achive this, a new ima policy and
hook id, defined KEXEC_CMDLINE and ima_kexec_cmdline respectively,
are added.

Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
---
 Documentation/ABI/testing/ima_policy |  1 +
 include/linux/ima.h                  |  2 +
 security/integrity/ima/ima.h         |  1 +
 security/integrity/ima/ima_api.c     |  1 +
 security/integrity/ima/ima_main.c    | 84 ++++++++++++++++++++++++++++
 security/integrity/ima/ima_policy.c  |  9 +++
 6 files changed, 98 insertions(+)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 74c6702de74e..62e7cd687e9c 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -29,6 +29,7 @@ Description:
 		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
+				[KEXEC_CMDLINE]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
 			       [[^]MAY_EXEC]
 			fsmagic:= hex value
diff --git a/include/linux/ima.h b/include/linux/ima.h
index dc12fbcf484c..2e2c77280be8 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -26,6 +26,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_kexec_cmdline(const void *buf, int size);
 
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);
@@ -92,6 +93,7 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
 	return;
 }
 
+static inline void ima_kexec_cmdline(const void *buf, int size) {}
 #endif /* CONFIG_IMA */
 
 #ifndef CONFIG_IMA_KEXEC
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d213e835c498..226a26d8de09 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -184,6 +184,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
 	hook(KEXEC_KERNEL_CHECK)	\
 	hook(KEXEC_INITRAMFS_CHECK)	\
 	hook(POLICY_CHECK)		\
+	hook(KEXEC_CMDLINE)		\
 	hook(MAX_CHECK)
 #define __ima_hook_enumify(ENUM)	ENUM,
 
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c7505fb122d4..800d965232e5 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -169,6 +169,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  *		subj=, obj=, type=, func=, mask=, fsmagic=
  *	subj,obj, and type: are LSM specific.
  *	func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
+ *	| KEXEC_CMDLINE
  *	mask: contains the permission mask
  *	fsmagic: hex value
  *
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 357edd140c09..1d186bda25fe 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -576,6 +576,90 @@ int ima_load_data(enum kernel_load_data_id id)
 	return 0;
 }
 
+/*
+ * process_buffer_measurement - Measure the buffer passed to ima log.
+ * (Instead of using the file hash use the buffer hash).
+ * @buf - The buffer that needs to be added to the log
+ * @size - size of buffer(in bytes)
+ * @eventname - event name to be used for buffer.
+ *
+ * The buffer passed is added to the ima log.
+ *
+ * On success return 0.
+ * On error cases surface errors from ima calls.
+ */
+static int process_buffer_measurement(const void *buf, int size,
+				const char *eventname, const struct cred *cred,
+				u32 secid)
+{
+	int ret = 0;
+	struct ima_template_entry *entry = NULL;
+	struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
+	struct ima_event_data event_data = {iint, NULL, NULL,
+						NULL, 0, NULL};
+	struct {
+		struct ima_digest_data hdr;
+		char digest[IMA_MAX_DIGEST_SIZE];
+	} hash;
+	int violation = 0;
+	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
+	int action = 0;
+
+	action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr);
+	if (!(action & IMA_AUDIT) && !(action & IMA_MEASURE))
+		goto out;
+
+	memset(iint, 0, sizeof(*iint));
+	memset(&hash, 0, sizeof(hash));
+
+	event_data.filename = eventname;
+
+	iint->ima_hash = &hash.hdr;
+	iint->ima_hash->algo = ima_hash_algo;
+	iint->ima_hash->length = hash_digest_size[ima_hash_algo];
+
+	ret = ima_calc_buffer_hash(buf, size, iint->ima_hash);
+	if (ret < 0)
+		goto out;
+
+	ret = ima_alloc_init_template(&event_data, &entry);
+	if (ret < 0)
+		goto out;
+
+	if (action & IMA_MEASURE)
+		ret = ima_store_template(entry, violation, NULL, buf, pcr);
+
+	if (action & IMA_AUDIT)
+		ima_audit_measurement(iint, event_data.filename);
+
+	if (ret < 0) {
+		ima_free_template_entry(entry);
+		goto out;
+	}
+
+out:
+	return ret;
+}
+
+/**
+ * ima_kexec_cmdline - based on policy, store kexec cmdline args
+ * @buf: pointer to buffer
+ * @size: size of buffer
+ *
+ * Buffers can only be measured, not appraised.
+ */
+void ima_kexec_cmdline(const void *buf, int size)
+{
+	u32 secid;
+
+	if (buf && size != 0) {
+		security_task_getsecid(current, &secid);
+		process_buffer_measurement(buf, size, "kexec-cmdline",
+				current_cred(), secid);
+	}
+}
+
+
 static int __init init_ima(void)
 {
 	int error;
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e0cc323f948f..413e5921b248 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -291,6 +291,13 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 {
 	int i;
 
+	/* only incase of KEXEC_CMDLINE, inode is NULL */
+	if (func == KEXEC_CMDLINE) {
+		if ((rule->flags & IMA_FUNC) &&
+			(rule->func == func) && (!inode))
+			return true;
+		return false;
+	}
 	if ((rule->flags & IMA_FUNC) &&
 	    (rule->func != func && func != POST_SETATTR))
 		return false;
@@ -869,6 +876,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				entry->func = KEXEC_INITRAMFS_CHECK;
 			else if (strcmp(args[0].from, "POLICY_CHECK") == 0)
 				entry->func = POLICY_CHECK;
+			else if (strcmp(args[0].from, "KEXEC_CMDLINE") == 0)
+				entry->func = KEXEC_CMDLINE;
 			else
 				result = -EINVAL;
 			if (!result)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-10 22:37 [PATCH 0/3 v5] Kexec cmdline bufffer measure Prakhar Srivastava
2019-05-10 22:37 ` [PATCH 1/3 v5] add a new ima hook and policy to measure the cmdline Prakhar Srivastava
2019-05-13 16:56   ` Mimi Zohar
2019-05-14  4:53     ` prakhar srivastava
2019-05-14 14:36       ` Mimi Zohar
2019-05-10 22:37 ` [PATCH 2/3 v5] add a new template field buf to contain the buffer Prakhar Srivastava
2019-05-13 13:48   ` Roberto Sassu
2019-05-14  5:07     ` prakhar srivastava
2019-05-14 13:22       ` Roberto Sassu
2019-05-17 23:32         ` prakhar srivastava
2019-05-20 12:18           ` Roberto Sassu
2019-05-10 22:37 ` [PATCH 3/3 v5] call ima_kexec_cmdline from kexec_file_load path Prakhar Srivastava
2019-05-14 14:46   ` Mimi Zohar
  -- strict thread matches above, loose matches on Subject: below --
2019-05-10 22:32 [PATCH 0/3 v5] Kexec cmdline bufffer measure Prakhar Srivastava
2019-05-10 22:32 ` [PATCH 1/3 v5] add a new ima hook and policy to measure the cmdline Prakhar Srivastava

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org linux-integrity@archiver.kernel.org
	public-inbox-index linux-integrity


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/ public-inbox