| Message ID | 20201101222626.6111-4-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IMA: Infrastructure for measurement of critical kernel data | expand |
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. > > 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. > > 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". > > 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". > + * @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. > + * @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. > + * @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. > + * > + * 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 > +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;
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;
Hi Tushar, On Thu, 2020-11-12 at 13:57 -0800, Tushar Sugandhi wrote: > >> 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? Just the description. > > 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? Data source support doesn't belong in this patch. Each patch should do one logical thing and only that one thing. This patch is adding support for measuring critical data. The data source patch will limit the critical data being measured. Other than updating the data source list in the documentation, definitely do not add data source support to the SELinux patch. thanks, Mimi
>>> 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? > > Data source support doesn't belong in this patch. Each patch should do > one logical thing and only that one thing. This patch is adding > support for measuring critical data. The data source patch will limit > the critical data being measured. > > Other than updating the data source list in the documentation, > definitely do not add data source support to the SELinux patch. > > thanks, > > Mimi > Makes sense, I will move the data_source from this patch to a separate one before SeLinux. And the SeLinux patch will simply update the documentation. Thanks Mimi.
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy index cd572912c593..3de6c774c37e 100644 --- a/Documentation/ABI/testing/ima_policy +++ b/Documentation/ABI/testing/ima_policy @@ -29,7 +29,7 @@ Description: base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK] [FIRMWARE_CHECK] [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK] - [KEXEC_CMDLINE] [KEY_CHECK] + [KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA] mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND] [[^]MAY_EXEC] fsmagic:= hex value diff --git a/include/linux/ima.h b/include/linux/ima.h index 8fa7bcfb2da2..60ba073f1575 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -30,6 +30,10 @@ 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_data_source, + const char *event_name, + const void *buf, int buf_len, + bool measure_buf_hash); #ifdef CONFIG_IMA_KEXEC extern void ima_add_kexec_buffer(struct kimage *image); @@ -116,6 +120,10 @@ 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_data_source, + 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 0f77e0b697a3..c1acf88e1b5d 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -200,6 +200,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 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 + * @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 + * @event_name: name of an event from the kernel subsystem that is sending + * the data for measurement + * @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 + * + * 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. + */ +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; diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 4edc9be62048..f48e82450fe1 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -1251,6 +1251,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) else if (IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) && strcmp(args[0].from, "KEY_CHECK") == 0) entry->func = KEY_CHECK; + else if (strcmp(args[0].from, "CRITICAL_DATA") == 0) + entry->func = CRITICAL_DATA; else result = -EINVAL; if (!result)
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. 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. 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. Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> --- Documentation/ABI/testing/ima_policy | 2 +- include/linux/ima.h | 8 ++++++ security/integrity/ima/ima.h | 1 + security/integrity/ima/ima_api.c | 2 +- security/integrity/ima/ima_main.c | 38 ++++++++++++++++++++++++++++ security/integrity/ima/ima_policy.c | 2 ++ 6 files changed, 51 insertions(+), 2 deletions(-)