[PATCH v2 1/7] ima: refactor ima_dump_measurement_list to move memory allocation to a separate function

[Tushar Sugandhi]

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 at 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;