| Message ID | 20231216010729.2904751-2-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: kexec: measure events between kexec load and execute | expand |
On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote: > Refactor ima_dump_measurement_list() to move the memory allocation part > to a separate function ima_alloc_kexec_file_buf() to allocate buffer of > size 'kexec_segment_size' at kexec 'load'. Make the local variables in > function ima_dump_measurement_list() global, so that they can be accessed > from ima_alloc_kexec_file_buf(). Only ima_kexec_file is being moved and does not need to be global, but should be defined as local static. > Make necessary changes to the function > ima_add_kexec_buffer() to call the above two functions. > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > --- > security/integrity/ima/ima_kexec.c | 96 +++++++++++++++++++++--------- > 1 file changed, 67 insertions(+), 29 deletions(-) > > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c > index 419dc405c831..ae27101d0615 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -15,62 +15,93 @@ > #include "ima.h" > > #ifdef CONFIG_IMA_KEXEC > +struct seq_file ima_kexec_file; ^static. > +void ima_free_kexec_file_buf(struct seq_file *sf) ^static > +{ > + vfree(sf->buf); > + sf->buf = NULL; > + sf->size = 0; > + sf->read_pos = 0; > + sf->count = 0; -- thanks, Mimi
Thanks Mimi for the feedback on V3. Sorry for the late response, I was on vacation during the holidays. Responses inline. Happy new year! :) On 12/20/23 08:13, Mimi Zohar wrote: > On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote: >> Refactor ima_dump_measurement_list() to move the memory allocation part >> to a separate function ima_alloc_kexec_file_buf() to allocate buffer of >> size 'kexec_segment_size' at kexec 'load'. Make the local variables in >> function ima_dump_measurement_list() global, so that they can be accessed >> from ima_alloc_kexec_file_buf(). > > Only ima_kexec_file is being moved and does not need to be global, but > should be defined as local static. > Will do. I followed your comment on V2 of this series [1], and didn't make the change last time. But making it local static scoped to this source file (ima_kexec.c) makes sense. [1] From v2 of this series https://lore.kernel.org/all/2c9e3b71-5416-4336-82f1-cd78e26dd62e@linux.microsoft.com/ >> Define "ima_kexec_file" as static since it only used in this file. >> Since the variable does not need to be global, is there still a reason >> for changing its name? Minimize code change. > > Adding "static" would make ima_kexec_file a global static variable. > Please ignore my comment about reverting the variable name change. > > Mimi > >> Make necessary changes to the function >> ima_add_kexec_buffer() to call the above two functions. >> >> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >> --- >> security/integrity/ima/ima_kexec.c | 96 +++++++++++++++++++++--------- >> 1 file changed, 67 insertions(+), 29 deletions(-) >> >> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c >> index 419dc405c831..ae27101d0615 100644 >> --- a/security/integrity/ima/ima_kexec.c >> +++ b/security/integrity/ima/ima_kexec.c >> @@ -15,62 +15,93 @@ >> #include "ima.h" >> >> #ifdef CONFIG_IMA_KEXEC >> +struct seq_file ima_kexec_file; > > ^static. > Will do. >> +void ima_free_kexec_file_buf(struct seq_file *sf) > > ^static Will do. ~Tushar > >> +{ >> + vfree(sf->buf); >> + sf->buf = NULL; >> + sf->size = 0; >> + sf->read_pos = 0; >> + sf->count = 0; > > -- > thanks, > > Mimi
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c index 419dc405c831..ae27101d0615 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -15,62 +15,93 @@ #include "ima.h" #ifdef CONFIG_IMA_KEXEC +struct seq_file ima_kexec_file; + +void ima_free_kexec_file_buf(struct seq_file *sf) +{ + vfree(sf->buf); + sf->buf = NULL; + sf->size = 0; + sf->read_pos = 0; + sf->count = 0; +} + +static int ima_alloc_kexec_file_buf(size_t segment_size) +{ + /* + * kexec 'load' may be called multiple times. + * Free and realloc the buffer only if the segment_size is + * changed from the previous kexec 'load' call. + */ + if (ima_kexec_file.buf && + ima_kexec_file.size == segment_size && + ima_kexec_file.read_pos == 0 && + ima_kexec_file.count == sizeof(struct ima_kexec_hdr)) + return 0; + + ima_free_kexec_file_buf(&ima_kexec_file); + + /* segment size can't change between kexec load and execute */ + ima_kexec_file.buf = vmalloc(segment_size); + if (!ima_kexec_file.buf) + return -ENOMEM; + + ima_kexec_file.size = segment_size; + ima_kexec_file.read_pos = 0; + ima_kexec_file.count = sizeof(struct ima_kexec_hdr); /* reserved space */ + + return 0; +} + static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, unsigned long segment_size) { struct ima_queue_entry *qe; - struct seq_file file; struct ima_kexec_hdr khdr; - int ret = 0; - /* segment size can't change between kexec load and execute */ - file.buf = vmalloc(segment_size); - if (!file.buf) { - ret = -ENOMEM; - goto out; + if (!ima_kexec_file.buf) { + *buffer_size = 0; + *buffer = NULL; + pr_err("%s: Kexec file buf not allocated\n", __func__); + return -EINVAL; } - file.size = segment_size; - file.read_pos = 0; - file.count = sizeof(khdr); /* reserved space */ - memset(&khdr, 0, sizeof(khdr)); khdr.version = 1; + + /* Copy as many IMA measurements list records as possible */ list_for_each_entry_rcu(qe, &ima_measurements, later) { - if (file.count < file.size) { + if (ima_kexec_file.count < ima_kexec_file.size) { khdr.count++; - ima_measurements_show(&file, qe); + ima_measurements_show(&ima_kexec_file, qe); } else { - ret = -EINVAL; + pr_err("%s: IMA log file is too big for Kexec buf\n", + __func__); break; } } - if (ret < 0) - goto out; - /* * fill in reserved space with some buffer details * (eg. version, buffer size, number of measurements) */ - khdr.buffer_size = file.count; + khdr.buffer_size = ima_kexec_file.count; if (ima_canonical_fmt) { khdr.version = cpu_to_le16(khdr.version); khdr.count = cpu_to_le64(khdr.count); khdr.buffer_size = cpu_to_le64(khdr.buffer_size); } - memcpy(file.buf, &khdr, sizeof(khdr)); + memcpy(ima_kexec_file.buf, &khdr, sizeof(khdr)); print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1, - file.buf, file.count < 100 ? file.count : 100, + ima_kexec_file.buf, ima_kexec_file.count < 100 ? + ima_kexec_file.count : 100, true); - *buffer_size = file.count; - *buffer = file.buf; -out: - if (ret == -EINVAL) - vfree(file.buf); - return ret; + *buffer_size = ima_kexec_file.count; + *buffer = ima_kexec_file.buf; + + return 0; } /* @@ -108,13 +139,20 @@ void ima_add_kexec_buffer(struct kimage *image) return; } - ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, - kexec_segment_size); - if (!kexec_buffer) { + ret = ima_alloc_kexec_file_buf(kexec_segment_size); + if (ret < 0) { pr_err("Not enough memory for the kexec measurement buffer.\n"); return; } + ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, + kexec_segment_size); + if (ret < 0) { + pr_err("%s: Failed to dump IMA measurements. Error:%d.\n", + __func__, ret); + return; + } + kbuf.buffer = kexec_buffer; kbuf.bufsz = kexec_buffer_size; kbuf.memsz = kexec_segment_size;
Refactor ima_dump_measurement_list() to move the memory allocation part to a separate function ima_alloc_kexec_file_buf() to allocate buffer of size 'kexec_segment_size' at kexec 'load'. Make the local variables in function ima_dump_measurement_list() global, so that they can be accessed from ima_alloc_kexec_file_buf(). Make necessary changes to the function ima_add_kexec_buffer() to call the above two functions. Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> --- security/integrity/ima/ima_kexec.c | 96 +++++++++++++++++++++--------- 1 file changed, 67 insertions(+), 29 deletions(-)