2019-06-07 00:53:48

by Prakhar Srivastava

[permalink] [raw]
Subject: [PATCH v7 0/3] add new ima hook ima_kexec_cmdline to measure kexec boot cmdline args

The motive behind the patch series is to measure the boot 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_file_load.

Currently for Kexec(kexec_file_load)/soft reboot scenario the boot cmdline
args for the next kernel are 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, no cmdline arguments measurement takes place.

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 be carried over to the next kernel, which will be followed
up by other patchsets for x86_64 and arm64.

The kexec cmdline hash can be verified using
sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements |
grep kexec-cmdline | cut -d' ' -f 6 | xxd -r -p | sha256sum

Changelog:
V7:
- rebased to next-queued-testing
https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git/log/?h=next-queued-testing

V6:
-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]
add new fields to ima_event_data to store/read buffer data.
[suggested by Roberto]
-call ima_kexec_cmdline from kexec_file_load path

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 ima_kexec_cmdline to measure cmdline args
add a new ima template field buf
call ima_kexec_cmdline to measure the cmdline args

Documentation/ABI/testing/ima_policy | 1 +
Documentation/security/IMA-templates.rst | 2 +-
include/linux/ima.h | 2 +
kernel/kexec_file.c | 8 ++-
security/integrity/ima/ima.h | 3 +
security/integrity/ima/ima_api.c | 5 +-
security/integrity/ima/ima_init.c | 2 +-
security/integrity/ima/ima_main.c | 80 +++++++++++++++++++++++
security/integrity/ima/ima_policy.c | 9 +++
security/integrity/ima/ima_template.c | 2 +
security/integrity/ima/ima_template_lib.c | 20 ++++++
security/integrity/ima/ima_template_lib.h | 4 ++
12 files changed, 131 insertions(+), 7 deletions(-)

--
2.17.1


2019-06-07 00:54:02

by Prakhar Srivastava

[permalink] [raw]
Subject: [PATCH v7 1/3] Add a new ima hook ima_kexec_cmdline to measure cmdline args

This patch adds support in ima to measure kexec cmdline args
during soft reboot kexec_file_load.

- A new ima hook ima_kexec_cmdline is defined to be called by the
kexec code.
- A new function process_buffer_measurement is defined to measure
the buffer hash into the ima log.
- A new func policy KEXEC_CMDLINE is defined to control the
measurement.[Suggested by Mimi]

Hash computation can be tested using
sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements |
grep kexec-cmdline | cut -d' ' -f 6 | xxd -r -p | sha256sum

Signed-off-by: Prakhar Srivastava <[email protected]>
---
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 | 77 ++++++++++++++++++++++++++++
security/integrity/ima/ima_policy.c | 9 ++++
6 files changed, 91 insertions(+)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index b383c1763610..fc376a323908 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -28,6 +28,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 fd9f7cf4cdf5..b42f5a006042 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 18b48a6d0b80..a4ad1270bffa 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -185,6 +185,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 78eb11c7ac07..ea7d8cbf712f 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -176,6 +176,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 af341a80118f..e4f301381ffb 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -605,6 +605,83 @@ int ima_load_data(enum kernel_load_data_id id)
return 0;
}

