[PATCH v2 1/7] ima: refactor ima_dump_measurement_list to move memory allocation to a separate function
Tushar Sugandhi
tusharsu at linux.microsoft.com
Tue Nov 14 14:31:39 PST 2023
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 at 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;
>> }
>
More information about the kexec
mailing list