| Message ID | 20250304190351.96975-2-chenste@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ima: kexec: measure events between kexec load and execute | expand |
On Tue, 2025-03-04 at 11:03 -0800, steven chen wrote: > > - Compared the memory size allocated with memory size of the entire > measurement record. Copy only complete measurement records if there > is enough memory. If there is not enough memory, it will not copy > any IMA measurement records, and this situation will result in a > failure of remote attestation. In discussions with Tushar, I was very clear that as many measurement records as possible should be carried over to the kexec'ed kernel. The main change between v8 and v9 was to make sure the last record copied was a complete record. Mimi
On Tue, 2025-03-04 at 21:08 -0500, Mimi Zohar wrote: > On Tue, 2025-03-04 at 11:03 -0800, steven chen wrote: > > > > - Compared the memory size allocated with memory size of the entire > > measurement record. Copy only complete measurement records if there > > is enough memory. If there is not enough memory, it will not copy > > any IMA measurement records, and this situation will result in a > > failure of remote attestation. > > In discussions with Tushar, I was very clear that as many measurement records as > possible should be carried over to the kexec'ed kernel. The main change between > v8 and v9 was to make sure the last record copied was a complete record. Steven, let me clarify my comment on v8. The patch description said, "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'." The intention is fine, but it also did other things: - only copied a full last measurement - if there wasn't enough room, it didn't copy any measurement records. Copying a full last measurement should be a separate, new patch to simplify review. I'm asking you to separate that change from the rest of the patch, so that it can be back ported independently of the rest of the patch set. When splitting the function "that allocates the buffer and copies the measurement records into separate functions", please make sure it still copies as many measurement records as possible. thanks, Mimi
On 03/04/25 at 11:03am, steven chen wrote: > 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'. > > This patch includes the following changes: I don't know why one patch need include so many changes. From below log, it should be split into separate patches. It may not need to make one patch to reflect one change, we should at least split and wrap several kind of changes to ease patch understanding and reviewing. My personal opinion. > - Refactor ima_dump_measurement_list() to move the memory allocation > to a separate function ima_alloc_kexec_file_buf() which allocates > buffer of size 'kexec_segment_size' at kexec 'load'. > - Make the local variable ima_kexec_file in ima_dump_measurement_list() > a local static to the file, so that it can be accessed from > ima_alloc_kexec_file_buf(). Compare actual memory required to ensure > there is enough memory for the entire measurement record. > - Copy only complete measurement records. > - Make necessary changes to the function ima_add_kexec_buffer() to call > the above two functions. > - Compared the memory size allocated with memory size of the entire > measurement record. Copy only complete measurement records if there > is enough memory. If there is not enough memory, it will not copy > any IMA measurement records, and this situation will result in a > failure of remote attestation. > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com> > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > Signed-off-by: steven chen <chenste@linux.microsoft.com> > --- > security/integrity/ima/ima.h | 1 + > security/integrity/ima/ima_kexec.c | 105 ++++++++++++++++++++++------- > security/integrity/ima/ima_queue.c | 4 +- > 3 files changed, 83 insertions(+), 27 deletions(-) > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index 24d09ea91b87..4428fcf42167 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -274,6 +274,7 @@ 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); > +int ima_get_binary_runtime_entry_size(struct ima_template_entry *entry); > 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 9d45f4d26f73..6195df128482 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -15,63 +15,106 @@ > #include "ima.h" > > #ifdef CONFIG_IMA_KEXEC > +static struct seq_file ima_kexec_file; > + > +static void ima_reset_kexec_file(struct seq_file *sf) > +{ > + sf->buf = NULL; > + sf->size = 0; > + sf->read_pos = 0; > + sf->count = 0; > +} > + > +static void ima_free_kexec_file_buf(struct seq_file *sf) > +{ > + vfree(sf->buf); > + ima_reset_kexec_file(sf); > +} > + > +static int ima_alloc_kexec_file_buf(size_t segment_size) > +{ > + /* > + * kexec 'load' may be called multiple times. > + * Free and realloc the buffer only if the segment_size is > + * changed from the previous kexec 'load' call. > + */ > + if (ima_kexec_file.buf && ima_kexec_file.size == segment_size) > + goto out; > + > + 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; > + > +out: > + ima_kexec_file.read_pos = 0; > + ima_kexec_file.count = sizeof(struct ima_kexec_hdr); /* reserved space */ > + > + return 0; > +} > + > +/* > + * Copy the measurement list to the allocated memory > + * compare the size of IMA measurement list with the size of the allocated memory > + * if the size of the allocated memory is not less than the size of IMA measurement list > + * copy the measurement list to the allocated memory. > + * else return error > + */ > static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, > unsigned long segment_size) > { > struct ima_queue_entry *qe; > - struct seq_file file; > struct ima_kexec_hdr khdr; > int ret = 0; > + size_t entry_size = 0; > > - /* segment size can't change between kexec load and execute */ > - file.buf = vmalloc(segment_size); > - if (!file.buf) { > - ret = -ENOMEM; > - goto out; > + if (!ima_kexec_file.buf) { > + pr_err("Kexec file buf not allocated\n"); > + return -EINVAL; > } > > - file.file = NULL; > - file.size = segment_size; > - file.read_pos = 0; > - 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 */ > list_for_each_entry_rcu(qe, &ima_measurements, later, true) { > - if (file.count < file.size) { > + entry_size += ima_get_binary_runtime_entry_size(qe->entry); > + if (entry_size <= segment_size) { > khdr.count++; > - ima_measurements_show(&file, qe); > + ima_measurements_show(&ima_kexec_file, qe); > } else { > ret = -EINVAL; > + pr_err("IMA log file is too big for Kexec buf\n"); > break; > } > } > > if (ret < 0) > goto out; > - > /* > * fill in reserved space with some buffer details > * (eg. version, buffer size, number of measurements) > */ > - khdr.buffer_size = file.count; > + khdr.buffer_size = ima_kexec_file.count; > if (ima_canonical_fmt) { > khdr.version = cpu_to_le16(khdr.version); > khdr.count = cpu_to_le64(khdr.count); > khdr.buffer_size = cpu_to_le64(khdr.buffer_size); > } > - memcpy(file.buf, &khdr, sizeof(khdr)); > + memcpy(ima_kexec_file.buf, &khdr, sizeof(khdr)); > > print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1, > - file.buf, file.count < 100 ? file.count : 100, > + ima_kexec_file.buf, ima_kexec_file.count < 100 ? > + ima_kexec_file.count : 100, > true); > > - *buffer_size = file.count; > - *buffer = file.buf; > + *buffer_size = ima_kexec_file.count; > + *buffer = ima_kexec_file.buf; > + > out: > - if (ret == -EINVAL) > - vfree(file.buf); > return ret; > } > > @@ -90,7 +133,7 @@ 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_buffer_size = 0; > size_t kexec_segment_size; > int ret; > > @@ -110,13 +153,19 @@ void ima_add_kexec_buffer(struct kimage *image) > return; > } > > - ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, > - kexec_segment_size); > - if (!kexec_buffer) { > + ret = ima_alloc_kexec_file_buf(kexec_segment_size); > + if (ret < 0) { > pr_err("Not enough memory for the kexec measurement buffer.\n"); > return; > } > > + ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, > + kexec_segment_size); > + if (ret < 0) { > + pr_err("Failed to dump IMA measurements. Error:%d.\n", ret); > + return; > + } > + > kbuf.buffer = kexec_buffer; > kbuf.bufsz = kexec_buffer_size; > kbuf.memsz = kexec_segment_size; > @@ -131,6 +180,12 @@ void ima_add_kexec_buffer(struct kimage *image) > image->ima_buffer_size = kexec_segment_size; > image->ima_buffer = kexec_buffer; > > + /* > + * kexec owns kexec_buffer after kexec_add_buffer() is called > + * and it will vfree() that buffer. > + */ > + ima_reset_kexec_file(&ima_kexec_file); > + > kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n", > kbuf.mem); > } > diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c > index 83d53824aa98..3dfd178d4292 100644 > --- a/security/integrity/ima/ima_queue.c > +++ b/security/integrity/ima/ima_queue.c > @@ -78,7 +78,7 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value, > * binary_runtime_measurement list entry, which contains a > * couple of variable length fields (e.g template name and data). > */ > -static int get_binary_runtime_size(struct ima_template_entry *entry) > +int ima_get_binary_runtime_entry_size(struct ima_template_entry *entry) > { > int size = 0; > > @@ -122,7 +122,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry, > if (binary_runtime_size != ULONG_MAX) { > int size; > > - size = get_binary_runtime_size(entry); > + size = ima_get_binary_runtime_entry_size(entry); > binary_runtime_size = (binary_runtime_size < ULONG_MAX - size) ? > binary_runtime_size + size : ULONG_MAX; > } > -- > 2.25.1 >
On Wed, 2025-03-05 at 20:08 +0800, Baoquan He wrote: > On 03/04/25 at 11:03am, steven chen wrote: > > 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'. > > > > This patch includes the following changes: > > I don't know why one patch need include so many changes. From below log, > it should be split into separate patches. It may not need to make one > patch to reflect one change, we should at least split and wrap several > kind of changes to ease patch understanding and reviewing. My personal > opinion. Agreed, well explained. Mimi > > > - Refactor ima_dump_measurement_list() to move the memory allocation > > to a separate function ima_alloc_kexec_file_buf() which allocates > > buffer of size 'kexec_segment_size' at kexec 'load'. > > - Make the local variable ima_kexec_file in ima_dump_measurement_list() > > a local static to the file, so that it can be accessed from > > ima_alloc_kexec_file_buf(). Compare actual memory required to ensure > > there is enough memory for the entire measurement record. > > - Copy only complete measurement records. > > - Make necessary changes to the function ima_add_kexec_buffer() to call > > the above two functions. > > - Compared the memory size allocated with memory size of the entire > > measurement record. Copy only complete measurement records if there > > is enough memory. If there is not enough memory, it will not copy > > any IMA measurement records, and this situation will result in a > > failure of remote attestation. > > > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com> > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > > Signed-off-by: steven chen <chenste@linux.microsoft.com>
On 3/5/2025 4:27 AM, Mimi Zohar wrote: > On Wed, 2025-03-05 at 20:08 +0800, Baoquan He wrote: >> On 03/04/25 at 11:03am, steven chen wrote: >>> 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'. >>> >>> This patch includes the following changes: >> I don't know why one patch need include so many changes. From below log, >> it should be split into separate patches. It may not need to make one >> patch to reflect one change, we should at least split and wrap several >> kind of changes to ease patch understanding and reviewing. My personal >> opinion. > Agreed, well explained. > > Mimi > >>> - Refactor ima_dump_measurement_list() to move the memory allocation >>> to a separate function ima_alloc_kexec_file_buf() which allocates >>> buffer of size 'kexec_segment_size' at kexec 'load'. >>> - Make the local variable ima_kexec_file in ima_dump_measurement_list() >>> a local static to the file, so that it can be accessed from >>> ima_alloc_kexec_file_buf(). Compare actual memory required to ensure >>> there is enough memory for the entire measurement record. >>> - Copy only complete measurement records. >>> - Make necessary changes to the function ima_add_kexec_buffer() to call >>> the above two functions. >>> - Compared the memory size allocated with memory size of the entire >>> measurement record. Copy only complete measurement records if there >>> is enough memory. If there is not enough memory, it will not copy >>> any IMA measurement records, and this situation will result in a >>> failure of remote attestation. >>> >>> Suggested-by: Mimi Zohar <zohar@linux.ibm.com> >>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >>> Signed-off-by: steven chen <chenste@linux.microsoft.com> I will split this patch into the following two patches: ima: define and call ima_alloc_kexec_file_buf ima: copy measurement records as much as possible across kexec Thanks, Steven
On Thu, 2025-03-06 at 14:45 -0800, steven chen wrote: > On 3/5/2025 4:27 AM, Mimi Zohar wrote: > > On Wed, 2025-03-05 at 20:08 +0800, Baoquan He wrote: > > > On 03/04/25 at 11:03am, steven chen wrote: > > > > 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'. > > > > > > > > This patch includes the following changes: > > > I don't know why one patch need include so many changes. From below log, > > > it should be split into separate patches. It may not need to make one > > > patch to reflect one change, we should at least split and wrap several > > > kind of changes to ease patch understanding and reviewing. My personal > > > opinion. > > Agreed, well explained. > > > > Mimi > > > > > > - Refactor ima_dump_measurement_list() to move the memory allocation > > > > to a separate function ima_alloc_kexec_file_buf() which allocates > > > > buffer of size 'kexec_segment_size' at kexec 'load'. > > > > - Make the local variable ima_kexec_file in ima_dump_measurement_list() > > > > a local static to the file, so that it can be accessed from > > > > ima_alloc_kexec_file_buf(). Compare actual memory required to ensure > > > > there is enough memory for the entire measurement record. > > > > - Copy only complete measurement records. > > > > - Make necessary changes to the function ima_add_kexec_buffer() to call > > > > the above two functions. > > > > - Compared the memory size allocated with memory size of the entire > > > > measurement record. Copy only complete measurement records if there > > > > is enough memory. If there is not enough memory, it will not copy > > > > any IMA measurement records, and this situation will result in a > > > > failure of remote attestation. > > > > > > > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com> > > > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > > > > Signed-off-by: steven chen <chenste@linux.microsoft.com> > > I will split this patch into the following two patches: > > ima: define and call ima_alloc_kexec_file_buf > ima: copy measurement records as much as possible across kexec Steven, breaking up code into patches is in order to simplify patch review. This is done by limiting each patch to a single "logical change" [1]. For example, the change below has nothing to do with "separate allocating the buffer and copying the measurement records into separate functions". /* This is an append-only list, no need to hold the RCU read lock */ list_for_each_entry_rcu(qe, &ima_measurements, later, true) { - if (file.count < file.size) { + entry_size += ima_get_binary_runtime_entry_size(qe->entry); + if (entry_size <= segment_size) { khdr.count++; - ima_measurements_show(&file, qe); + ima_measurements_show(&ima_kexec_file, qe); } else { ret = -EINVAL; + pr_err("IMA log file is too big for Kexec buf\n"); break; } } The original code potentially copied a partial last measurement record, not a complete measurement record. For ease of review, the above change is fine, but it needs to be a separate patch. Patches: 1. ima: copy only complete measurement records across kexec 2. ima: define and call ima_alloc_kexec_file_buf() The original code copied as many measurement records as possible. Please do not change it. thanks, Mimi [1] Refer to the section "Separate your changes" in https://www.kernel.org/doc/Documentation/process/submitting-patches.rst
On Thu, 2025-03-06 at 21:51 -0500, Mimi Zohar wrote: > On Thu, 2025-03-06 at 14:45 -0800, steven chen wrote: > > On 3/5/2025 4:27 AM, Mimi Zohar wrote: > > > On Wed, 2025-03-05 at 20:08 +0800, Baoquan He wrote: > > > > On 03/04/25 at 11:03am, steven chen wrote: > > > > > 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'. > > > > > > > > > > This patch includes the following changes: > > > > I don't know why one patch need include so many changes. From below log, > > > > it should be split into separate patches. It may not need to make one > > > > patch to reflect one change, we should at least split and wrap several > > > > kind of changes to ease patch understanding and reviewing. My personal > > > > opinion. > > > Agreed, well explained. > > > > > > Mimi > > > > > > > > - Refactor ima_dump_measurement_list() to move the memory allocation > > > > > to a separate function ima_alloc_kexec_file_buf() which allocates > > > > > buffer of size 'kexec_segment_size' at kexec 'load'. > > > > > - Make the local variable ima_kexec_file in ima_dump_measurement_list() > > > > > a local static to the file, so that it can be accessed from > > > > > ima_alloc_kexec_file_buf(). Compare actual memory required to ensure > > > > > there is enough memory for the entire measurement record. > > > > > - Copy only complete measurement records. > > > > > - Make necessary changes to the function ima_add_kexec_buffer() to call > > > > > the above two functions. > > > > > - Compared the memory size allocated with memory size of the entire > > > > > measurement record. Copy only complete measurement records if there > > > > > is enough memory. If there is not enough memory, it will not copy > > > > > any IMA measurement records, and this situation will result in a > > > > > failure of remote attestation. > > > > > > > > > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com> > > > > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > > > > > Signed-off-by: steven chen <chenste@linux.microsoft.com> > > > > I will split this patch into the following two patches: > > > > ima: define and call ima_alloc_kexec_file_buf > > ima: copy measurement records as much as possible across kexec > > Steven, breaking up code into patches is in order to simplify patch review. > This is done by limiting each patch to a single "logical change" [1]. For > example, the change below has nothing to do with "separate allocating the buffer > and copying the measurement records into separate functions". > > /* This is an append-only list, no need to hold the RCU read lock */ > list_for_each_entry_rcu(qe, &ima_measurements, later, true) { > - if (file.count < file.size) { > + entry_size += ima_get_binary_runtime_entry_size(qe->entry); > + if (entry_size <= segment_size) { > khdr.count++; > - ima_measurements_show(&file, qe); > + ima_measurements_show(&ima_kexec_file, qe); > } else { > ret = -EINVAL; > + pr_err("IMA log file is too big for Kexec buf\n"); > break; > } > } > > The original code potentially copied a partial last measurement record, not a > complete measurement record. For ease of review, the above change is fine, but > it needs to be a separate patch. > > Patches: > 1. ima: copy only complete measurement records across kexec > 2. ima: define and call ima_alloc_kexec_file_buf() Steven, The alternative would be to revert using ima_get_binary_runtime_entry_size() and simply use "ima_kexec_file.count < ima_kexec_file.size". Only ima_kexec_file.size would be initialized in ima_alloc_kexec_buf(). The rest would remain in ima_dump_measurement_list(). get_binary_runtime_size() wouldn't need to be made global. To further simplify the patch review, first define a separate patch to just rename the seq_file "file" to "ima_kexec_file". Mimi
On 3/11/2025 5:44 AM, Mimi Zohar wrote: > On Thu, 2025-03-06 at 21:51 -0500, Mimi Zohar wrote: >> On Thu, 2025-03-06 at 14:45 -0800, steven chen wrote: >>> On 3/5/2025 4:27 AM, Mimi Zohar wrote: >>>> On Wed, 2025-03-05 at 20:08 +0800, Baoquan He wrote: >>>>> On 03/04/25 at 11:03am, steven chen wrote: >>>>>> 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'. >>>>>> >>>>>> This patch includes the following changes: >>>>> I don't know why one patch need include so many changes. From below log, >>>>> it should be split into separate patches. It may not need to make one >>>>> patch to reflect one change, we should at least split and wrap several >>>>> kind of changes to ease patch understanding and reviewing. My personal >>>>> opinion. >>>> Agreed, well explained. >>>> >>>> Mimi >>>> >>>>>> - Refactor ima_dump_measurement_list() to move the memory allocation >>>>>> to a separate function ima_alloc_kexec_file_buf() which allocates >>>>>> buffer of size 'kexec_segment_size' at kexec 'load'. >>>>>> - Make the local variable ima_kexec_file in ima_dump_measurement_list() >>>>>> a local static to the file, so that it can be accessed from >>>>>> ima_alloc_kexec_file_buf(). Compare actual memory required to ensure >>>>>> there is enough memory for the entire measurement record. >>>>>> - Copy only complete measurement records. >>>>>> - Make necessary changes to the function ima_add_kexec_buffer() to call >>>>>> the above two functions. >>>>>> - Compared the memory size allocated with memory size of the entire >>>>>> measurement record. Copy only complete measurement records if there >>>>>> is enough memory. If there is not enough memory, it will not copy >>>>>> any IMA measurement records, and this situation will result in a >>>>>> failure of remote attestation. >>>>>> >>>>>> Suggested-by: Mimi Zohar <zohar@linux.ibm.com> >>>>>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >>>>>> Signed-off-by: steven chen <chenste@linux.microsoft.com> >>> I will split this patch into the following two patches: >>> >>> ima: define and call ima_alloc_kexec_file_buf >>> ima: copy measurement records as much as possible across kexec >> Steven, breaking up code into patches is in order to simplify patch review. >> This is done by limiting each patch to a single "logical change" [1]. For >> example, the change below has nothing to do with "separate allocating the buffer >> and copying the measurement records into separate functions". >> >> /* This is an append-only list, no need to hold the RCU read lock */ >> list_for_each_entry_rcu(qe, &ima_measurements, later, true) { >> - if (file.count < file.size) { >> + entry_size += ima_get_binary_runtime_entry_size(qe->entry); >> + if (entry_size <= segment_size) { >> khdr.count++; >> - ima_measurements_show(&file, qe); >> + ima_measurements_show(&ima_kexec_file, qe); >> } else { >> ret = -EINVAL; >> + pr_err("IMA log file is too big for Kexec buf\n"); >> break; >> } >> } >> >> The original code potentially copied a partial last measurement record, not a >> complete measurement record. For ease of review, the above change is fine, but >> it needs to be a separate patch. >> >> Patches: >> 1. ima: copy only complete measurement records across kexec >> 2. ima: define and call ima_alloc_kexec_file_buf() > Steven, > > The alternative would be to revert using ima_get_binary_runtime_entry_size() and > simply use "ima_kexec_file.count < ima_kexec_file.size". Only > ima_kexec_file.size would be initialized in ima_alloc_kexec_buf(). The rest > would remain in ima_dump_measurement_list(). get_binary_runtime_size() wouldn't > need to be made global. > > To further simplify the patch review, first define a separate patch to just > rename the seq_file "file" to "ima_kexec_file". > > Mimi Hi Mimi, I will work on it. Thanks, Steven
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 24d09ea91b87..4428fcf42167 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -274,6 +274,7 @@ 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); +int ima_get_binary_runtime_entry_size(struct ima_template_entry *entry); 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 9d45f4d26f73..6195df128482 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -15,63 +15,106 @@ #include "ima.h" #ifdef CONFIG_IMA_KEXEC +static struct seq_file ima_kexec_file; + +static void ima_reset_kexec_file(struct seq_file *sf) +{ + sf->buf = NULL; + sf->size = 0; + sf->read_pos = 0; + sf->count = 0; +} + +static void ima_free_kexec_file_buf(struct seq_file *sf) +{ + vfree(sf->buf); + ima_reset_kexec_file(sf); +} + +static int ima_alloc_kexec_file_buf(size_t segment_size) +{ + /* + * kexec 'load' may be called multiple times. + * Free and realloc the buffer only if the segment_size is + * changed from the previous kexec 'load' call. + */ + if (ima_kexec_file.buf && ima_kexec_file.size == segment_size) + goto out; + + 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; + +out: + ima_kexec_file.read_pos = 0; + ima_kexec_file.count = sizeof(struct ima_kexec_hdr); /* reserved space */ + + return 0; +} + +/* + * Copy the measurement list to the allocated memory + * compare the size of IMA measurement list with the size of the allocated memory + * if the size of the allocated memory is not less than the size of IMA measurement list + * copy the measurement list to the allocated memory. + * else return error + */ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, unsigned long segment_size) { struct ima_queue_entry *qe; - struct seq_file file; struct ima_kexec_hdr khdr; int ret = 0; + size_t entry_size = 0; - /* segment size can't change between kexec load and execute */ - file.buf = vmalloc(segment_size); - if (!file.buf) { - ret = -ENOMEM; - goto out; + if (!ima_kexec_file.buf) { + pr_err("Kexec file buf not allocated\n"); + return -EINVAL; } - file.file = NULL; - file.size = segment_size; - file.read_pos = 0; - 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 */ list_for_each_entry_rcu(qe, &ima_measurements, later, true) { - if (file.count < file.size) { + entry_size += ima_get_binary_runtime_entry_size(qe->entry); + if (entry_size <= segment_size) { khdr.count++; - ima_measurements_show(&file, qe); + ima_measurements_show(&ima_kexec_file, qe); } else { ret = -EINVAL; + pr_err("IMA log file is too big for Kexec buf\n"); break; } } if (ret < 0) goto out; - /* * fill in reserved space with some buffer details * (eg. version, buffer size, number of measurements) */ - khdr.buffer_size = file.count; + khdr.buffer_size = ima_kexec_file.count; if (ima_canonical_fmt) { khdr.version = cpu_to_le16(khdr.version); khdr.count = cpu_to_le64(khdr.count); khdr.buffer_size = cpu_to_le64(khdr.buffer_size); } - memcpy(file.buf, &khdr, sizeof(khdr)); + memcpy(ima_kexec_file.buf, &khdr, sizeof(khdr)); print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1, - file.buf, file.count < 100 ? file.count : 100, + ima_kexec_file.buf, ima_kexec_file.count < 100 ? + ima_kexec_file.count : 100, true); - *buffer_size = file.count; - *buffer = file.buf; + *buffer_size = ima_kexec_file.count; + *buffer = ima_kexec_file.buf; + out: - if (ret == -EINVAL) - vfree(file.buf); return ret; } @@ -90,7 +133,7 @@ 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_buffer_size = 0; size_t kexec_segment_size; int ret; @@ -110,13 +153,19 @@ void ima_add_kexec_buffer(struct kimage *image) return; } - ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, - kexec_segment_size); - if (!kexec_buffer) { + ret = ima_alloc_kexec_file_buf(kexec_segment_size); + if (ret < 0) { pr_err("Not enough memory for the kexec measurement buffer.\n"); return; } + ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, + kexec_segment_size); + if (ret < 0) { + pr_err("Failed to dump IMA measurements. Error:%d.\n", ret); + return; + } + kbuf.buffer = kexec_buffer; kbuf.bufsz = kexec_buffer_size; kbuf.memsz = kexec_segment_size; @@ -131,6 +180,12 @@ void ima_add_kexec_buffer(struct kimage *image) image->ima_buffer_size = kexec_segment_size; image->ima_buffer = kexec_buffer; + /* + * kexec owns kexec_buffer after kexec_add_buffer() is called + * and it will vfree() that buffer. + */ + ima_reset_kexec_file(&ima_kexec_file); + kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n", kbuf.mem); } diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index 83d53824aa98..3dfd178d4292 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -78,7 +78,7 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value, * binary_runtime_measurement list entry, which contains a * couple of variable length fields (e.g template name and data). */ -static int get_binary_runtime_size(struct ima_template_entry *entry) +int ima_get_binary_runtime_entry_size(struct ima_template_entry *entry) { int size = 0; @@ -122,7 +122,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry, if (binary_runtime_size != ULONG_MAX) { int size; - size = get_binary_runtime_size(entry); + size = ima_get_binary_runtime_entry_size(entry); binary_runtime_size = (binary_runtime_size < ULONG_MAX - size) ? binary_runtime_size + size : ULONG_MAX; }