Re: [PATCH v4 5/7] ima: suspend measurements during buffer copy at kexec execute

[Tushar Sugandhi <tusharsu@xxxxxxxxxxxxxxxxxxx>]

On 1/23/24 10:18, Stefan Berger wrote:
> On 1/22/24 13:38, Tushar Sugandhi wrote:
> > New measurements added to the IMA log while the log is being copied
> > during the kexec 'execute' may not get copied over.  This can cause the
> > measurement log to be out of sync with the IMA TPM PCR, which could
>
> It could be out of sync with any PCR that IMA extends, most often only 
> the IMA TPM PCR.
Agreed.  That's what I meant. But I can see my above wording could be 
misleading.
I will reword it to "This can cause the measurement log to be out of 
sync with the TPM PCRs that IMA extends, which could result in..."

> > result in breaking the integrity of the measurements after kexec soft
> > reboot.
> >
> > Implement and call the functions ima_measurements_suspend() and
> > ima_measurements_resume() from ima_update_kexec_buffer().
> >
> > Add a check in the ima_add_template_entry() function not to measure
> > events when 'suspend_ima_measurements' flag is set.
> >
> > This ensures the integrity of the IMA log while it is being copied over
> > to the new Kernel during kexec 'execute'.
> >
> > Signed-off-by: Tushar Sugandhi <tusharsu@xxxxxxxxxxxxxxxxxxx>
>
> Reviewed-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
Thanks for the tag. I will add it to this patch in the next iteration of 
the series.
Please let me know if you want it to be added to any other patches.

~Tushar
> > ---
> >   security/integrity/ima/ima.h       |  2 ++
> >   security/integrity/ima/ima_kexec.c |  7 +++++++
> >   security/integrity/ima/ima_queue.c | 32 ++++++++++++++++++++++++++++++
> >   3 files changed, 41 insertions(+)
> >
> > 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 25150bfc7129..f26b5b342d84 100644
> > --- a/security/integrity/ima/ima_kexec.c
> > +++ b/security/integrity/ima/ima_kexec.c
> > @@ -179,6 +179,7 @@ static int ima_update_kexec_buffer(struct 
> > notifier_block *self,
> >       void *buf = NULL;
> >       size_t buf_size;
> >       int ret = NOTIFY_OK;
> > +    bool resume = false;
> >       if (!kexec_in_progress) {
> >           pr_info("%s: No kexec in progress.\n", __func__);
> > @@ -190,12 +191,15 @@ static int ima_update_kexec_buffer(struct 
> > notifier_block *self,
> >           return ret;
> >       }
> > +    ima_measurements_suspend();
> > +
> >       ret = ima_dump_measurement_list(&buf_size, &buf,
> >                       kexec_segment_size);
> >       if (!buf) {
> >           pr_err("%s: Dump measurements failed. Error:%d\n",
> >                  __func__, ret);
> > +        resume = true;
> >           goto out;
> >       }
> >       memcpy(ima_kexec_buffer, buf, buf_size);
> > @@ -203,6 +207,9 @@ static int ima_update_kexec_buffer(struct 
> > notifier_block *self,
> >       kimage_unmap_segment(ima_kexec_buffer);
> >       ima_kexec_buffer = NULL;
> > +    if (resume)
> > +        ima_measurements_resume();
> > +
> >       return ret;
> >   }
> > diff --git a/security/integrity/ima/ima_queue.c 
> > b/security/integrity/ima/ima_queue.c
> > index 532da87ce519..5946a26a2849 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)
> > @@ -148,6 +153,20 @@ static int ima_pcr_extend(struct tpm_digest 
> > *digests_arg, int pcr)
> >       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
> >    * extend the pcr.
> > @@ -176,6 +195,19 @@ int ima_add_template_entry(struct 
> > ima_template_entry *entry, int violation,
> >           }
> >       }
> > +    /*
> > +     * suspend_ima_measurements will be set if the system is
> > +     * undergoing kexec soft boot to a new kernel.
> > +     * suspending measurements in this short window ensures the
> > +     * consistency of the IMA measurement list during copying
> > +     * of the kexec buffer.
> > +     */
> > +    if (atomic_read(&suspend_ima_measurements)) {
> > +        audit_cause = "measurements_suspended";
> > +        audit_info = 0;
> > +        goto out;
> > +    }
> > +
> >       result = ima_add_digest_entry(entry,
> >                         !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
> >       if (result < 0) {