Re: [PATCH v9 3/8] IMA: define a hook to measure kernel integrity critical data

From: Mimi Zohar <zohar@linux.ibm.com>
To: Tushar Sugandhi <tusharsu@linux.microsoft.com>,
	stephen.smalley.work@gmail.com, casey@schaufler-ca.com,
	agk@redhat.com, snitzer@redhat.com, gmazyland@gmail.com,
	paul@paul-moore.com
Cc: tyhicks@linux.microsoft.com, sashal@kernel.org,
	jmorris@namei.org, nramas@linux.microsoft.com,
	linux-integrity@vger.kernel.org, selinux@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	linux-kernel@vger.kernel.org, dm-devel@redhat.com
Subject: Re: [PATCH v9 3/8] IMA: define a hook to measure kernel integrity critical data
Date: Thu, 24 Dec 2020 08:04:40 -0500	[thread overview]
Message-ID: <5ae72a76664ce7011d3041689efbfe1a2c67d44f.camel@linux.ibm.com> (raw)
In-Reply-To: <20201212180251.9943-4-tusharsu@linux.microsoft.com>

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;