| Message ID | 20231005182602.634615-2-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: kexec: measure events between kexec load and execute | expand |
On 10/5/23 14:25, Tushar Sugandhi wrote: > IMA allocates memory and dumps the measurement during kexec soft reboot > as a single function call ima_dump_measurement_list(). It gets called > during kexec 'load' operation. It results in the IMA measurements > between the window of kexec 'load' and 'execute' getting dropped when the > system boots into the new Kernel. One of the kexec requirements is the > segment size cannot change between the 'load' and the 'execute'. > Therefore, to address this problem, ima_dump_measurement_list() needs > to be refactored to allocate the memory at kexec 'load', and dump the > measurements at kexec 'execute'. The function that allocates the memory > should handle the scenario where the kexec load is called multiple times. > > Refactor ima_dump_measurement_list() to move the memory allocation part > to a separate function ima_alloc_kexec_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_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 | 126 +++++++++++++++++++++-------- > 1 file changed, 93 insertions(+), 33 deletions(-) > > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c > index 419dc405c831..307e07991865 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -15,61 +15,114 @@ > #include "ima.h" > > #ifdef CONFIG_IMA_KEXEC > +struct seq_file ima_kexec_file; > +struct ima_kexec_hdr ima_khdr; Since you are only populating the buffer at kexec 'execute' time, you should be able to move this header into the function where it is needed. > + > +void ima_clear_kexec_file(void) > +{ > + vfree(ima_kexec_file.buf); I would pass the ima_kexec_file onto this function here as a parameter rather than accessing the file-static variable. I find this better to read once you look at ima_clear_kexec_file() further below and wonder why it doesn't take a parameter. > + ima_kexec_file.buf = NULL; > + ima_kexec_file.size = 0; > + ima_kexec_file.read_pos = 0; > + ima_kexec_file.count = 0; > +} > + > +static int ima_alloc_kexec_buf(size_t kexec_segment_size) Call it segment size to avoid the later kexec_segment_size static variable in this file. > +{ > + if ((kexec_segment_size == 0) || > + (kexec_segment_size == ULONG_MAX) || > + ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) { These tests are already done before ima_alloca_kexec_buf() is called. Also, kexec_segment_size cannot be 0. > + pr_err("%s: Invalid segment size for kexec: %zu\n", > + __func__, kexec_segment_size); > + return -EINVAL; > + } > + > + /* > + * If kexec load was called before, clear the existing buffer > + * before allocating a new one > + */ > + if (ima_kexec_file.buf) > + ima_clear_kexec_file(); > + ima_clear_file(&ima_kexec_file); > + /* segment size can't change between kexec load and execute */ > + ima_kexec_file.buf = vmalloc(kexec_segment_size); > + if (!ima_kexec_file.buf) { > + pr_err("%s: No memory for ima kexec measurement buffer\n", > + __func__); > + return -ENOMEM; > + } > + > + ima_kexec_file.size = kexec_segment_size; > + ima_kexec_file.read_pos = 0; > + ima_kexec_file.count = sizeof(ima_khdr); /* reserved space */ > + > + memset(&ima_khdr, 0, sizeof(ima_khdr)); > + ima_khdr.version = 1; Move this into ima_dump_measurement_list() since it's only used there once and getting rid of this file-static variable is a plus. > + > + 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; Don't remove it from here. > 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) { > + 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 */ > + /* > + * Ensure the kexec buffer is large enough to hold ima_khdr > + */ > + if (ima_kexec_file.size < sizeof(ima_khdr)) { > + pr_err("%s: Kexec buffer size too low to hold ima_khdr\n", > + __func__); > + ima_clear_kexec_file(); > + return -ENOMEM; > + } > > - memset(&khdr, 0, sizeof(khdr)); > - khdr.version = 1; > + /* > + * If we reach here, then there is enough memory > + * of size kexec_segment_size in ima_kexec_file.buf > + * to copy at least partial IMA log. > + * Make best effort to copy as many IMA measurements > + * as possible. You can reformat these comments to (at least) 80 columns. > + */ > list_for_each_entry_rcu(qe, &ima_measurements, later) { > - if (file.count < file.size) { > - khdr.count++; > - ima_measurements_show(&file, qe); > + if (ima_kexec_file.count < ima_kexec_file.size) { > + ima_khdr.count++; > + ima_measurements_show(&ima_kexec_file, qe); > } else { > - ret = -EINVAL; > + ret = EFBIG; Hm, you are not looking for EFBIG after calling this function and the overrun could actually also happen in the ima_measurement_show() above and go unreported if this is the last element. if (ima_kexec_file.count < ima_kexec_file.size) { ima_khdr.count++; ima_measurements_show(&ima_kexec_file, qe); } if (ima_kexec_file.count >= ima_kexec_file.size) { /* leave ret = 0; caller doesn't need to worry about undersized buffer */ pr_err("%s: IMA log file is too big for Kexec buf\n", __func__); break; } > + 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; > + ima_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); > + ima_khdr.version = cpu_to_le16(ima_khdr.version); > + ima_khdr.count = cpu_to_le64(ima_khdr.count); > + ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size); > } > - memcpy(file.buf, &khdr, sizeof(khdr)); > + memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_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); > + *buffer_size = ima_kexec_file.count; > + *buffer = ima_kexec_file.buf; > + > return ret; > } > > @@ -108,13 +161,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_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;
Thanks a lot Stefan for reviewing this series. Really appreciate it. On 10/12/23 17:28, Stefan Berger wrote: > > On 10/5/23 14:25, Tushar Sugandhi wrote: >> IMA allocates memory and dumps the measurement during kexec soft reboot >> as a single function call ima_dump_measurement_list(). It gets called >> during kexec 'load' operation. It results in the IMA measurements >> between the window of kexec 'load' and 'execute' getting dropped when the >> system boots into the new Kernel. One of the kexec requirements is the >> segment size cannot change between the 'load' and the 'execute'. >> Therefore, to address this problem, ima_dump_measurement_list() needs >> to be refactored to allocate the memory at kexec 'load', and dump the >> measurements at kexec 'execute'. The function that allocates the memory >> should handle the scenario where the kexec load is called multiple times. >> >> Refactor ima_dump_measurement_list() to move the memory allocation part >> to a separate function ima_alloc_kexec_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_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 | 126 +++++++++++++++++++++-------- >> 1 file changed, 93 insertions(+), 33 deletions(-) >> >> diff --git a/security/integrity/ima/ima_kexec.c >> b/security/integrity/ima/ima_kexec.c >> index 419dc405c831..307e07991865 100644 >> --- a/security/integrity/ima/ima_kexec.c >> +++ b/security/integrity/ima/ima_kexec.c >> @@ -15,61 +15,114 @@ >> #include "ima.h" >> #ifdef CONFIG_IMA_KEXEC >> +struct seq_file ima_kexec_file; >> +struct ima_kexec_hdr ima_khdr; > > Since you are only populating the buffer at kexec 'execute' time, you > should be able to move this header into the function where it is needed. > Yup, ima_khdr doesn't need to be static. Will fix. > >> + >> +void ima_clear_kexec_file(void) >> +{ >> + vfree(ima_kexec_file.buf); > I would pass the ima_kexec_file onto this function here as a parameter > rather than accessing the file-static variable. I find this better to > read once you look at ima_clear_kexec_file() further below and wonder > why it doesn't take a parameter. Agreed. This will make the code more readable. >> + ima_kexec_file.buf = NULL; >> + ima_kexec_file.size = 0; >> + ima_kexec_file.read_pos = 0; >> + ima_kexec_file.count = 0; >> +} >> + >> +static int ima_alloc_kexec_buf(size_t kexec_segment_size) > > Call it segment size to avoid the later kexec_segment_size static > variable in this file. > > Will do. >> +{ >> + if ((kexec_segment_size == 0) || >> + (kexec_segment_size == ULONG_MAX) || >> + ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) { > > These tests are already done before ima_alloca_kexec_buf() is called. > Also, kexec_segment_size cannot be 0. > > I was being extra cautious here. If ima_alloc_kexec_buf() gets called from some other place, then this check would be handy. Being said that, maybe this function is a better place to have this check. I will see what I can do here to simplify things. Thanks for the feedback. >> + pr_err("%s: Invalid segment size for kexec: %zu\n", >> + __func__, kexec_segment_size); >> + return -EINVAL; >> + } >> + >> + /* >> + * If kexec load was called before, clear the existing buffer >> + * before allocating a new one >> + */ >> + if (ima_kexec_file.buf) >> + ima_clear_kexec_file(); >> + > > ima_clear_file(&ima_kexec_file); > > Agreed. Will do. >> + /* segment size can't change between kexec load and execute */ >> + ima_kexec_file.buf = vmalloc(kexec_segment_size); >> + if (!ima_kexec_file.buf) { >> + pr_err("%s: No memory for ima kexec measurement buffer\n", >> + __func__); >> + return -ENOMEM; >> + } >> + >> + ima_kexec_file.size = kexec_segment_size; >> + ima_kexec_file.read_pos = 0; >> + ima_kexec_file.count = sizeof(ima_khdr); /* reserved space */ >> + >> + memset(&ima_khdr, 0, sizeof(ima_khdr)); >> + ima_khdr.version = 1; > > Move this into ima_dump_measurement_list() since it's only used there > once and getting rid of this file-static variable is a plus. > > Yes. This will get rid of an extra static variable. >> + >> + 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; > Don't remove it from here. Agreed. khdr doesn't need to be static. >> 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) { >> + 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 */ >> + /* >> + * Ensure the kexec buffer is large enough to hold ima_khdr >> + */ >> + if (ima_kexec_file.size < sizeof(ima_khdr)) { >> + pr_err("%s: Kexec buffer size too low to hold ima_khdr\n", >> + __func__); >> + ima_clear_kexec_file(); >> + return -ENOMEM; >> + } >> - memset(&khdr, 0, sizeof(khdr)); >> - khdr.version = 1; >> + /* >> + * If we reach here, then there is enough memory >> + * of size kexec_segment_size in ima_kexec_file.buf >> + * to copy at least partial IMA log. >> + * Make best effort to copy as many IMA measurements >> + * as possible. > You can reformat these comments to (at least) 80 columns. > Sure thing. >> + */ >> list_for_each_entry_rcu(qe, &ima_measurements, later) { >> - if (file.count < file.size) { >> - khdr.count++; >> - ima_measurements_show(&file, qe); >> + if (ima_kexec_file.count < ima_kexec_file.size) { >> + ima_khdr.count++; >> + ima_measurements_show(&ima_kexec_file, qe); >> } else { >> - ret = -EINVAL; >> + ret = EFBIG; > > Hm, you are not looking for EFBIG after calling this function and the > overrun could actually also happen in the ima_measurement_show() above > and go unreported if this is the last element. > > if (ima_kexec_file.count < ima_kexec_file.size) { > ima_khdr.count++; > ima_measurements_show(&ima_kexec_file, qe); > } > > if (ima_kexec_file.count >= ima_kexec_file.size) { > /* leave ret = 0; caller doesn't need to worry about undersized > buffer */ > pr_err("%s: IMA log file is too big for Kexec buf\n", > __func__); > break; > } > This makes it more clean. Thanks. Will do. ~Tushar >> + 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; >> + ima_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); >> + ima_khdr.version = cpu_to_le16(ima_khdr.version); >> + ima_khdr.count = cpu_to_le64(ima_khdr.count); >> + ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size); >> } >> - memcpy(file.buf, &khdr, sizeof(khdr)); >> + memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_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); >> + *buffer_size = ima_kexec_file.count; >> + *buffer = ima_kexec_file.buf; >> + >> return ret; >> } >> @@ -108,13 +161,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_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;
On 10/20/23 16:33, Tushar Sugandhi wrote: > Thanks a lot Stefan for reviewing this series. > Really appreciate it. You are welcome. What may be a bit problematic is the fact that between the time the buffer for the flattened IMA log is allocated (kexec 'load') and the time it is filled (kexec 'exec') that the log may grow quite a bit. I would say that the size of the growths may depend a lot on how people are using their system that the additional kilobytes may or may not be enough. So a distro would probably have to specify additional bytes to allocate for like the worst case. But how many kilobytes is the worst case? Stefan
On 10/20/23 14:21, Stefan Berger wrote: > > On 10/20/23 16:33, Tushar Sugandhi wrote: >> Thanks a lot Stefan for reviewing this series. >> Really appreciate it. > > > You are welcome. > > What may be a bit problematic is the fact that between the time the > buffer for the flattened IMA log is allocated (kexec 'load') and the > time it is filled (kexec 'exec') that the log may grow quite a bit. I > would say that the size of the growths may depend a lot on how people > are using their system that the additional kilobytes may or may not be > enough. So a distro would probably have to specify additional bytes to > allocate for like the worst case. But how many kilobytes is the worst case? > > Stefan Yes. Its a genuine concern. The window between kexec 'load' and 'execute' could be arbitrarily long. (hours, days, months). And the log can grow quite a bit. And there is always a possibility that it will run out of the extra allocated memory- no matter how much we allocate at load. We can never know with certainty - how many kilobytes is the worst case? So I used another approach to address this issue. I addressed this issue in patch 7/7 of this series[1] by measuring a marker event ("kexec_execute") just before kexec 'execute'. Also pasting the code from 7/7 below[1] for reference. If IMA runs out of the extra allocated memory while copying the events, this marker event ("kexec_execute") will not be present in the IMA log when the system boots into the new Kernel. So the event sequence in IMA log would be as follows: IMA log -------- 'boot_aggregate' # clean boot ... ... # events before new kexec 'load' ... 'kexec_load' ... ...# arbitrary many more events ... ... ... 'kexec_execute' #if this 'kexec_execute' event is missing after the #system kexec soft boots into the new Kernel, #i.e. between the two boot_aggregate events, #it can be safely concluded that the IMA log #ran out of memory in during kexec reboot, #and now it is out of sync with PCR quotes #and thus the system needs to be hard rebooted. 'boot_aggregate' # clean boot ... ... # events after kexec soft boot ... This logic can effectively conclude if IMA log is out of sync with the PCR quotes. If it is, then the remote attestation service/client can take appropriate action on the system (clean boot) to recover. Hope this approach makes sense. ~Tushar [1] [v2,7/7] ima: record log size at kexec load and execute https://patchwork.kernel.org/project/linux-integrity/patch/20231005182602.634615-8-tusharsu@linux.microsoft.com/ diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c index 6cd5f46a7208..0f9c424fe808 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -17,6 +17,8 @@ #include "ima.h" #ifdef CONFIG_IMA_KEXEC +#define IMA_KEXEC_EVENT_LEN 128 + struct seq_file ima_kexec_file; struct ima_kexec_hdr ima_khdr; static void *ima_kexec_buffer; @@ -34,6 +36,8 @@ void ima_clear_kexec_file(void) static int ima_alloc_kexec_buf(size_t kexec_segment_size) { + char ima_kexec_event[IMA_KEXEC_EVENT_LEN]; + if ((kexec_segment_size == 0) || (kexec_segment_size == ULONG_MAX) || ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) { @@ -64,6 +68,12 @@ static int ima_alloc_kexec_buf(size_t kexec_segment_size) memset(&ima_khdr, 0, sizeof(ima_khdr)); ima_khdr.version = 1; + scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN, + "kexec_segment_size=%lu;", kexec_segment_size); + + ima_measure_critical_data("ima_kexec", "kexec_load", ima_kexec_event, + strlen(ima_kexec_event), false, NULL, 0); + return 0; } @@ -198,6 +208,7 @@ void ima_add_kexec_buffer(struct kimage *image) static int ima_update_kexec_buffer(struct notifier_block *self, unsigned long action, void *data) { + char ima_kexec_event[IMA_KEXEC_EVENT_LEN]; void *buf = NULL; size_t buf_size; bool resume = false; @@ -213,9 +224,31 @@ static int ima_update_kexec_buffer(struct notifier_block *self, return NOTIFY_OK; } + buf_size = ima_get_binary_runtime_size(); + scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN, + "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;", + kexec_segment_size, buf_size); + + /* + * This is one of the very last events measured by IMA before kexec + * soft rebooting into the new Kernal. + * This event can be used as a marker after the system soft reboots + * to the new Kernel to check if there was sufficient memory allocated + * at kexec 'load' to capture the events measured between the window + * of kexec 'load' and 'execute'. + * This event needs to be present in the IMA log, in between the two + * 'boot_aggregate' events that are logged for the previous boot and + * the current soft reboot. If it is not present after the system soft + * reboots into the new Kernel, it would mean the IMA log is not + * consistent with the TPM PCR quotes, and the system needs to be + * cold-booted for the attestation to succeed again. + */ + ima_measure_critical_data("ima_kexec", "kexec_execute", + ima_kexec_event, strlen(ima_kexec_event), + false, NULL, 0); + ima_measurements_suspend(); - buf_size = ima_get_binary_runtime_size(); ret = ima_dump_measurement_list(&buf_size, &buf, kexec_segment_size);
Hi Tushar, According to Documentation/process/submitting-patches.rst, the subject line should be between 70-75 characters. Perhaps something like "ima: define and call ima_alloc_kexec_buffer()". On Thu, 2023-10-05 at 11:25 -0700, Tushar Sugandhi wrote: > IMA allocates memory and dumps the measurement during kexec soft reboot > as a single function call ima_dump_measurement_list(). It gets called > during kexec 'load' operation. It results in the IMA measurements > between the window of kexec 'load' and 'execute' getting dropped when the > system boots into the new Kernel. One of the kexec requirements is the > segment size cannot change between the 'load' and the 'execute'. > Therefore, to address this problem, ima_dump_measurement_list() needs > to be refactored to allocate the memory at kexec 'load', and dump the > measurements at kexec 'execute'. The function that allocates the memory > should handle the scenario where the kexec load is called multiple times The above pragraph is unnecessary. > Refactor ima_dump_measurement_list() to move the memory allocation part > to a separate function ima_alloc_kexec_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_buf(). Make necessary changes to the function > ima_add_kexec_buffer() to call the above two functions. Fix the wording based on the suggested changes below. > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> Before re-posting this patch set, verify there aren't any "checkpatch.pl --strict" issues. After applying each patch, compile the kernel and verify it still works. > --- > security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++-------- > 1 file changed, 93 insertions(+), 33 deletions(-) > > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c > index 419dc405c831..307e07991865 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -15,61 +15,114 @@ > #include "ima.h" > > #ifdef CONFIG_IMA_KEXEC > +struct seq_file ima_kexec_file; 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. > +struct ima_kexec_hdr ima_khdr; > + > +void ima_clear_kexec_file(void) The opposite of "set" is "clear" (e.g. set_git, clear_bit). The opposite of "alloc" would be "free" (e.g. ima_alloc_kexec_buf, ima_free_kexec_buf) > +{ > + vfree(ima_kexec_file.buf); > + ima_kexec_file.buf = NULL; > + ima_kexec_file.size = 0; > + ima_kexec_file.read_pos = 0; > + ima_kexec_file.count = 0; > +} > + > +static int ima_alloc_kexec_buf(size_t kexec_segment_size) > +{ > + if ((kexec_segment_size == 0) || > + (kexec_segment_size == ULONG_MAX) || > + ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) { > + pr_err("%s: Invalid segment size for kexec: %zu\n", > + __func__, kexec_segment_size); > + return -EINVAL; > + } Please avoid code duplication. If moving the test here, then remove it from its original place. > + > + /* > + * If kexec load was called before, clear the existing buffer > + * before allocating a new one > + */ > + if (ima_kexec_file.buf) > + ima_clear_kexec_file(); Perhaps instead of always freeing the buffer, check and see whether the size has changed. If it hasn't changed, then simply return. If it has changed, perhaps use realloc(). Possible wording: "Kexec may be called multiple times. Free and re-alloc the buffer if the size changed." > + > + /* segment size can't change between kexec load and execute */ > + ima_kexec_file.buf = vmalloc(kexec_segment_size); > + if (!ima_kexec_file.buf) { > + pr_err("%s: No memory for ima kexec measurement buffer\n", > + __func__); > + return -ENOMEM; > + } > + > + ima_kexec_file.size = kexec_segment_size; > + ima_kexec_file.read_pos = 0; > + ima_kexec_file.count = sizeof(ima_khdr); /* reserved space */ > + > + memset(&ima_khdr, 0, sizeof(ima_khdr)); > + ima_khdr.version = 1; > + > + 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) { > + 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 */ > + /* > + * Ensure the kexec buffer is large enough to hold ima_khdr > + */ > + if (ima_kexec_file.size < sizeof(ima_khdr)) { > + pr_err("%s: Kexec buffer size too low to hold ima_khdr\n", > + __func__); > + ima_clear_kexec_file(); > + return -ENOMEM; > + } Is this necessary? > - memset(&khdr, 0, sizeof(khdr)); > - khdr.version = 1; > + /* > + * If we reach here, then there is enough memory > + * of size kexec_segment_size in ima_kexec_file.buf > + * to copy at least partial IMA log. > + * Make best effort to copy as many IMA measurements > + * as possible. > + */ Suggestion: /* Copy as many IMA measurements list records as possible */ > list_for_each_entry_rcu(qe, &ima_measurements, later) { > - if (file.count < file.size) { > - khdr.count++; > - ima_measurements_show(&file, qe); > + if (ima_kexec_file.count < ima_kexec_file.size) { > + ima_khdr.count++; > + ima_measurements_show(&ima_kexec_file, qe); > } else { > - ret = -EINVAL; > + ret = EFBIG; > + 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; > + ima_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); > + ima_khdr.version = cpu_to_le16(ima_khdr.version); > + ima_khdr.count = cpu_to_le64(ima_khdr.count); > + ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size); > } > - memcpy(file.buf, &khdr, sizeof(khdr)); > + memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_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); > + *buffer_size = ima_kexec_file.count; > + *buffer = ima_kexec_file.buf; > + > return ret; > }
On Thu, 2023-10-26 at 16:16 -0400, Mimi Zohar wrote: > Hi Tushar, > > According to Documentation/process/submitting-patches.rst, the subject > line should be between 70-75 characters. > > Perhaps something like "ima: define and call ima_alloc_kexec_buffer()". > > On Thu, 2023-10-05 at 11:25 -0700, Tushar Sugandhi wrote: > > IMA allocates memory and dumps the measurement during kexec soft reboot > > as a single function call ima_dump_measurement_list(). It gets called > > during kexec 'load' operation. It results in the IMA measurements > > between the window of kexec 'load' and 'execute' getting dropped when the > > system boots into the new Kernel. One of the kexec requirements is the > > segment size cannot change between the 'load' and the 'execute'. > > Therefore, to address this problem, ima_dump_measurement_list() needs > > to be refactored to allocate the memory at kexec 'load', and dump the > > measurements at kexec 'execute'. The function that allocates the memory > > should handle the scenario where the kexec load is called multiple times > > The above pragraph is unnecessary. > > > Refactor ima_dump_measurement_list() to move the memory allocation part > > to a separate function ima_alloc_kexec_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_buf(). Make necessary changes to the function > > ima_add_kexec_buffer() to call the above two functions. > > Fix the wording based on the suggested changes below. > > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > > - Before re-posting this patch set, verify there aren't any > "checkpatch.pl --strict" issues. > - After applying each patch, compile the kernel and verify it still > works. Doing this will detect whether or not the patch set is bisect safe. > > --- > > security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++-------- > > 1 file changed, 93 insertions(+), 33 deletions(-) > > > > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c > > index 419dc405c831..307e07991865 100644 > > --- a/security/integrity/ima/ima_kexec.c > > +++ b/security/integrity/ima/ima_kexec.c > > @@ -15,61 +15,114 @@ > > #include "ima.h" > > > > #ifdef CONFIG_IMA_KEXEC > > +struct seq_file ima_kexec_file; > > 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 > > > +struct ima_kexec_hdr ima_khdr; > > + > > +void ima_clear_kexec_file(void) > > The opposite of "set" is "clear" (e.g. set_git, clear_bit). The > opposite of "alloc" would be "free" (e.g. ima_alloc_kexec_buf, > ima_free_kexec_buf) > > > +{ > > + vfree(ima_kexec_file.buf); > > + ima_kexec_file.buf = NULL; > > + ima_kexec_file.size = 0; > > + ima_kexec_file.read_pos = 0; > > + ima_kexec_file.count = 0; > > +} > > + > > +static int ima_alloc_kexec_buf(size_t kexec_segment_size) > > +{ > > + if ((kexec_segment_size == 0) || > > + (kexec_segment_size == ULONG_MAX) || > > + ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) { > > + pr_err("%s: Invalid segment size for kexec: %zu\n", > > + __func__, kexec_segment_size); > > + return -EINVAL; > > + } > > Please avoid code duplication. If moving the test here, then remove it > from its original place. > > > + > > + /* > > + * If kexec load was called before, clear the existing buffer > > + * before allocating a new one > > + */ > > > + if (ima_kexec_file.buf) > > + ima_clear_kexec_file(); > > Perhaps instead of always freeing the buffer, check and see whether the > size has changed. If it hasn't changed, then simply return. If it has > changed, perhaps use realloc(). Possible wording: > > "Kexec may be called multiple times. Free and re-alloc the buffer if > the size changed." > > > + > > + /* segment size can't change between kexec load and execute */ > > + ima_kexec_file.buf = vmalloc(kexec_segment_size); > > + if (!ima_kexec_file.buf) { > > + pr_err("%s: No memory for ima kexec measurement buffer\n", > > + __func__); > > + return -ENOMEM; > > + } > > + > > + ima_kexec_file.size = kexec_segment_size; > > + ima_kexec_file.read_pos = 0; > > + ima_kexec_file.count = sizeof(ima_khdr); /* reserved space */ > > + > > + memset(&ima_khdr, 0, sizeof(ima_khdr)); > > + ima_khdr.version = 1; > > + > > + 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) { > > + 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 */ > > + /* > > + * Ensure the kexec buffer is large enough to hold ima_khdr > > + */ > > + if (ima_kexec_file.size < sizeof(ima_khdr)) { > > + pr_err("%s: Kexec buffer size too low to hold ima_khdr\n", > > + __func__); > > + ima_clear_kexec_file(); > > + return -ENOMEM; > > + } > > Is this necessary? > > > - memset(&khdr, 0, sizeof(khdr)); > > - khdr.version = 1; > > + /* > > + * If we reach here, then there is enough memory > > + * of size kexec_segment_size in ima_kexec_file.buf > > + * to copy at least partial IMA log. > > + * Make best effort to copy as many IMA measurements > > + * as possible. > > + */ > > Suggestion: > > /* Copy as many IMA measurements list records as possible */ > > > list_for_each_entry_rcu(qe, &ima_measurements, later) { > > - if (file.count < file.size) { > > - khdr.count++; > > - ima_measurements_show(&file, qe); > > + if (ima_kexec_file.count < ima_kexec_file.size) { > > + ima_khdr.count++; > > + ima_measurements_show(&ima_kexec_file, qe); > > } else { > > - ret = -EINVAL; > > + ret = EFBIG; > > + 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; > > + ima_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); > > + ima_khdr.version = cpu_to_le16(ima_khdr.version); > > + ima_khdr.count = cpu_to_le64(ima_khdr.count); > > + ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size); > > } > > - memcpy(file.buf, &khdr, sizeof(khdr)); > > + memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_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); > > + *buffer_size = ima_kexec_file.count; > > + *buffer = ima_kexec_file.buf; > > + > > return ret; > > } >
Thanks a lot for reviewing this patch set Mimi. On 10/26/23 13:16, Mimi Zohar wrote: > Hi Tushar, > > According to Documentation/process/submitting-patches.rst, the subject > line should be between 70-75 characters. > > Perhaps something like "ima: define and call ima_alloc_kexec_buffer()". > Sure thing. I will shorten the subject line. Here and elsewhere. > On Thu, 2023-10-05 at 11:25 -0700, Tushar Sugandhi wrote: >> IMA allocates memory and dumps the measurement during kexec soft reboot >> as a single function call ima_dump_measurement_list(). It gets called >> during kexec 'load' operation. It results in the IMA measurements >> between the window of kexec 'load' and 'execute' getting dropped when the >> system boots into the new Kernel. One of the kexec requirements is the >> segment size cannot change between the 'load' and the 'execute'. >> Therefore, to address this problem, ima_dump_measurement_list() needs >> to be refactored to allocate the memory at kexec 'load', and dump the >> measurements at kexec 'execute'. The function that allocates the memory >> should handle the scenario where the kexec load is called multiple times > > The above pragraph is unnecessary. > Sounds good. I will remove the above paragraph. >> Refactor ima_dump_measurement_list() to move the memory allocation part >> to a separate function ima_alloc_kexec_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_buf(). Make necessary changes to the function >> ima_add_kexec_buffer() to call the above two functions. > > Fix the wording based on the suggested changes below. > Will do. >> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > > Before re-posting this patch set, verify there aren't any > "checkpatch.pl --strict" issues. > After applying each patch, compile the kernel and verify it still > works. > >> --- >> security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++-------- >> 1 file changed, 93 insertions(+), 33 deletions(-) >> >> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c >> index 419dc405c831..307e07991865 100644 >> --- a/security/integrity/ima/ima_kexec.c >> +++ b/security/integrity/ima/ima_kexec.c >> @@ -15,61 +15,114 @@ >> #include "ima.h" >> >> #ifdef CONFIG_IMA_KEXEC >> +struct seq_file ima_kexec_file; > > 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. > >> +struct ima_kexec_hdr ima_khdr; >> + >> +void ima_clear_kexec_file(void) > > The opposite of "set" is "clear" (e.g. set_git, clear_bit). The > opposite of "alloc" would be "free" (e.g. ima_alloc_kexec_buf, > ima_free_kexec_buf) > Agreed. Will make it ima_free_kexec_buf. >> +{ >> + vfree(ima_kexec_file.buf); >> + ima_kexec_file.buf = NULL; >> + ima_kexec_file.size = 0; >> + ima_kexec_file.read_pos = 0; >> + ima_kexec_file.count = 0; >> +} >> + >> +static int ima_alloc_kexec_buf(size_t kexec_segment_size) >> +{ >> + if ((kexec_segment_size == 0) || >> + (kexec_segment_size == ULONG_MAX) || >> + ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) { >> + pr_err("%s: Invalid segment size for kexec: %zu\n", >> + __func__, kexec_segment_size); >> + return -EINVAL; >> + } > > Please avoid code duplication. If moving the test here, then remove it > from its original place. > Sure. Will do. >> + >> + /* >> + * If kexec load was called before, clear the existing buffer >> + * before allocating a new one >> + */ > >> + if (ima_kexec_file.buf) >> + ima_clear_kexec_file(); > > Perhaps instead of always freeing the buffer, check and see whether the > size has changed. If it hasn't changed, then simply return. If it has > changed, perhaps use realloc(). Possible wording: > > "Kexec may be called multiple times. Free and re-alloc the buffer if > the size changed." > That's a good optimization. Thanks for the suggestion. Will do. >> + >> + /* segment size can't change between kexec load and execute */ >> + ima_kexec_file.buf = vmalloc(kexec_segment_size); >> + if (!ima_kexec_file.buf) { >> + pr_err("%s: No memory for ima kexec measurement buffer\n", >> + __func__); >> + return -ENOMEM; >> + } >> + >> + ima_kexec_file.size = kexec_segment_size; >> + ima_kexec_file.read_pos = 0; >> + ima_kexec_file.count = sizeof(ima_khdr); /* reserved space */ >> + >> + memset(&ima_khdr, 0, sizeof(ima_khdr)); >> + ima_khdr.version = 1; >> + >> + 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) { >> + 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 */ >> + /* >> + * Ensure the kexec buffer is large enough to hold ima_khdr >> + */ >> + if (ima_kexec_file.size < sizeof(ima_khdr)) { >> + pr_err("%s: Kexec buffer size too low to hold ima_khdr\n", >> + __func__); >> + ima_clear_kexec_file(); >> + return -ENOMEM; >> + } > > Is this necessary? > Yeh. It's an overkill. Will remove. >> - memset(&khdr, 0, sizeof(khdr)); >> - khdr.version = 1; >> + /* >> + * If we reach here, then there is enough memory >> + * of size kexec_segment_size in ima_kexec_file.buf >> + * to copy at least partial IMA log. >> + * Make best effort to copy as many IMA measurements >> + * as possible. >> + */ > > Suggestion: > > /* Copy as many IMA measurements list records as possible */ > That's a much cleaner comment. Will update. ~Tushar >> list_for_each_entry_rcu(qe, &ima_measurements, later) { >> - if (file.count < file.size) { >> - khdr.count++; >> - ima_measurements_show(&file, qe); >> + if (ima_kexec_file.count < ima_kexec_file.size) { >> + ima_khdr.count++; >> + ima_measurements_show(&ima_kexec_file, qe); >> } else { >> - ret = -EINVAL; >> + ret = EFBIG; >> + 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; >> + ima_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); >> + ima_khdr.version = cpu_to_le16(ima_khdr.version); >> + ima_khdr.count = cpu_to_le64(ima_khdr.count); >> + ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size); >> } >> - memcpy(file.buf, &khdr, sizeof(khdr)); >> + memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_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); >> + *buffer_size = ima_kexec_file.count; >> + *buffer = ima_kexec_file.buf; >> + >> return ret; >> } >
On 10/26/23 20:25, Mimi Zohar wrote: > On Thu, 2023-10-26 at 16:16 -0400, Mimi Zohar wrote: >> Hi Tushar, >> >> According to Documentation/process/submitting-patches.rst, the subject >> line should be between 70-75 characters. >> >> Perhaps something like "ima: define and call ima_alloc_kexec_buffer()". >> >> On Thu, 2023-10-05 at 11:25 -0700, Tushar Sugandhi wrote: >>> IMA allocates memory and dumps the measurement during kexec soft reboot >>> as a single function call ima_dump_measurement_list(). It gets called >>> during kexec 'load' operation. It results in the IMA measurements >>> between the window of kexec 'load' and 'execute' getting dropped when the >>> system boots into the new Kernel. One of the kexec requirements is the >>> segment size cannot change between the 'load' and the 'execute'. >>> Therefore, to address this problem, ima_dump_measurement_list() needs >>> to be refactored to allocate the memory at kexec 'load', and dump the >>> measurements at kexec 'execute'. The function that allocates the memory >>> should handle the scenario where the kexec load is called multiple times >> >> The above pragraph is unnecessary. >> >>> Refactor ima_dump_measurement_list() to move the memory allocation part >>> to a separate function ima_alloc_kexec_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_buf(). Make necessary changes to the function >>> ima_add_kexec_buffer() to call the above two functions. >> >> Fix the wording based on the suggested changes below. >> >>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >> >> - Before re-posting this patch set, verify there aren't any >> "checkpatch.pl --strict" issues. >> - After applying each patch, compile the kernel and verify it still >> works. > > Doing this will detect whether or not the patch set is bisect safe. > I usually just do checkpatch.pl <.patch file>. I didn't know about --strict and it's benefits. Will do it going forward. >>> --- >>> security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++-------- >>> 1 file changed, 93 insertions(+), 33 deletions(-) >>> >>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c >>> index 419dc405c831..307e07991865 100644 >>> --- a/security/integrity/ima/ima_kexec.c >>> +++ b/security/integrity/ima/ima_kexec.c >>> @@ -15,61 +15,114 @@ >>> #include "ima.h" >>> >>> #ifdef CONFIG_IMA_KEXEC >>> +struct seq_file ima_kexec_file; >> >> 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 > Sure :) ~Tushar ...
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c index 419dc405c831..307e07991865 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -15,61 +15,114 @@ #include "ima.h" #ifdef CONFIG_IMA_KEXEC +struct seq_file ima_kexec_file; +struct ima_kexec_hdr ima_khdr; + +void ima_clear_kexec_file(void) +{ + vfree(ima_kexec_file.buf); + ima_kexec_file.buf = NULL; + ima_kexec_file.size = 0; + ima_kexec_file.read_pos = 0; + ima_kexec_file.count = 0; +} + +static int ima_alloc_kexec_buf(size_t kexec_segment_size) +{ + if ((kexec_segment_size == 0) || + (kexec_segment_size == ULONG_MAX) || + ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) { + pr_err("%s: Invalid segment size for kexec: %zu\n", + __func__, kexec_segment_size); + return -EINVAL; + } + + /* + * If kexec load was called before, clear the existing buffer + * before allocating a new one + */ + if (ima_kexec_file.buf) + ima_clear_kexec_file(); + + /* segment size can't change between kexec load and execute */ + ima_kexec_file.buf = vmalloc(kexec_segment_size); + if (!ima_kexec_file.buf) { + pr_err("%s: No memory for ima kexec measurement buffer\n", + __func__); + return -ENOMEM; + } + + ima_kexec_file.size = kexec_segment_size; + ima_kexec_file.read_pos = 0; + ima_kexec_file.count = sizeof(ima_khdr); /* reserved space */ + + memset(&ima_khdr, 0, sizeof(ima_khdr)); + ima_khdr.version = 1; + + 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) { + 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 */ + /* + * Ensure the kexec buffer is large enough to hold ima_khdr + */ + if (ima_kexec_file.size < sizeof(ima_khdr)) { + pr_err("%s: Kexec buffer size too low to hold ima_khdr\n", + __func__); + ima_clear_kexec_file(); + return -ENOMEM; + } - memset(&khdr, 0, sizeof(khdr)); - khdr.version = 1; + /* + * If we reach here, then there is enough memory + * of size kexec_segment_size in ima_kexec_file.buf + * to copy at least partial IMA log. + * Make best effort to copy as many IMA measurements + * as possible. + */ list_for_each_entry_rcu(qe, &ima_measurements, later) { - if (file.count < file.size) { - khdr.count++; - ima_measurements_show(&file, qe); + if (ima_kexec_file.count < ima_kexec_file.size) { + ima_khdr.count++; + ima_measurements_show(&ima_kexec_file, qe); } else { - ret = -EINVAL; + ret = EFBIG; + 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; + ima_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); + ima_khdr.version = cpu_to_le16(ima_khdr.version); + ima_khdr.count = cpu_to_le64(ima_khdr.count); + ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size); } - memcpy(file.buf, &khdr, sizeof(khdr)); + memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_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); + *buffer_size = ima_kexec_file.count; + *buffer = ima_kexec_file.buf; + return ret; } @@ -108,13 +161,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_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;
IMA allocates memory and dumps the measurement during kexec soft reboot as a single function call ima_dump_measurement_list(). It gets called during kexec 'load' operation. It results in the IMA measurements between the window of kexec 'load' and 'execute' getting dropped when the system boots into the new Kernel. One of the kexec requirements is the segment size cannot change between the 'load' and the 'execute'. Therefore, to address this problem, ima_dump_measurement_list() needs to be refactored to allocate the memory at kexec 'load', and dump the measurements at kexec 'execute'. The function that allocates the memory should handle the scenario where the kexec load is called multiple times. Refactor ima_dump_measurement_list() to move the memory allocation part to a separate function ima_alloc_kexec_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_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 | 126 +++++++++++++++++++++-------- 1 file changed, 93 insertions(+), 33 deletions(-)