Messages in this thread | | | Subject | Re: [PATCH v9 3/8] IMA: define a hook to measure kernel integrity critical data | From | Mimi Zohar <> | Date | Thu, 24 Dec 2020 08:04:40 -0500 |
| |
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@linux.microsoft.com> > Reviewed-by: Tyler Hicks <tyhicks@linux.microsoft.com> > ---
<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;
| |