On Sat, 2020-12-12 at 10:02 -0800, Tushar Sugandhi wrote:
> IMA provides capabilities to measure file data, and in-memory buffer
No need for the comma here.
Up to this patch set, all the patches refer to "buffer data", not "in-
memory buffer data". This patch introduces the concept of measuring
"in-memory buffer data". Please remove "in-memory" above.
> data. However, various data structures, policies, and states
Here and everywhere else, there are two blanks after a period.
> stored in kernel memory also impact the integrity of the system.
> Several kernel subsystems contain such integrity critical data. These
> kernel subsystems help protect the integrity of a device. Currently,
^integrity of the system.
> IMA does not provide a generic function for kernel subsystems to measure
> their integrity critical data.
The emphasis should not be on "kernel subsystems". Simplify to "for
measuring kernel integrity critical data".
>
> Define a new IMA hook - ima_measure_critical_data to measure kernel
> integrity critical data.
Either "ima_measure_critical_data" is between hyphens or without any
hyphens. If not hyphenated, then you could say "named
ima_measure_critical_data", but "named" isn't necessary. Or reverse "a
new IMA hook" and "ima_measure_critical_data", adding comma's like:
Define ima_measure_critical_data, a new IMA hook, to ...
Any of the above options work, just not a single hyphen.
>
> Signed-off-by: Tushar Sugandhi <tusharsu@xxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxxx>
> ---
<snip>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 0f8409d77602..dff4bce4fb09 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -922,6 +922,40 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
> fdput(f);
> }
>
> +/**
> + * ima_measure_critical_data - measure kernel integrity critical data
> + * @event_name: event name to be used for the buffer entry
Why future tense? By "buffer entry" do you mean a record in the IMA
measurement list?
> + * @buf: pointer to buffer containing data to measure
^pointer to buffer data
> + * @buf_len: length of buffer(in bytes)
^length of buffer data (in bytes)
> + * @measure_buf_hash: measure buffer hash
As requested in 2/8, please abbreviate the boolean name to "hash".
Refer to section "4) Naming" in Documentation/process/coding-style.rst
for variable naming conventions.
^@hash: measure buffer data hash
> + *
> + * Measure the kernel subsystem data, critical to the integrity of the kernel,
> + * into the IMA log and extend the @pcr.
> + *
> + * Use @event_name to describe the state/buffer data change.
> + * Examples of critical data (@buf) could be various data structures,
> + * policies, and states stored in kernel memory that can impact the integrity
> + * of the system.
> + *
> + * If @measure_buf_hash is set to true - measure hash of the buffer data,
> + * else measure the buffer data itself.
> + * @measure_buf_hash can be used to save space, if the data being measured
> + * is too large.
> + *
> + * The data (@buf) can only be measured, not appraised.
The "/**" is the start of kernel-doc. Have you seen anywhere else in
the kernel using the @<variable name> in the longer function
description? Have you seen this style of longer function
description? Refer to Documentation/doc-guide/kernel-doc.rst and other
code for examples.
> + */
> +void ima_measure_critical_data(const char *event_name,
> + const void *buf, int buf_len,
As "buf_len" should always be >= 0, it should not be defined as a
signed variable.
> + bool measure_buf_hash)
> +{
> + if (!event_name || !buf || !buf_len)
> + return;
> +
> + process_buffer_measurement(NULL, buf, buf_len, event_name,
> + CRITICAL_DATA, 0, NULL,
> + measure_buf_hash);
^hash
thanks,
Mimi
> +}
> +
> static int __init init_ima(void)
> {
> int error;
