| Message ID | 20231216010729.2904751-6-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: kexec: measure events between kexec load and execute | expand |
On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote: > If the new measurements are added to the IMA log while it is being > being copied to the kexec buffer during kexec 'execute', it can miss > copying those new measurements to the kexec buffer, and the buffer can go > out of sync with TPM PCRs. This could result in breaking the integrity > of the measurements after the kexec soft reboot to the new Kernel. > > Add a check in the ima_add_template_entry() function not to measure > events and return from the function early when 'suspend_ima_measurements' > flag is set. > > This ensures the consistency of the IMA measurement list while copying > them to the kexec buffer. When the 'suspend_ima_measurements' flag is > set, any new measurements will be ignored until the flag is unset. This > allows the buffer to be safely copied without worrying about concurrent > modifications to the measurement list. This is crucial for maintaining > the integrity of the measurements during a kexec soft reboot. > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > --- > security/integrity/ima/ima_queue.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c > index cb9abc02a304..5946a26a2849 100644 > --- a/security/integrity/ima/ima_queue.c > +++ b/security/integrity/ima/ima_queue.c > @@ -195,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) { I assume you meant to include the suspend/resume code in "ima: kexec: move ima log copy from kexec load to execute" in this patch.
On 12/20/23 12:44, Mimi Zohar wrote: > On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote: >> If the new measurements are added to the IMA log while it is being >> being copied to the kexec buffer during kexec 'execute', it can miss >> copying those new measurements to the kexec buffer, and the buffer can go >> out of sync with TPM PCRs. This could result in breaking the integrity >> of the measurements after the kexec soft reboot to the new Kernel. >> >> Add a check in the ima_add_template_entry() function not to measure >> events and return from the function early when 'suspend_ima_measurements' >> flag is set. >> >> This ensures the consistency of the IMA measurement list while copying >> them to the kexec buffer. When the 'suspend_ima_measurements' flag is >> set, any new measurements will be ignored until the flag is unset. This >> allows the buffer to be safely copied without worrying about concurrent >> modifications to the measurement list. This is crucial for maintaining >> the integrity of the measurements during a kexec soft reboot. >> >> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >> --- >> security/integrity/ima/ima_queue.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c >> index cb9abc02a304..5946a26a2849 100644 >> --- a/security/integrity/ima/ima_queue.c >> +++ b/security/integrity/ima/ima_queue.c >> @@ -195,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) { > > I assume you meant to include the suspend/resume code in "ima: kexec: > move ima log copy from kexec load to execute" in this patch. > Sure, I can move the suspend/resume code from Patch 2/7 of this series to this patch (5/7). Earlier I introduced the suspend/resume functionality in patch 2 because it was used in the functions in that patch. But shifting it hear will make the patches cleaner. ~Tushar
On Fri, 2024-01-05 at 11:50 -0800, Tushar Sugandhi wrote: > > On 12/20/23 12:44, Mimi Zohar wrote: > > On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote: > >> If the new measurements are added to the IMA log while it is being > >> being copied to the kexec buffer during kexec 'execute', it can miss > >> copying those new measurements to the kexec buffer, and the buffer can go > >> out of sync with TPM PCRs. This could result in breaking the integrity > >> of the measurements after the kexec soft reboot to the new Kernel. > >> > >> Add a check in the ima_add_template_entry() function not to measure > >> events and return from the function early when 'suspend_ima_measurements' > >> flag is set. > >> > >> This ensures the consistency of the IMA measurement list while copying > >> them to the kexec buffer. When the 'suspend_ima_measurements' flag is > >> set, any new measurements will be ignored until the flag is unset. This > >> allows the buffer to be safely copied without worrying about concurrent > >> modifications to the measurement list. This is crucial for maintaining > >> the integrity of the measurements during a kexec soft reboot. > >> > >> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > >> --- > >> security/integrity/ima/ima_queue.c | 13 +++++++++++++ > >> 1 file changed, 13 insertions(+) > >> > >> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c > >> index cb9abc02a304..5946a26a2849 100644 > >> --- a/security/integrity/ima/ima_queue.c > >> +++ b/security/integrity/ima/ima_queue.c > >> @@ -195,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) { > > > > I assume you meant to include the suspend/resume code in "ima: kexec: > > move ima log copy from kexec load to execute" in this patch. > > > > Sure, I can move the suspend/resume code from Patch 2/7 of this series > to this patch (5/7). > > Earlier I introduced the suspend/resume functionality in patch 2 because > it was used in the functions in that patch. > > But shifting it hear will make the patches cleaner. Just a reminder this isn't the only issued mentioned in 2/7. Please refer to it for the other comments (e.g. make not including/verifying the IMA segment hash a separate patch). Before reposting, please remember to test after applying each patch in the patch set to ensure that the measurement list is properly carried across kexec.
On 1/11/24 09:30, Mimi Zohar wrote: > On Fri, 2024-01-05 at 11:50 -0800, Tushar Sugandhi wrote: >> >> On 12/20/23 12:44, Mimi Zohar wrote: >>> On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote: >>>> If the new measurements are added to the IMA log while it is being >>>> being copied to the kexec buffer during kexec 'execute', it can miss >>>> copying those new measurements to the kexec buffer, and the buffer can go >>>> out of sync with TPM PCRs. This could result in breaking the integrity >>>> of the measurements after the kexec soft reboot to the new Kernel. >>>> >>>> Add a check in the ima_add_template_entry() function not to measure >>>> events and return from the function early when 'suspend_ima_measurements' >>>> flag is set. >>>> >>>> This ensures the consistency of the IMA measurement list while copying >>>> them to the kexec buffer. When the 'suspend_ima_measurements' flag is >>>> set, any new measurements will be ignored until the flag is unset. This >>>> allows the buffer to be safely copied without worrying about concurrent >>>> modifications to the measurement list. This is crucial for maintaining >>>> the integrity of the measurements during a kexec soft reboot. >>>> >>>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >>>> --- >>>> security/integrity/ima/ima_queue.c | 13 +++++++++++++ >>>> 1 file changed, 13 insertions(+) >>>> >>>> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c >>>> index cb9abc02a304..5946a26a2849 100644 >>>> --- a/security/integrity/ima/ima_queue.c >>>> +++ b/security/integrity/ima/ima_queue.c >>>> @@ -195,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) { >>> >>> I assume you meant to include the suspend/resume code in "ima: kexec: >>> move ima log copy from kexec load to execute" in this patch. >>> >> >> Sure, I can move the suspend/resume code from Patch 2/7 of this series >> to this patch (5/7). >> >> Earlier I introduced the suspend/resume functionality in patch 2 because >> it was used in the functions in that patch. >> >> But shifting it hear will make the patches cleaner. > > Just a reminder this isn't the only issued mentioned in 2/7. Please refer to it > for the other comments (e.g. make not including/verifying the IMA segment hash a > separate patch). > > Before reposting, please remember to test after applying each patch in the patch > set to ensure that the measurement list is properly carried across kexec. Yes, I had read your responses on patch 2/7. I have been meaning to respond to you on 2/7, but I kept getting distracted by some other work-items on my plate. Really sorry :( I will respond to your comments on 2/7 by end of the day, and incorporate the feedback before reposting.
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index cb9abc02a304..5946a26a2849 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -195,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) {
If the new measurements are added to the IMA log while it is being being copied to the kexec buffer during kexec 'execute', it can miss copying those new measurements to the kexec buffer, and the buffer can go out of sync with TPM PCRs. This could result in breaking the integrity of the measurements after the kexec soft reboot to the new Kernel. Add a check in the ima_add_template_entry() function not to measure events and return from the function early when 'suspend_ima_measurements' flag is set. This ensures the consistency of the IMA measurement list while copying them to the kexec buffer. When the 'suspend_ima_measurements' flag is set, any new measurements will be ignored until the flag is unset. This allows the buffer to be safely copied without worrying about concurrent modifications to the measurement list. This is crucial for maintaining the integrity of the measurements during a kexec soft reboot. Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> --- security/integrity/ima/ima_queue.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)