+/*
+ * process_buffer_measurement - Measure the buffer to ima log.
+ * @buf: pointer to the buffer that needs to be added to the log.
+ * @size: size of buffer(in bytes).
+ * @eventname: event name to be used for the buffer entry.
+ * @cred: a pointer to a credentials structure for user validation.
+ * @secid: the secid of the task to be validated.
+ *
+ * Based on policy, the buffer is measured into the ima log.
+ */
+static void 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 = iint};
+ struct ima_template_desc *template_desc = 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,
+ &template_desc);
+ if (!(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, template_desc);
+ if (ret < 0)
+ goto out;
+
+ if (action & IMA_MEASURE)
+ ret = ima_store_template(entry, violation, NULL, buf, pcr);
+
+ if (ret < 0)
+ ima_free_template_entry(entry);
+
+out:
+ return;
+}
+
+/**
+ * ima_kexec_cmdline - measure kexec cmdline boot 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 fd9b01881d17..98e351e13557 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -292,6 +292,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;
@@ -880,6 +887,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.19.1

2019-06-07 00:54:07

by Prakhar Srivastava

[permalink] [raw]
Subject: [PATCH v7 3/3] call ima_kexec_cmdline to measure the cmdline args

During soft reboot(kexec_file_load) boot cmdline args
are not measured.Thus the new kernel on load boots with
an assumption of cold reboot.

This patch makes a call to the ima hook ima_kexec_cmdline,
added in "Add a new ima hook ima_kexec_cmdline to measure
cmdline args"
to measure the boot cmdline args into the ima log.

- call ima_kexec_cmdline from kexec_file_load.
- move the call ima_add_kexec_buffer after the cmdline
args have been measured.

Signed-off-by: Prakhar Srivastava <[email protected]>
---
kernel/kexec_file.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 072b6ee55e3f..ed4727586fc3 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -198,9 +198,6 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
return ret;
image->kernel_buf_len = size;

- /* IMA needs to pass the measurement list to the next kernel. */
- ima_add_kexec_buffer(image);
-
/* Call arch image probe handlers */
ret = arch_kexec_kernel_image_probe(image, image->kernel_buf,
image->kernel_buf_len);
@@ -241,8 +238,13 @@ 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);
}

+ /* IMA needs to pass the measurement list to the next kernel. */
+ ima_add_kexec_buffer(image);
+
/* Call arch image load handlers */
ldata = arch_kexec_kernel_image_load(image);

--
2.19.1

2019-06-07 00:54:32

by Prakhar Srivastava

[permalink] [raw]
Subject: [PATCH v7 2/3] add a new ima template field buf

A buffer(kexec cmdline args) measured into ima cannot be
appraised without already being aware of the buffer contents.
Since hashes are non-reversible, raw buffer is needed for
validation or regenerating hash for appraisal/attestation.

This patch adds support to ima to allow store/read the
buffer contents in HEX.

- Add two new fields to ima_event_data to hold the buf and
buf_len [Suggested by Roberto]
- Add a new temaplte field 'buf' to be used to store/read
the buffer data.[Suggested by Mimi]
- Updated process_buffer_meaurement to add the buffer to
ima_event_data. process_buffer_measurement added in
"Add a new ima hook ima_kexec_cmdline to measure cmdline args"

Signed-off-by: Prakhar Srivastava <[email protected]>
Reviewed-by: Roberto Sassu <[email protected]>
---
Documentation/security/IMA-templates.rst | 4 ++--
security/integrity/ima/ima.h | 2 ++
security/integrity/ima/ima_api.c | 4 ++--
security/integrity/ima/ima_init.c | 2 +-
security/integrity/ima/ima_main.c | 2 ++
security/integrity/ima/ima_template.c | 2 ++
security/integrity/ima/ima_template_lib.c | 20 ++++++++++++++++++++
security/integrity/ima/ima_template_lib.h | 4 ++++
8 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/Documentation/security/IMA-templates.rst b/Documentation/security/IMA-templates.rst
index 2cd0e273cc9a..3e78ce3591db 100644
--- a/Documentation/security/IMA-templates.rst
+++ b/Documentation/security/IMA-templates.rst
@@ -69,8 +69,8 @@ descriptors by adding their identifier to the format string
algorithm (field format: [<hash algo>:]digest, where the digest
prefix is shown only if the hash algorithm is not SHA1 or MD5);
- 'n-ng': the name of the event, without size limitations;
- - 'sig': the file signature.
-
+ - 'sig': the file signature;
+ - 'buf': the buffer data that was used to generate the hash without size limitations;

Below, there is the list of defined template descriptors:

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index a4ad1270bffa..16110180545c 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -65,6 +65,8 @@ struct ima_event_data {
struct evm_ima_xattr_data *xattr_value;
int xattr_len;
const char *violation;
+ const void *buf;
+ int buf_len;
};

