| Message ID | 20201212180251.9943-4-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | IMA: support for measuring kernel integrity critical data | expand |
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; -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 2020-12-24 5:04 a.m., Mimi Zohar wrote: > 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. > Will update the description accordingly. >> data. However, various data structures, policies, and states > > Here and everywhere else, there are two blanks after a period. > I checked this patch file in multiple text editors, but couldn’t find any instance of period followed by two spaces. I will double check again all the patches for multiple spaces, and remove them if any. >> 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. > Will do. >> 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". > Will do. >> >> 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. > Thanks for the suggestion. I will use “Define ima_measure_critical_data, a new IMA hook, to ...” >> >> 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? I simply used the description from p_b_m() * @eventname: event name to be used for the buffer entry. By "buffer entry" do you mean a record in the IMA > measurement list? > Yes, a record in the IMA measurement list. Will remove the future tense and reword it to something like: * @event_name: event name for the buffer measurement entry -or- * @event_name: event name for the record in the IMA measurement list >> + * @buf: pointer to buffer containing data to measure > > ^pointer to buffer data > will do. >> + * @buf_len: length of buffer(in bytes) > > ^length of buffer data (in bytes) > will do. >> + * @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 > Sounds good. Will do. >> + * >> + * 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 My impression was the hooks in ima_main.c e.g. ima_file_free() ima_file_mmap() required the double-asterisk ("/**"), and internal functions like ima_rdwr_violation_check() require a single-asterisk ("/*") kernel-doc.rst suggest the double-asterisk ("/**") for function comment as well. Function documentation ---------------------- The general format of a function and function-like macro kernel-doc comment is:: /** * function_name() - Brief description of function. Please let me know if you still want me to remove the double-asterisk ("/**") here. > 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. > Thanks. I will remove the prefix "@" from <variable name> in the longer function description. >> + */ >> +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. > Good point. I will switch to either size_t or unsigned int. >> + 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 > Will do. > thanks, > > Mimi > Thanks, Tushar >> +} >> + >> static int __init init_ima(void) >> { >> int error; > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Tue, 2021-01-05 at 12:01 -0800, Tushar Sugandhi wrote: > > >> data. However, various data structures, policies, and states > > > > Here and everywhere else, there are two blanks after a period. > > > I checked this patch file in multiple text editors, but couldn’t find > any instance of period followed by two spaces. I will double check again > all the patches for multiple spaces, and remove them if any. There should be two blanks after a period, not one blank. <snip> > >> + * > >> + * 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 > My impression was the hooks in ima_main.c e.g. ima_file_free() > ima_file_mmap() required the double-asterisk ("/**"), and internal > functions like ima_rdwr_violation_check() require a single-asterisk > ("/*") > > kernel-doc.rst suggest the double-asterisk ("/**") for function comment > as well. > > Function documentation > ---------------------- > > The general format of a function and function-like macro kernel-doc > comment is:: > > /** > * function_name() - Brief description of function. > > Please let me know if you still want me to remove the double-asterisk > ("/**") here. Yes, of course this needs to be kernel-doc and requires "/**" > > > 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. > > > Thanks. I will remove the prefix "@" from <variable name> in the longer > function description. Removing the @<variable name> isn't sufficient. Please look at other examples of longer function definitions before reposting. thanks, Mimi -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 2021-01-05 12:16 p.m., Mimi Zohar wrote: > On Tue, 2021-01-05 at 12:01 -0800, Tushar Sugandhi wrote: >> >>>> data. However, various data structures, policies, and states >>> >>> Here and everywhere else, there are two blanks after a period. >>> >> I checked this patch file in multiple text editors, but couldn’t find >> any instance of period followed by two spaces. I will double check again >> all the patches for multiple spaces, and remove them if any. > > There should be two blanks after a period, not one blank. > > <snip> > >>>> + * >>>> + * 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 >> My impression was the hooks in ima_main.c e.g. ima_file_free() >> ima_file_mmap() required the double-asterisk ("/**"), and internal >> functions like ima_rdwr_violation_check() require a single-asterisk >> ("/*") >> >> kernel-doc.rst suggest the double-asterisk ("/**") for function comment >> as well. >> >> Function documentation >> ---------------------- >> >> The general format of a function and function-like macro kernel-doc >> comment is:: >> >> /** >> * function_name() - Brief description of function. >> >> Please let me know if you still want me to remove the double-asterisk >> ("/**") here. > > Yes, of course this needs to be kernel-doc and requires "/**" > Thanks for confirming. >> >>> 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. >>> >> Thanks. I will remove the prefix "@" from <variable name> in the longer >> function description. > > Removing the @<variable name> isn't sufficient. Please look at other > examples of longer function definitions before reposting. > Yes. Agreed. I will go as per the guidance in kernel-doc.rst Thanks again, Tushar > thanks, > > Mimi > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/include/linux/ima.h b/include/linux/ima.h index ac3d82f962f2..675f54db6264 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -30,6 +30,9 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size, extern void ima_post_path_mknod(struct dentry *dentry); extern int ima_file_hash(struct file *file, char *buf, size_t buf_size); extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size); +extern void ima_measure_critical_data(const char *event_name, + const void *buf, int buf_len, + bool measure_buf_hash); #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM extern void ima_appraise_parse_cmdline(void); @@ -122,6 +125,9 @@ static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size) } static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {} +static inline void ima_measure_critical_data(const char *event_name, + const void *buf, int buf_len, + bool measure_buf_hash) {} #endif /* CONFIG_IMA */ #ifndef CONFIG_IMA_KEXEC diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index fa3044a7539f..7d9deda6a8b3 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -201,6 +201,7 @@ static inline unsigned int ima_hash_key(u8 *digest) hook(POLICY_CHECK, policy) \ hook(KEXEC_CMDLINE, kexec_cmdline) \ hook(KEY_CHECK, key) \ + hook(CRITICAL_DATA, critical_data) \ hook(MAX_CHECK, none) #define __ima_hook_enumify(ENUM, str) ENUM, diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index af218babd198..9917e1730cb6 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -176,7 +176,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename, * subj=, obj=, type=, func=, mask=, fsmagic= * subj,obj, and type: are LSM specific. * func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK - * | KEXEC_CMDLINE | KEY_CHECK + * | KEXEC_CMDLINE | KEY_CHECK | CRITICAL_DATA * mask: contains the permission mask * fsmagic: hex value * 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 + * @buf: pointer to buffer containing data to measure + * @buf_len: length of buffer(in bytes) + * @measure_buf_hash: measure buffer 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. + */ +void ima_measure_critical_data(const char *event_name, + const void *buf, int buf_len, + 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); +} + static int __init init_ima(void) { int error;