| Message ID | 20250203232033.64123-9-chenste@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: kexec: measure events between kexec load and excute | expand |
On Mon, 2025-02-03 at 15:20 -0800, steven chen wrote: > The amount of memory allocated at kexec load, even with the extra memory > allocated, might not be large enough for the entire measurement list. The > indeterminate interval between kexec 'load' and 'execute' could exacerbate > this problem. > > Define two new IMA events, 'kexec_load' and 'kexec_execute', to be > measured as critical data at kexec 'load' and 'execute' respectively. > Report the allocated kexec segment size, IMA binary log size and the > runtime measurements count as part of those events. > > These events, and the values reported through them, serve as markers in > the IMA log to verify the IMA events are captured during kexec soft > reboot. The presence of a 'kexec_load' event in between the last two > 'boot_aggregate' events in the IMA log implies this is a kexec soft > reboot, and not a cold-boot. And the absence of 'kexec_execute' event > after kexec soft reboot implies missing events in that window which > results in inconsistency with TPM PCR quotes, necessitating a cold boot > for a successful remote attestation. As a reminder, please include directions for verifying the buffer data hash against the buffer data. The directions would be similar to those in commit 6b4da8c0e7f ("IMA: Define a new template field buf"). > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > Author: Tushar Sugandhi <tusharsu@linux.microsoft.com> > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > Signed-off-by: steven chen <chenste@linux.microsoft.com> > --- > security/integrity/ima/ima_kexec.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/security/integrity/ima/ima_kexec.c > b/security/integrity/ima/ima_kexec.c > index c9c916f69ca7..0342ddfa9342 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -17,6 +17,8 @@ > #include "ima.h" > > #ifdef CONFIG_IMA_KEXEC > +#define IMA_KEXEC_EVENT_LEN 256 > + > static struct seq_file ima_kexec_file; > static void *ima_kexec_buffer; > static size_t kexec_segment_size; > @@ -36,6 +38,24 @@ static void ima_free_kexec_file_buf(struct seq_file *sf) > ima_reset_kexec_file(sf); > } > > +static void ima_measure_kexec_event(const char *event_name) > +{ > + char ima_kexec_event[IMA_KEXEC_EVENT_LEN]; > + size_t buf_size = 0; > + long len; > + > + buf_size = ima_get_binary_runtime_size(); > + len = atomic_long_read(&ima_htable.len); > + > + scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN, > + "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;" > + "ima_runtime_measurements_count=%ld;", > + kexec_segment_size, buf_size, len); From scripts/checkpatch.pl, "Alignment should match open parenthesis". > + > + ima_measure_critical_data("ima_kexec", event_name, ima_kexec_event, > + strlen(ima_kexec_event), false, NULL, 0); From the kernel-doc scnprintf(), returns the number of bytes. There should be no need to calculate it using strlen(). > +} > + > static int ima_alloc_kexec_file_buf(size_t segment_size) > { > /* > @@ -60,6 +80,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size) > out: > ima_kexec_file.read_pos = 0; > ima_kexec_file.count = sizeof(struct ima_kexec_hdr); /* reserved space > */ > + ima_measure_kexec_event("kexec_load"); > > return 0; > } > @@ -201,6 +222,8 @@ static int ima_update_kexec_buffer(struct notifier_block *self, > return ret; > } > > + ima_measure_kexec_event("kexec_execute"); > + > ret = ima_dump_measurement_list(&buf_size, &buf, > kexec_segment_size); > After fixing up and applying this patch set to 6.14.0-rc1, I'm not seeing the "kexec_execute". Even after changing the default extra memory, I'm still not seeing the measurement. Mimi
On Fri, 2025-02-07 at 10:16 -0500, Mimi Zohar wrote: > On Mon, 2025-02-03 at 15:20 -0800, steven chen wrote: > > The amount of memory allocated at kexec load, even with the extra memory > > allocated, might not be large enough for the entire measurement list. The > > indeterminate interval between kexec 'load' and 'execute' could exacerbate > > this problem. > > > > Define two new IMA events, 'kexec_load' and 'kexec_execute', to be > > measured as critical data at kexec 'load' and 'execute' respectively. > > Report the allocated kexec segment size, IMA binary log size and the > > runtime measurements count as part of those events. > > > > These events, and the values reported through them, serve as markers in > > the IMA log to verify the IMA events are captured during kexec soft > > reboot. The presence of a 'kexec_load' event in between the last two > > 'boot_aggregate' events in the IMA log implies this is a kexec soft > > reboot, and not a cold-boot. And the absence of 'kexec_execute' event > > after kexec soft reboot implies missing events in that window which > > results in inconsistency with TPM PCR quotes, necessitating a cold boot > > for a successful remote attestation. > > As a reminder, please include directions for verifying the buffer data hash against > the buffer data. The directions would be similar to those in commit 6b4da8c0e7f > ("IMA: Define a new template field buf"). > > > > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > > Author: Tushar Sugandhi <tusharsu@linux.microsoft.com> > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > > Signed-off-by: steven chen <chenste@linux.microsoft.com> > > --- > > security/integrity/ima/ima_kexec.c | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/security/integrity/ima/ima_kexec.c > > b/security/integrity/ima/ima_kexec.c > > index c9c916f69ca7..0342ddfa9342 100644 > > --- a/security/integrity/ima/ima_kexec.c > > +++ b/security/integrity/ima/ima_kexec.c > > @@ -17,6 +17,8 @@ > > #include "ima.h" > > > > #ifdef CONFIG_IMA_KEXEC > > +#define IMA_KEXEC_EVENT_LEN 256 > > + > > static struct seq_file ima_kexec_file; > > static void *ima_kexec_buffer; > > static size_t kexec_segment_size; > > @@ -36,6 +38,24 @@ static void ima_free_kexec_file_buf(struct seq_file *sf) > > ima_reset_kexec_file(sf); > > } > > > > +static void ima_measure_kexec_event(const char *event_name) > > +{ > > + char ima_kexec_event[IMA_KEXEC_EVENT_LEN]; > > + size_t buf_size = 0; > > + long len; > > + > > + buf_size = ima_get_binary_runtime_size(); > > + len = atomic_long_read(&ima_htable.len); > > + > > + scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN, > > + "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;" > > + "ima_runtime_measurements_count=%ld;", > > + kexec_segment_size, buf_size, len); > > From scripts/checkpatch.pl, "Alignment should match open parenthesis". > > > + > > + ima_measure_critical_data("ima_kexec", event_name, ima_kexec_event, > > + strlen(ima_kexec_event), false, NULL, > > 0); > > From the kernel-doc scnprintf(), returns the number of bytes. There should be no > need to calculate it using strlen(). > > > +} > > + > > static int ima_alloc_kexec_file_buf(size_t segment_size) > > { > > /* > > @@ -60,6 +80,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size) > > out: > > ima_kexec_file.read_pos = 0; > > ima_kexec_file.count = sizeof(struct ima_kexec_hdr); /* reserved > > space > > */ > > + ima_measure_kexec_event("kexec_load"); > > > > return 0; > > } > > @@ -201,6 +222,8 @@ static int ima_update_kexec_buffer(struct notifier_block > > *self, > > return ret; > > } > > > > + ima_measure_kexec_event("kexec_execute"); > > + > > ret = ima_dump_measurement_list(&buf_size, &buf, > > kexec_segment_size); > > > > After fixing up and applying this patch set to 6.14.0-rc1, I'm not seeing the > "kexec_execute". Even after changing the default extra memory, I'm still not > seeing > the measurement. FYI, after reverting commit 254ef9541d68 ("ima: Suspend PCR extends and log appends when rebooting"), I'm seeing the "kexec_execute" measurement. Mimi
On 2/7/25 12:06 PM, Mimi Zohar wrote: > On Fri, 2025-02-07 at 10:16 -0500, Mimi Zohar wrote: >> On Mon, 2025-02-03 at 15:20 -0800, steven chen wrote: >>> The amount of memory allocated at kexec load, even with the extra memory >>> allocated, might not be large enough for the entire measurement list. The >>> indeterminate interval between kexec 'load' and 'execute' could exacerbate >>> this problem. >>> >>> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be >>> measured as critical data at kexec 'load' and 'execute' respectively. >>> Report the allocated kexec segment size, IMA binary log size and the >>> runtime measurements count as part of those events. >>> >>> These events, and the values reported through them, serve as markers in >>> the IMA log to verify the IMA events are captured during kexec soft >>> reboot. The presence of a 'kexec_load' event in between the last two >>> 'boot_aggregate' events in the IMA log implies this is a kexec soft >>> reboot, and not a cold-boot. And the absence of 'kexec_execute' event >>> after kexec soft reboot implies missing events in that window which >>> results in inconsistency with TPM PCR quotes, necessitating a cold boot >>> for a successful remote attestation. >> >> As a reminder, please include directions for verifying the buffer data hash against >> the buffer data. The directions would be similar to those in commit 6b4da8c0e7f >> ("IMA: Define a new template field buf"). >> >>> >>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> >>> Author: Tushar Sugandhi <tusharsu@linux.microsoft.com> >>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >>> Signed-off-by: steven chen <chenste@linux.microsoft.com> >>> --- >>> security/integrity/ima/ima_kexec.c | 23 +++++++++++++++++++++++ >>> 1 file changed, 23 insertions(+) >>> >>> diff --git a/security/integrity/ima/ima_kexec.c >>> b/security/integrity/ima/ima_kexec.c >>> index c9c916f69ca7..0342ddfa9342 100644 >>> --- a/security/integrity/ima/ima_kexec.c >>> +++ b/security/integrity/ima/ima_kexec.c >>> @@ -17,6 +17,8 @@ >>> #include "ima.h" >>> >>> #ifdef CONFIG_IMA_KEXEC >>> +#define IMA_KEXEC_EVENT_LEN 256 >>> + >>> static struct seq_file ima_kexec_file; >>> static void *ima_kexec_buffer; >>> static size_t kexec_segment_size; >>> @@ -36,6 +38,24 @@ static void ima_free_kexec_file_buf(struct seq_file *sf) >>> ima_reset_kexec_file(sf); >>> } >>> >>> +static void ima_measure_kexec_event(const char *event_name) >>> +{ >>> + char ima_kexec_event[IMA_KEXEC_EVENT_LEN]; >>> + size_t buf_size = 0; >>> + long len; >>> + >>> + buf_size = ima_get_binary_runtime_size(); >>> + len = atomic_long_read(&ima_htable.len); >>> + >>> + scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN, >>> + "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;" >>> + "ima_runtime_measurements_count=%ld;", >>> + kexec_segment_size, buf_size, len); >> >> From scripts/checkpatch.pl, "Alignment should match open parenthesis". >> >>> + >>> + ima_measure_critical_data("ima_kexec", event_name, ima_kexec_event, >>> + strlen(ima_kexec_event), false, NULL, >>> 0); >> >> From the kernel-doc scnprintf(), returns the number of bytes. There should be no >> need to calculate it using strlen(). >> >>> +} >>> + >>> static int ima_alloc_kexec_file_buf(size_t segment_size) >>> { >>> /* >>> @@ -60,6 +80,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size) >>> out: >>> ima_kexec_file.read_pos = 0; >>> ima_kexec_file.count = sizeof(struct ima_kexec_hdr); /* reserved >>> space >>> */ >>> + ima_measure_kexec_event("kexec_load"); >>> >>> return 0; >>> } >>> @@ -201,6 +222,8 @@ static int ima_update_kexec_buffer(struct notifier_block >>> *self, >>> return ret; >>> } >>> >>> + ima_measure_kexec_event("kexec_execute"); >>> + >>> ret = ima_dump_measurement_list(&buf_size, &buf, >>> kexec_segment_size); >>> >> >> After fixing up and applying this patch set to 6.14.0-rc1, I'm not seeing the >> "kexec_execute". Even after changing the default extra memory, I'm still not >> seeing >> the measurement. > > FYI, after reverting commit 254ef9541d68 ("ima: Suspend PCR extends and log appends > when rebooting"), I'm seeing the "kexec_execute" measurement. I would try sth. like this: static int ima_reboot_notifier(struct notifier_block *nb, unsigned long action, void *data) { if (action == SYS_RESTART && data && !strcmp(data, "kexec reboot")) ima_measure_kexec_event("kexec_execute"); > > Mimi > >
On 2/7/2025 9:48 AM, Stefan Berger wrote: > > > On 2/7/25 12:06 PM, Mimi Zohar wrote: >> On Fri, 2025-02-07 at 10:16 -0500, Mimi Zohar wrote: >>> On Mon, 2025-02-03 at 15:20 -0800, steven chen wrote: >>>> The amount of memory allocated at kexec load, even with the extra >>>> memory >>>> allocated, might not be large enough for the entire measurement >>>> list. The >>>> indeterminate interval between kexec 'load' and 'execute' could >>>> exacerbate >>>> this problem. >>>> >>>> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be >>>> measured as critical data at kexec 'load' and 'execute' respectively. >>>> Report the allocated kexec segment size, IMA binary log size and the >>>> runtime measurements count as part of those events. >>>> >>>> These events, and the values reported through them, serve as >>>> markers in >>>> the IMA log to verify the IMA events are captured during kexec soft >>>> reboot. The presence of a 'kexec_load' event in between the last two >>>> 'boot_aggregate' events in the IMA log implies this is a kexec soft >>>> reboot, and not a cold-boot. And the absence of 'kexec_execute' event >>>> after kexec soft reboot implies missing events in that window which >>>> results in inconsistency with TPM PCR quotes, necessitating a cold >>>> boot >>>> for a successful remote attestation. >>> >>> As a reminder, please include directions for verifying the buffer >>> data hash against >>> the buffer data. The directions would be similar to those in commit >>> 6b4da8c0e7f >>> ("IMA: Define a new template field buf"). >>> >>>> >>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> >>>> Author: Tushar Sugandhi <tusharsu@linux.microsoft.com> >>>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >>>> Signed-off-by: steven chen <chenste@linux.microsoft.com> >>>> --- >>>> security/integrity/ima/ima_kexec.c | 23 +++++++++++++++++++++++ >>>> 1 file changed, 23 insertions(+) >>>> >>>> diff --git a/security/integrity/ima/ima_kexec.c >>>> b/security/integrity/ima/ima_kexec.c >>>> index c9c916f69ca7..0342ddfa9342 100644 >>>> --- a/security/integrity/ima/ima_kexec.c >>>> +++ b/security/integrity/ima/ima_kexec.c >>>> @@ -17,6 +17,8 @@ >>>> #include "ima.h" >>>> #ifdef CONFIG_IMA_KEXEC >>>> +#define IMA_KEXEC_EVENT_LEN 256 >>>> + >>>> static struct seq_file ima_kexec_file; >>>> static void *ima_kexec_buffer; >>>> static size_t kexec_segment_size; >>>> @@ -36,6 +38,24 @@ static void ima_free_kexec_file_buf(struct >>>> seq_file *sf) >>>> ima_reset_kexec_file(sf); >>>> } >>>> +static void ima_measure_kexec_event(const char *event_name) >>>> +{ >>>> + char ima_kexec_event[IMA_KEXEC_EVENT_LEN]; >>>> + size_t buf_size = 0; >>>> + long len; >>>> + >>>> + buf_size = ima_get_binary_runtime_size(); >>>> + len = atomic_long_read(&ima_htable.len); >>>> + >>>> + scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN, >>>> + "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;" >>>> + "ima_runtime_measurements_count=%ld;", >>>> + kexec_segment_size, buf_size, len); >>> >>> From scripts/checkpatch.pl, "Alignment should match open parenthesis". >>> >>>> + >>>> + ima_measure_critical_data("ima_kexec", event_name, >>>> ima_kexec_event, >>>> + strlen(ima_kexec_event), false, NULL, >>>> 0); >>> >>> From the kernel-doc scnprintf(), returns the number of bytes. >>> There should be no >>> need to calculate it using strlen(). >>> >>>> +} >>>> + >>>> static int ima_alloc_kexec_file_buf(size_t segment_size) >>>> { >>>> /* >>>> @@ -60,6 +80,7 @@ static int ima_alloc_kexec_file_buf(size_t >>>> segment_size) >>>> out: >>>> ima_kexec_file.read_pos = 0; >>>> ima_kexec_file.count = sizeof(struct ima_kexec_hdr); /* >>>> reserved >>>> space >>>> */ >>>> + ima_measure_kexec_event("kexec_load"); >>>> return 0; >>>> } >>>> @@ -201,6 +222,8 @@ static int ima_update_kexec_buffer(struct >>>> notifier_block >>>> *self, >>>> return ret; >>>> } >>>> + ima_measure_kexec_event("kexec_execute"); >>>> + >>>> ret = ima_dump_measurement_list(&buf_size, &buf, >>>> kexec_segment_size); >>> >>> After fixing up and applying this patch set to 6.14.0-rc1, I'm not >>> seeing the >>> "kexec_execute". Even after changing the default extra memory, I'm >>> still not >>> seeing >>> the measurement. >> >> FYI, after reverting commit 254ef9541d68 ("ima: Suspend PCR extends >> and log appends >> when rebooting"), I'm seeing the "kexec_execute" measurement. > > I would try sth. like this: > static int ima_reboot_notifier(struct notifier_block *nb, > unsigned long action, > void *data) > { > if (action == SYS_RESTART && data && !strcmp(data, "kexec reboot")) > ima_measure_kexec_event("kexec_execute"); >> >> Mimi >> >> Thanks, will fix this in 6.14.0-rc1 in next version
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c index c9c916f69ca7..0342ddfa9342 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -17,6 +17,8 @@ #include "ima.h" #ifdef CONFIG_IMA_KEXEC +#define IMA_KEXEC_EVENT_LEN 256 + static struct seq_file ima_kexec_file; static void *ima_kexec_buffer; static size_t kexec_segment_size; @@ -36,6 +38,24 @@ static void ima_free_kexec_file_buf(struct seq_file *sf) ima_reset_kexec_file(sf); } +static void ima_measure_kexec_event(const char *event_name) +{ + char ima_kexec_event[IMA_KEXEC_EVENT_LEN]; + size_t buf_size = 0; + long len; + + buf_size = ima_get_binary_runtime_size(); + len = atomic_long_read(&ima_htable.len); + + scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN, + "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;" + "ima_runtime_measurements_count=%ld;", + kexec_segment_size, buf_size, len); + + ima_measure_critical_data("ima_kexec", event_name, ima_kexec_event, + strlen(ima_kexec_event), false, NULL, 0); +} + static int ima_alloc_kexec_file_buf(size_t segment_size) { /* @@ -60,6 +80,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size) out: ima_kexec_file.read_pos = 0; ima_kexec_file.count = sizeof(struct ima_kexec_hdr); /* reserved space */ + ima_measure_kexec_event("kexec_load"); return 0; } @@ -201,6 +222,8 @@ static int ima_update_kexec_buffer(struct notifier_block *self, return ret; } + ima_measure_kexec_event("kexec_execute"); + ret = ima_dump_measurement_list(&buf_size, &buf, kexec_segment_size);