Messages in this thread | | | Subject | Re: [PATCH v5 3/7] IMA: add hook to measure critical data | From | Tushar Sugandhi <> | Date | Thu, 12 Nov 2020 13:57:22 -0800 |
| |
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@linux.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;
| |