Re: [PATCH v2 2/7] ima: move ima_dump_measurement_list call from kexec load to execute

[Tushar Sugandhi <tusharsu@linux.microsoft.com>]

On 10/12/23 17:28, Stefan Berger wrote:
> 
> On 10/5/23 14:25, Tushar Sugandhi wrote:
>> In the current IMA implementation, ima_dump_measurement_list() is called
>> during the kexec 'load' operation.  This can result in loss of IMA
>> measurements taken between the 'load' and 'execute' phases when the
>> system goes through Kexec soft reboot to a new Kernel.  The call to the
>> function ima_dump_measurement_list() needs to be moved out of the
>> function ima_add_kexec_buffer() and needs to be called during the kexec
>> 'execute' operation.
>>
>> Implement a function ima_update_kexec_buffer() that is called during
>> kexec 'execute', allowing IMA to update the measurement list with the
>> events between kexec 'load' and 'execute'.  Move the
>> ima_dump_measurement_list() call from ima_add_kexec_buffer() to
>> ima_update_kexec_buffer().  Make ima_kexec_buffer and kexec_segment_size
>> variables global, so that they can be accessed during both kexec 'load'
>> and 'execute'.  Add functions ima_measurements_suspend() and
>> ima_measurements_resume() to set and reset the 'suspend_ima_measurements'
>> variable respectively, to suspend/resume IMA measurements.  Use
>> the existing 'ima_extend_list_mutex' to ensure that the operations are
>> thread-safe.  These function calls will help maintaining the integrity
>> of the IMA log while it is being copied to the new Kernel's buffer.
>> Add a reboot notifier_block 'update_buffer_nb' to ensure
>> the function ima_update_kexec_buffer() gets called during kexec
>> soft-reboot.
>>
>> Signed-off-by: Tushar Sugandhi<tusharsu@linux.microsoft.com>
>> ---
>>   security/integrity/ima/ima.h       |  2 ++
>>   security/integrity/ima/ima_kexec.c | 58 +++++++++++++++++++++++++-----
>>   security/integrity/ima/ima_queue.c | 18 ++++++++++
>>   3 files changed, 69 insertions(+), 9 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index c29db699c996..49a6047dd8eb 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -161,6 +161,8 @@ bool ima_template_has_modsig(const struct 
>> ima_template_desc *ima_template);
>>   int ima_restore_measurement_entry(struct ima_template_entry *entry);
>>   int ima_restore_measurement_list(loff_t bufsize, void *buf);
>>   int ima_measurements_show(struct seq_file *m, void *v);
>> +void ima_measurements_suspend(void);
>> +void ima_measurements_resume(void);
>>   unsigned long ima_get_binary_runtime_size(void);
>>   int ima_init_template(void);
>>   void ima_init_template_list(void);
>> diff --git a/security/integrity/ima/ima_kexec.c 
>> b/security/integrity/ima/ima_kexec.c
>> index 307e07991865..2c11bbe6efef 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -17,6 +17,8 @@
>>   #ifdef CONFIG_IMA_KEXEC
>>   struct seq_file ima_kexec_file;
>>   struct ima_kexec_hdr ima_khdr;
>> +static void *ima_kexec_buffer;
>> +static size_t kexec_segment_size;
>>   void ima_clear_kexec_file(void)
>>   {
>> @@ -142,7 +144,6 @@ void ima_add_kexec_buffer(struct kimage *image)
>>       /* use more understandable variable names than defined in kbuf */
>>       void *kexec_buffer = NULL;
>>       size_t kexec_buffer_size;
>> -    size_t kexec_segment_size;
>>       int ret;
>>       /*
>> @@ -167,14 +168,6 @@ void ima_add_kexec_buffer(struct kimage *image)
>>           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;
>> @@ -192,6 +185,53 @@ void ima_add_kexec_buffer(struct kimage *image)
>>       pr_debug("kexec measurement buffer for the loaded kernel at 
>> 0x%lx.\n",
>>            kbuf.mem);
>>   }
>> +
>> +/*
>> + * Called during kexec execute so that IMA can update the measurement 
>> list.
>> + */
>> +static int ima_update_kexec_buffer(struct notifier_block *self,
>> +                   unsigned long action, void *data)
>> +{
>> +    void *buf = NULL;
>> +    size_t buf_size;
>> +    bool resume = false;
>> +    int ret;
>> +
>> +    if (!kexec_in_progress) {
>> +        pr_info("%s: No kexec in progress.\n", __func__);
>> +        return NOTIFY_OK;
>> +    }
>> +
>> +    if (!ima_kexec_buffer) {
>> +        pr_err("%s: Kexec buffer not set.\n", __func__);
>> +        return NOTIFY_OK;
>> +    }
>> +
>> +    ima_measurements_suspend();
>> +
>> +    buf_size = ima_get_binary_runtime_size();
> There doesn't seem to be a need to call this function and pass in the 
> binary runtime size into the dump function. You should be able to remove 
> it.
Oh. This was an oversight.
This line is removed in patch 7/7.  It should have been removed here
itself.
Thanks for catching it. Will fix.
>> +    ret = ima_dump_measurement_list(&buf_size, &buf,
>> +                    kexec_segment_size);
>> +
>> +    if (!buf || ret < 0) {
> I don't think this function can (or should) ever return ret >= 0 with 
> buf == NULL.
I will simplify this check. Thanks.

~Tushar
>> +        pr_err("%s: Dump measurements failed. Error:%d\n",
>> +               __func__, ret);
>> +        resume = true;
>> +        goto out;
>> +    }
>> +    memcpy(ima_kexec_buffer, buf, buf_size);
>> +out:
>> +    ima_kexec_buffer = NULL;
>> +
>> +    if (resume)
>> +        ima_measurements_resume();
>> +
>> +    return NOTIFY_OK;
>> +}
>> +struct notifier_block update_buffer_nb = {
>> +    .notifier_call = ima_update_kexec_buffer,
>> +};
>> +
>>   #endif /* IMA_KEXEC */
>>   /*
>> diff --git a/security/integrity/ima/ima_queue.c 
>> b/security/integrity/ima/ima_queue.c
>> index 532da87ce519..9e7d1196006e 100644
>> --- a/security/integrity/ima/ima_queue.c
>> +++ b/security/integrity/ima/ima_queue.c
>> @@ -44,6 +44,11 @@ struct ima_h_table ima_htable = {
>>    */
>>   static DEFINE_MUTEX(ima_extend_list_mutex);
>> +/*
>> + * Used internally by the kernel to suspend-resume ima measurements.
>> + */
>> +static atomic_t suspend_ima_measurements;
>> +
>>   /* lookup up the digest value in the hash table, and return the 
>> entry */
>>   static struct ima_queue_entry *ima_lookup_digest_entry(u8 
>> *digest_value,
>>                                  int pcr)
>> @@ -147,6 +152,19 @@ static int ima_pcr_extend(struct tpm_digest 
>> *digests_arg, int pcr)
>>           pr_err("Error Communicating to TPM chip, result: %d\n", 
>> result);
>>       return result;
>>   }
>> +void ima_measurements_suspend(void)
>> +{
>> +    mutex_lock(&ima_extend_list_mutex);
>> +    atomic_set(&suspend_ima_measurements, 1);
>> +    mutex_unlock(&ima_extend_list_mutex);
>> +}
>> +
>> +void ima_measurements_resume(void)
>> +{
>> +    mutex_lock(&ima_extend_list_mutex);
>> +    atomic_set(&suspend_ima_measurements, 0);
>> +    mutex_unlock(&ima_extend_list_mutex);
>> +}
>>   /*
>>    * Add template entry to the measurement list and hash table, and

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec