[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <25622ca6-359d-fa97-c5e6-e314cba51306@linux.microsoft.com>
Date: Thu, 12 Nov 2020 13:57:22 -0800
From: Tushar Sugandhi <tusharsu@...ux.microsoft.com>
To: Mimi Zohar <zohar@...ux.ibm.com>, stephen.smalley.work@...il.com,
casey@...aufler-ca.com, agk@...hat.com, snitzer@...hat.com,
gmazyland@...il.com, paul@...l-moore.com
Cc: tyhicks@...ux.microsoft.com, sashal@...nel.org, jmorris@...ei.org,
nramas@...ux.microsoft.com, linux-integrity@...r.kernel.org,
selinux@...r.kernel.org, linux-security-module@...r.kernel.org,
linux-kernel@...r.kernel.org, dm-devel@...hat.com
Subject: Re: [PATCH v5 3/7] IMA: add hook to measure critical data
On 2020-11-06 5:24 a.m., Mimi Zohar wrote:
> Hi Tushar,
>
> On Sun, 2020-11-01 at 14:26 -0800, Tushar Sugandhi wrote:
>> Currently, IMA does not provide a generic function for kernel subsystems
>> to measure their critical data. Examples of critical data in this context
>> could be kernel in-memory r/o structures, hash of the memory structures,
>> or data that represents a linux kernel subsystem state change. The
>> critical data, if accidentally or maliciously altered, can compromise
>> the integrity of the system.
>
> Start out with what IMA does do (e.g. measures files and more recently
> buffer data). Afterwards continue with kernel integrity critical data
> should also be measured. Please include a definition of kernel
> integrity critical data here, as well as in the cover letter.
>
Yes, will do. I will also describe what kernel integrity critical data
is – by providing a definition, and not by example - as you suggested.
(here and in the cover letter)
>>
>> A generic function provided by IMA to measure critical data would enable
>> various subsystems with easier and faster on-boarding to use IMA
>> infrastructure and would also avoid code duplication.
>
> By definition LSM and IMA hooks are generic with callers in appropriate
> places in the kernel. This paragraph is redundant.
>
Sounds good. I will remove this paragraph.
>>
>> Add a new IMA func CRITICAL_DATA and a corresponding IMA hook
>> ima_measure_critical_data() to support measuring critical data for
>> various kernel subsystems.
>
> Instead of using the word "add", it would be more appropriate to use
> the word "define". Define a new IMA hook named
> ima_measure_critical_data to measure kernel integrity critical data.
> Please also update the Subject line as well. "ima: define an IMA hook
> to measure kernel integrity critical data".
>
Sounds good. Will do.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@...ux.microsoft.com>
>> ---
>>
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index 4485d87c0aa5..6e1b11dcba53 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -921,6 +921,44 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
>> fdput(f);
>> }
>>
>> +/**
>> + * ima_measure_critical_data - measure kernel subsystem data
>> + * critical to integrity of the kernel
>
> Please change this to "measure kernel integrity critical data".
>
*Question*
Thanks Mimi. Do you want us just to update the description, or do you
want us to update the function name too?
I believe you meant just description, but still want to clarify.
ima_measure_kernel_integrity_critical_data() would be too long.
Maybe ima_measure_integrity_critical_data()?
Or do you want us to keep the existing ima_measure_critical_data()?
Could you please let us know?
>> + * @event_data_source: name of the data source being measured;
>> + * typically it should be the name of the kernel subsystem that is sending
>> + * the data for measurement
>
> Including "data_source" here isn't quite right. "data source" should
> only be added in the first patch which uses it, not here. When adding
> it please shorten the field description to "kernel data source". The
> longer explanation can be included in the longer function description.
>
*Question*
Do you mean the parameter @event_data_source should be removed from this
patch? And then later added in patch 7/7 – where SeLinux uses it?
But ima_measure_critical_data() calls process_buffer_measurement(), and
p_b_m() accepts it as part of the param @func_data.
What should we pass to p_b_m() @func_data in this patch, if we remove
@event_data_source from this patch?
>> + * @event_name: name of an event from the kernel subsystem that is sending
>> + * the data for measurement
>
> As this is being passed to process_buffer_measurement(), this should be
> the same or similar to the existing definition.
>
Ok. I will change this to @eventname to be consistemt with p_b_m().
>> + * @buf: pointer to buffer containing data to measure
>> + * @buf_len: length of buffer(in bytes)
>> + * @measure_buf_hash: if set to true - will measure hash of the buf,
>> + * instead of buf
>
> kernel doc requires a single line. In this case, please shorten the
> argument definition to "measure buffer data or buffer data hash". The
> details can be included in the longer function description.
>
Sounds good. Will do.
>> + *
>> + * A given kernel subsystem (event_data_source) may send
>> + * data (buf) to be measured when the data or the subsystem state changes.
>> + * The state/data change can be described by event_name.
>> + * Examples of critical data (buf) could be kernel in-memory r/o structures,
>> + * hash of the memory structures, or data that represents subsystem
>> + * state change.
>> + * 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.
>> + */
>
> Please remove this longer function description, replacing it something
> more appropriate. The subsequent patch that introduces the "data
> source" parameter would expand the description.
>
> thanks,
>
> Mimi
>
*Question*
Hi Mimi, will do.
Do you want the data_source to be part of SeLinux patch? (patch 7/7 of
this series).
Or a separate patch before it?
~Tushar
>> +void ima_measure_critical_data(const char *event_data_source,
>> + const char *event_name,
>> + const void *buf, int buf_len,
>> + bool measure_buf_hash)
>> +{
>> + if (!event_name || !event_data_source || !buf || !buf_len) {
>> + pr_err("Invalid arguments passed to %s().\n", __func__);
>> + return;
>> + }
>> +
>> + process_buffer_measurement(NULL, buf, buf_len, event_name,
>> + CRITICAL_DATA, 0, event_data_source,
>> + measure_buf_hash);
>> +}
>> +
>> static int __init init_ima(void)
>> {
>> int error;
Powered by blists - more mailing lists