/* IMA template field data definition */
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index ea7d8cbf712f..83ca99d65e4b 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -140,7 +140,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
struct ima_template_entry *entry;
struct inode *inode = file_inode(file);
struct ima_event_data event_data = {iint, file, filename, NULL, 0,
- cause};
+ cause, NULL, 0};
int violation = 1;
int result;

@@ -296,7 +296,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
struct inode *inode = file_inode(file);
struct ima_template_entry *entry;
struct ima_event_data event_data = {iint, file, filename, xattr_value,
- xattr_len, NULL};
+ xattr_len, NULL, NULL, 0};
int violation = 0;

if (iint->measured_pcrs & (0x1 << pcr))
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 993d0f1915ff..c8591406c0e2 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -50,7 +50,7 @@ static int __init ima_add_boot_aggregate(void)
struct ima_template_entry *entry;
struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
struct ima_event_data event_data = {iint, NULL, boot_aggregate_name,
- NULL, 0, NULL};
+ NULL, 0, NULL, NULL, 0};
int result = -ENOMEM;
int violation = 0;
struct {
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index e4f301381ffb..9308d664f074 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -641,6 +641,8 @@ static void process_buffer_measurement(const void *buf, int size,
memset(&hash, 0, sizeof(hash));

event_data.filename = eventname;
+ event_data.buf = buf;
+ event_data.buf_len = size;

iint->ima_hash = &hash.hdr;
iint->ima_hash->algo = ima_hash_algo;
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index e6e892f31cbd..8c40de18a0aa 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..43d1404141c1 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -162,6 +162,12 @@ 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 +395,17 @@ 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)
+{
+ if ((!event_data->buf) || (event_data->buf_len == 0))
+ return 0;
+
+ return ima_write_template_field_data(event_data->buf, event_data->buf_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..f0178bc60c55 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 */
--
2.19.1

2019-06-11 16:41:23

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH v7 0/3] add new ima hook ima_kexec_cmdline to measure kexec boot cmdline args

Hi Prakhar,

The patch/patch set title in the Subject line should not explain "how"
you add a new feature.  In this case an appropriate patch set title
would be, "Add support for measuring the boot command line".
 Similarly, the first patch in this patch set could be named "Define a
new IMA hook to measure the boot command line arguments".

On Thu, 2019-06-06 at 17:23 -0700, Prakhar Srivastava wrote:
> The motive behind the patch series is to measure the boot cmdline args
> used for soft reboot/kexec case.

When mentoring, I suggest starting out with a simple status statement
(eg. "The kexec boot command line arguments are not currently being
measured."), followed by the problem statement in the first paragraph.

>
> For secure boot attestation, it is necessary to measure the kernel

Secure boot enforces local file data integrity.  The term here should
be "trusted boot attestation".

> command line and the kernel version.

The original version of this patch set included the kernel version.  
This version is just measuring the boot command line arguments.

> 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_file_load.
>
> Currently for Kexec(kexec_file_load)/soft reboot scenario the boot cmdline
> args for the next kernel are 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, no cmdline arguments measurement takes place.
>
> 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 be carried over to the next kernel, which will be followed
> up by other patchsets for x86_64 and arm64.
>
> The kexec cmdline hash

^stored in the "d-ng" field of the template data

> can be verified using

> sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements |
> grep kexec-cmdline | cut -d' ' -f 6 | xxd -r -p | sha256sum

Until per policy template field rule support is added, a template name
needs to be defined.  Please define "ima-buf" as:
{.name = "ima-buf", .fmt = "d-ng|n-ng|buf"}

I'm still seeing some scripts/checkpatch "WARNING: line over 80
characters".  scripts/Lindent should provide the correct way of
formatting these lines.

Some people feel that references to Lindent should be removed, but I
tend to agree with the Documentation/hwmon/submitting-patches.rst
comment pertaining to scripts/Lindent.

"* Running your patch or driver file(s) through checkpatch does not
mean its formatting is clean. If unsure about formatting in your new
driver, run it through Lindent. Lindent is not perfect, and you may
have to do some minor cleanup, but it is a good start."

Examples of where the line formatting is off is the call to
ima_get_action() in process_buffer_measurement() and the call to
process_buffer_measurement() in ima_kexec_cmdline().

thanks,

Mimi

>
> Changelog:
> V7:
> - rebased to next-queued-testing
> https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git/log/?h=next-queued-testing
>
> V6:
> -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]
> add new fields to ima_event_data to store/read buffer data.
> [suggested by Roberto]
> -call ima_kexec_cmdline from kexec_file_load path
>
> 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 ima_kexec_cmdline to measure cmdline args
> add a new ima template field buf
> call ima_kexec_cmdline to measure the cmdline args
>
> Documentation/ABI/testing/ima_policy | 1 +
> Documentation/security/IMA-templates.rst | 2 +-
> include/linux/ima.h | 2 +
> kernel/kexec_file.c | 8 ++-
> security/integrity/ima/ima.h | 3 +
> security/integrity/ima/ima_api.c | 5 +-
> security/integrity/ima/ima_init.c | 2 +-
> security/integrity/ima/ima_main.c | 80 +++++++++++++++++++++++
> security/integrity/ima/ima_policy.c | 9 +++
> security/integrity/ima/ima_template.c | 2 +
> security/integrity/ima/ima_template_lib.c | 20 ++++++
> security/integrity/ima/ima_template_lib.h | 4 ++
> 12 files changed, 131 insertions(+), 7 deletions(-)
>

2019-06-11 19:14:12

by Prakhar Srivastava

[permalink] [raw]
Subject: Re: [PATCH v7 0/3] add new ima hook ima_kexec_cmdline to measure kexec boot cmdline args

On Tue, Jun 11, 2019 at 8:37 AM Mimi Zohar <[email protected]> wrote:
>
> Hi Prakhar,
>
> The patch/patch set title in the Subject line should not explain "how"
> you add a new feature. In this case an appropriate patch set title
> would be, "Add support for measuring the boot command line".
> Similarly, the first patch in this patch set could be named "Define a
> new IMA hook to measure the boot command line arguments".
>
> On Thu, 2019-06-06 at 17:23 -0700, Prakhar Srivastava wrote:
> > The motive behind the patch series is to measure the boot cmdline args
> > used for soft reboot/kexec case.
>
> When mentoring, I suggest starting out with a simple status statement
> (eg. "The kexec boot command line arguments are not currently being
> measured."), followed by the problem statement in the first paragraph.
>
> >
> > For secure boot attestation, it is necessary to measure the kernel
>
> Secure boot enforces local file data integrity. The term here should
> be "trusted boot attestation".
>
> > command line and the kernel version.
>
> The original version of this patch set included the kernel version.
> This version is just measuring the boot command line arguments.
>
Sorry missed it while updating the cover letter.
<snip>

> > The ima logs need to be carried over to the next kernel, which will be followed
> > up by other patchsets for x86_64 and arm64.
> >
> > The kexec cmdline hash
>
> ^stored in the "d-ng" field of the template data
>
I will add another template-name for ima-buf
> > can be verified using
>
> > sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements |
> > grep kexec-cmdline | cut -d' ' -f 6 | xxd -r -p | sha256sum
>
> Until per policy template field rule support is added, a template name
> needs to be defined. Please define "ima-buf" as:
> {.name = "ima-buf", .fmt = "d-ng|n-ng|buf"}
>
> I'm still seeing some scripts/checkpatch "WARNING: line over 80
> characters". scripts/Lindent should provide the correct way of
> formatting these lines.
>
> Some people feel that references to Lindent should be removed, but I
> tend to agree with the Documentation/hwmon/submitting-patches.rst
> comment pertaining to scripts/Lindent.
>
> "* Running your patch or driver file(s) through checkpatch does not
> mean its formatting is clean. If unsure about formatting in your new
> driver, run it through Lindent. Lindent is not perfect, and you may
> have to do some minor cleanup, but it is a good start."
>
I will double check fix the issues.
> Examples of where the line formatting is off is the call to
> ima_get_action() in process_buffer_measurement() and the call to
> process_buffer_measurement() in ima_kexec_cmdline().
>
Thanks,
Prakhar Srivastava
> thanks,
>
> Mimi
<snip>