| Message ID | 20250416021028.1403-3-chenste@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: kexec: measure events between kexec load and execute | expand |
Hi Steven, On 04/15/25 at 07:10pm, steven chen wrote: > From: Steven Chen <chenste@linux.microsoft.com> > > In the current implementation, the ima_dump_measurement_list() API is > called during the kexec "load" phase, where a buffer is allocated and > the measurement records are copied. Due to this, new events added after > kexec load but before kexec execute are not carried over to the new kernel > during kexec operation > > Carrying the IMA measurement list across kexec requires allocating a > buffer and copying the measurement records. Separate allocating the > buffer and copying the measurement records into separate functions in > order to allocate the buffer at kexec 'load' and copy the measurements > at kexec 'execute'. Seems you didn't add note in this patch log to mention that the IMA measurement list fails to verify when doing two consecuritv "kexec -s -l" with/without a "kexec -s -u" in between. When people do bisecting and come to this patch, she/he will see the failure even though it's not a fatal blocker. ==== After moving the vfree() here at this stage in the patch set, the IMA measurement list fails to verify when doing two consecutive "kexec -s -l" with/without a "kexec -s -u" in between. Only after "ima: kexec: move IMA log copy from kexec load to execute" the IMA measurement list verifies properly with the vfree() here. ==== > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > Signed-off-by: Steven Chen <chenste@linux.microsoft.com> > --- > security/integrity/ima/ima_kexec.c | 46 +++++++++++++++++++++++------- > 1 file changed, 35 insertions(+), 11 deletions(-) > > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c > index 650beb74346c..b12ac3619b8f 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -15,26 +15,46 @@ > #include "ima.h" > > #ifdef CONFIG_IMA_KEXEC > +static struct seq_file ima_kexec_file; > + > +static void ima_free_kexec_file_buf(struct seq_file *sf) > +{ > + vfree(sf->buf); > + sf->buf = NULL; > + sf->size = 0; > + sf->read_pos = 0; > + sf->count = 0; > +} > + > +static int ima_alloc_kexec_file_buf(size_t segment_size) > +{ > + ima_free_kexec_file_buf(&ima_kexec_file); > + > + /* segment size can't change between kexec load and execute */ > + ima_kexec_file.buf = vmalloc(segment_size); > + if (!ima_kexec_file.buf) > + return -ENOMEM; > + > + ima_kexec_file.size = segment_size; > + ima_kexec_file.read_pos = 0; > + ima_kexec_file.count = sizeof(struct ima_kexec_hdr); /* reserved space */ > + > + return 0; > +} > + > static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, > unsigned long segment_size) > { > - struct seq_file ima_kexec_file; > struct ima_queue_entry *qe; > struct ima_kexec_hdr khdr; > int ret = 0; > > /* segment size can't change between kexec load and execute */ > - ima_kexec_file.buf = vmalloc(segment_size); > if (!ima_kexec_file.buf) { > - ret = -ENOMEM; > - goto out; > + pr_err("Kexec file buf not allocated\n"); > + return -EINVAL; > } > > - ima_kexec_file.file = NULL; > - ima_kexec_file.size = segment_size; > - ima_kexec_file.read_pos = 0; > - ima_kexec_file.count = sizeof(khdr); /* reserved space */ > - > memset(&khdr, 0, sizeof(khdr)); > khdr.version = 1; > /* This is an append-only list, no need to hold the RCU read lock */ > @@ -71,8 +91,6 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, > *buffer_size = ima_kexec_file.count; > *buffer = ima_kexec_file.buf; > out: > - if (ret == -EINVAL) > - vfree(ima_kexec_file.buf); > return ret; > } > > @@ -111,6 +129,12 @@ void ima_add_kexec_buffer(struct kimage *image) > return; > } > > + ret = ima_alloc_kexec_file_buf(kexec_segment_size); > + if (ret < 0) { > + pr_err("Not enough memory for the kexec measurement buffer.\n"); > + return; > + } > + > ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, > kexec_segment_size); > if (!kexec_buffer) { > -- > 2.43.0 >
On 4/17/2025 9:33 PM, Baoquan He wrote: > Hi Steven, > > On 04/15/25 at 07:10pm, steven chen wrote: >> From: Steven Chen <chenste@linux.microsoft.com> >> >> In the current implementation, the ima_dump_measurement_list() API is >> called during the kexec "load" phase, where a buffer is allocated and >> the measurement records are copied. Due to this, new events added after >> kexec load but before kexec execute are not carried over to the new kernel >> during kexec operation >> >> Carrying the IMA measurement list across kexec requires allocating a >> buffer and copying the measurement records. Separate allocating the >> buffer and copying the measurement records into separate functions in >> order to allocate the buffer at kexec 'load' and copy the measurements >> at kexec 'execute'. > Seems you didn't add note in this patch log to mention that the IMA > measurement list fails to verify when doing two consecuritv "kexec -s -l" > with/without a "kexec -s -u" in between. When people do bisecting and > come to this patch, she/he will see the failure even though it's not a > fatal blocker. > > ==== > After moving the vfree() here at this stage in the patch set, the IMA > measurement list fails to verify when doing two consecutive "kexec -s -l" > with/without a "kexec -s -u" in between. Only after "ima: kexec: move IMA log > copy from kexec load to execute" the IMA measurement list verifies properly with > the vfree() here. > ==== > Hi Baoquan, Thanks for your comments. I will add this in next version. Steven >> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >> Signed-off-by: Steven Chen <chenste@linux.microsoft.com> >> --- >> security/integrity/ima/ima_kexec.c | 46 +++++++++++++++++++++++------- >> 1 file changed, 35 insertions(+), 11 deletions(-) >> >> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c >> index 650beb74346c..b12ac3619b8f 100644 >> --- a/security/integrity/ima/ima_kexec.c >> +++ b/security/integrity/ima/ima_kexec.c >> @@ -15,26 +15,46 @@ >> #include "ima.h" >> >> #ifdef CONFIG_IMA_KEXEC >> +static struct seq_file ima_kexec_file; >> + >> +static void ima_free_kexec_file_buf(struct seq_file *sf) >> +{ >> + vfree(sf->buf); >> + sf->buf = NULL; >> + sf->size = 0; >> + sf->read_pos = 0; >> + sf->count = 0; >> +} >> + >> +static int ima_alloc_kexec_file_buf(size_t segment_size) >> +{ >> + ima_free_kexec_file_buf(&ima_kexec_file); >> + >> + /* segment size can't change between kexec load and execute */ >> + ima_kexec_file.buf = vmalloc(segment_size); >> + if (!ima_kexec_file.buf) >> + return -ENOMEM; >> + >> + ima_kexec_file.size = segment_size; >> + ima_kexec_file.read_pos = 0; >> + ima_kexec_file.count = sizeof(struct ima_kexec_hdr); /* reserved space */ >> + >> + return 0; >> +} >> + >> static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, >> unsigned long segment_size) >> { >> - struct seq_file ima_kexec_file; >> struct ima_queue_entry *qe; >> struct ima_kexec_hdr khdr; >> int ret = 0; >> >> /* segment size can't change between kexec load and execute */ >> - ima_kexec_file.buf = vmalloc(segment_size); >> if (!ima_kexec_file.buf) { >> - ret = -ENOMEM; >> - goto out; >> + pr_err("Kexec file buf not allocated\n"); >> + return -EINVAL; >> } >> >> - ima_kexec_file.file = NULL; >> - ima_kexec_file.size = segment_size; >> - ima_kexec_file.read_pos = 0; >> - ima_kexec_file.count = sizeof(khdr); /* reserved space */ >> - >> memset(&khdr, 0, sizeof(khdr)); >> khdr.version = 1; >> /* This is an append-only list, no need to hold the RCU read lock */ >> @@ -71,8 +91,6 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, >> *buffer_size = ima_kexec_file.count; >> *buffer = ima_kexec_file.buf; >> out: >> - if (ret == -EINVAL) >> - vfree(ima_kexec_file.buf); >> return ret; >> } >> >> @@ -111,6 +129,12 @@ void ima_add_kexec_buffer(struct kimage *image) >> return; >> } >> >> + ret = ima_alloc_kexec_file_buf(kexec_segment_size); >> + if (ret < 0) { >> + pr_err("Not enough memory for the kexec measurement buffer.\n"); >> + return; >> + } >> + >> ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, >> kexec_segment_size); >> if (!kexec_buffer) { >> -- >> 2.43.0 >>
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c index 650beb74346c..b12ac3619b8f 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -15,26 +15,46 @@ #include "ima.h" #ifdef CONFIG_IMA_KEXEC +static struct seq_file ima_kexec_file; + +static void ima_free_kexec_file_buf(struct seq_file *sf) +{ + vfree(sf->buf); + sf->buf = NULL; + sf->size = 0; + sf->read_pos = 0; + sf->count = 0; +} + +static int ima_alloc_kexec_file_buf(size_t segment_size) +{ + ima_free_kexec_file_buf(&ima_kexec_file); + + /* segment size can't change between kexec load and execute */ + ima_kexec_file.buf = vmalloc(segment_size); + if (!ima_kexec_file.buf) + return -ENOMEM; + + ima_kexec_file.size = segment_size; + ima_kexec_file.read_pos = 0; + ima_kexec_file.count = sizeof(struct ima_kexec_hdr); /* reserved space */ + + return 0; +} + static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, unsigned long segment_size) { - struct seq_file ima_kexec_file; struct ima_queue_entry *qe; struct ima_kexec_hdr khdr; int ret = 0; /* segment size can't change between kexec load and execute */ - ima_kexec_file.buf = vmalloc(segment_size); if (!ima_kexec_file.buf) { - ret = -ENOMEM; - goto out; + pr_err("Kexec file buf not allocated\n"); + return -EINVAL; } - ima_kexec_file.file = NULL; - ima_kexec_file.size = segment_size; - ima_kexec_file.read_pos = 0; - ima_kexec_file.count = sizeof(khdr); /* reserved space */ - memset(&khdr, 0, sizeof(khdr)); khdr.version = 1; /* This is an append-only list, no need to hold the RCU read lock */ @@ -71,8 +91,6 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, *buffer_size = ima_kexec_file.count; *buffer = ima_kexec_file.buf; out: - if (ret == -EINVAL) - vfree(ima_kexec_file.buf); return ret; } @@ -111,6 +129,12 @@ void ima_add_kexec_buffer(struct kimage *image) return; } + ret = ima_alloc_kexec_file_buf(kexec_segment_size); + if (ret < 0) { + pr_err("Not enough memory for the kexec measurement buffer.\n"); + return; + } + ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, kexec_segment_size); if (!kexec_buffer) {