From: Mimi Zohar <zohar@linux.ibm.com> To: Tushar Sugandhi <tusharsu@linux.microsoft.com>, roberto.sassu@huaweicloud.com, roberto.sassu@huawei.com, eric.snowberg@oracle.com, stefanb@linux.ibm.com, ebiederm@xmission.com, noodles@fb.com, bauermann@kolabnow.com, linux-integrity@vger.kernel.org, kexec@lists.infradead.org Cc: code@tyhicks.com, nramas@linux.microsoft.com, paul@paul-moore.com Subject: Re: [PATCH v5 1/8] ima: define and call ima_alloc_kexec_file_buf Date: Tue, 20 Feb 2024 19:12:41 -0500 [thread overview] Message-ID: <844e1af9a5d0414d4b972c8c947b7f821a776d71.camel@linux.ibm.com> (raw) In-Reply-To: <20240214153827.1087657-2-tusharsu@linux.microsoft.com> On Wed, 2024-02-14 at 07:38 -0800, Tushar Sugandhi 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: > - 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() > as local static to the file, so that it can be accessed from > ima_alloc_kexec_file_buf(). > - Make necessary changes to the function ima_add_kexec_buffer() to call > the above two functions. > > Suggested-by: Stefan Berger <stefanb@linux.ibm.com> > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > --- > security/integrity/ima/ima_kexec.c | 107 +++++++++++++++++++++-------- > 1 file changed, 78 insertions(+), 29 deletions(-) > > diff --git a/security/integrity/ima/ima_kexec.c > b/security/integrity/ima/ima_kexec.c > index dadc1d138118..a9cb5e882e2e 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -15,62 +15,98 @@ > #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 && > + ima_kexec_file.read_pos == 0 && > + ima_kexec_file.count == sizeof(struct ima_kexec_hdr)) > + return 0; ima_kexec_file.size will be initialized to zero. No need to test anything other than the segment size. > + > + ima_free_kexec_file_buf(&ima_kexec_file); The first time ima_free_kexec_file_buf() is called the memory has not been allocated. Test that the memory has been allocated before freeing it. Since there's no other ima_free_kexec_file_buf() caller, there's no need to clear the other fields as they will be set below. > + > + /* 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; static variables are initialized to zero. No need to set it here. > + 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 ima_queue_entry *qe; > - struct seq_file file; > struct ima_kexec_hdr khdr; > - int ret = 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) { > + *buffer_size = 0; > + *buffer = NULL; > + pr_err("%s: Kexec file buf not allocated\n", __func__); > + return -EINVAL; > } > > - file.size = segment_size; > - file.read_pos = 0; > - file.count = sizeof(khdr); /* reserved space */ > - > memset(&khdr, 0, sizeof(khdr)); > khdr.version = 1; > + > + /* Copy as many IMA measurements list records as possible */ > list_for_each_entry_rcu(qe, &ima_measurements, later) { > - if (file.count < file.size) { > + if (ima_kexec_file.count < ima_kexec_file.size) { With an extra half page memory allocated, there should be enough memory for a few extra records as a result of the kexec load. Subsequent patches are going to change this assumption. This test doesn't ensure there is enough memory for the entire measurement record. Call get_binary_runtime_size() to get the record size to ensure there is enough memory for the entire record. > khdr.count++; > - ima_measurements_show(&file, qe); > + ima_measurements_show(&ima_kexec_file, qe); > } else { > - ret = -EINVAL; > + pr_err("%s: IMA log file is too big for Kexec buf\n", > + __func__); This message and the one above should probably be pr_debug(). There's a new error message after the call to ima_dump_measurement_list(). > break; > } > } > > - if (ret < 0) > - goto out; > - Removing these lines is in for "copying as many measurements as possible". That change isn't mentioned in the patch description. > /* > * 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; > -out: > - if (ret == -EINVAL) > - vfree(file.buf); > - return ret; > + *buffer_size = ima_kexec_file.count; > + *buffer = ima_kexec_file.buf; > + > + return 0; > } > > /* > @@ -108,13 +144,20 @@ 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("%s: Failed to dump IMA measurements. Error:%d.\n", > + __func__, ret); Please remove "__func__" from the error message. > + return; > + } > + > kbuf.buffer = kexec_buffer; > kbuf.bufsz = kexec_buffer_size; > kbuf.memsz = kexec_segment_size; > @@ -129,6 +172,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); > } thanks, Mimi _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
WARNING: multiple messages have this Message-ID (diff)
From: Mimi Zohar <zohar@linux.ibm.com> To: Tushar Sugandhi <tusharsu@linux.microsoft.com>, roberto.sassu@huaweicloud.com, roberto.sassu@huawei.com, eric.snowberg@oracle.com, stefanb@linux.ibm.com, ebiederm@xmission.com, noodles@fb.com, bauermann@kolabnow.com, linux-integrity@vger.kernel.org, kexec@lists.infradead.org Cc: code@tyhicks.com, nramas@linux.microsoft.com, paul@paul-moore.com Subject: Re: [PATCH v5 1/8] ima: define and call ima_alloc_kexec_file_buf Date: Tue, 20 Feb 2024 19:12:41 -0500 [thread overview] Message-ID: <844e1af9a5d0414d4b972c8c947b7f821a776d71.camel@linux.ibm.com> (raw) In-Reply-To: <20240214153827.1087657-2-tusharsu@linux.microsoft.com> On Wed, 2024-02-14 at 07:38 -0800, Tushar Sugandhi 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: > - 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() > as local static to the file, so that it can be accessed from > ima_alloc_kexec_file_buf(). > - Make necessary changes to the function ima_add_kexec_buffer() to call > the above two functions. > > Suggested-by: Stefan Berger <stefanb@linux.ibm.com> > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > --- > security/integrity/ima/ima_kexec.c | 107 +++++++++++++++++++++-------- > 1 file changed, 78 insertions(+), 29 deletions(-) > > diff --git a/security/integrity/ima/ima_kexec.c > b/security/integrity/ima/ima_kexec.c > index dadc1d138118..a9cb5e882e2e 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -15,62 +15,98 @@ > #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 && > + ima_kexec_file.read_pos == 0 && > + ima_kexec_file.count == sizeof(struct ima_kexec_hdr)) > + return 0; ima_kexec_file.size will be initialized to zero. No need to test anything other than the segment size. > + > + ima_free_kexec_file_buf(&ima_kexec_file); The first time ima_free_kexec_file_buf() is called the memory has not been allocated. Test that the memory has been allocated before freeing it. Since there's no other ima_free_kexec_file_buf() caller, there's no need to clear the other fields as they will be set below. > + > + /* 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; static variables are initialized to zero. No need to set it here. > + 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 ima_queue_entry *qe; > - struct seq_file file; > struct ima_kexec_hdr khdr; > - int ret = 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) { > + *buffer_size = 0; > + *buffer = NULL; > + pr_err("%s: Kexec file buf not allocated\n", __func__); > + return -EINVAL; > } > > - file.size = segment_size; > - file.read_pos = 0; > - file.count = sizeof(khdr); /* reserved space */ > - > memset(&khdr, 0, sizeof(khdr)); > khdr.version = 1; > + > + /* Copy as many IMA measurements list records as possible */ > list_for_each_entry_rcu(qe, &ima_measurements, later) { > - if (file.count < file.size) { > + if (ima_kexec_file.count < ima_kexec_file.size) { With an extra half page memory allocated, there should be enough memory for a few extra records as a result of the kexec load. Subsequent patches are going to change this assumption. This test doesn't ensure there is enough memory for the entire measurement record. Call get_binary_runtime_size() to get the record size to ensure there is enough memory for the entire record. > khdr.count++; > - ima_measurements_show(&file, qe); > + ima_measurements_show(&ima_kexec_file, qe); > } else { > - ret = -EINVAL; > + pr_err("%s: IMA log file is too big for Kexec buf\n", > + __func__); This message and the one above should probably be pr_debug(). There's a new error message after the call to ima_dump_measurement_list(). > break; > } > } > > - if (ret < 0) > - goto out; > - Removing these lines is in for "copying as many measurements as possible". That change isn't mentioned in the patch description. > /* > * 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; > -out: > - if (ret == -EINVAL) > - vfree(file.buf); > - return ret; > + *buffer_size = ima_kexec_file.count; > + *buffer = ima_kexec_file.buf; > + > + return 0; > } > > /* > @@ -108,13 +144,20 @@ 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("%s: Failed to dump IMA measurements. Error:%d.\n", > + __func__, ret); Please remove "__func__" from the error message. > + return; > + } > + > kbuf.buffer = kexec_buffer; > kbuf.bufsz = kexec_buffer_size; > kbuf.memsz = kexec_segment_size; > @@ -129,6 +172,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); > } thanks, Mimi
next prev parent reply other threads:[~2024-02-21 0:13 UTC|newest] Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-02-14 15:38 [PATCH v5 0/8] ima: kexec: measure events between kexec load and execute Tushar Sugandhi 2024-02-14 15:38 ` Tushar Sugandhi 2024-02-14 15:38 ` [PATCH v5 1/8] ima: define and call ima_alloc_kexec_file_buf Tushar Sugandhi 2024-02-14 15:38 ` Tushar Sugandhi 2024-02-19 22:16 ` Stefan Berger 2024-02-19 22:16 ` Stefan Berger 2024-02-21 0:12 ` Mimi Zohar [this message] 2024-02-21 0:12 ` Mimi Zohar 2024-02-14 15:38 ` [PATCH v5 2/8] kexec: define functions to map and unmap segments Tushar Sugandhi 2024-02-14 15:38 ` Tushar Sugandhi 2024-02-14 19:43 ` Stefan Berger 2024-02-14 19:43 ` Stefan Berger 2024-02-15 6:13 ` Tushar Sugandhi 2024-02-15 6:13 ` Tushar Sugandhi 2024-02-21 20:22 ` Mimi Zohar 2024-02-21 20:22 ` Mimi Zohar 2024-02-14 15:38 ` [PATCH v5 3/8] ima: kexec: skip IMA segment validation after kexec soft reboot Tushar Sugandhi 2024-02-14 15:38 ` Tushar Sugandhi 2024-02-14 15:38 ` [PATCH v5 4/8] ima: kexec: define functions to copy IMA log at soft boot Tushar Sugandhi 2024-02-14 15:38 ` Tushar Sugandhi 2024-02-14 20:47 ` Stefan Berger 2024-02-14 20:47 ` Stefan Berger 2024-02-15 6:55 ` Tushar Sugandhi 2024-02-15 6:55 ` Tushar Sugandhi 2024-02-21 21:52 ` Mimi Zohar 2024-02-21 21:52 ` Mimi Zohar 2024-02-21 22:39 ` Mimi Zohar 2024-02-21 22:39 ` Mimi Zohar 2024-03-01 11:12 ` Petr Tesařík 2024-03-01 11:12 ` Petr Tesařík 2024-02-14 15:38 ` [PATCH v5 5/8] ima: kexec: move IMA log copy from kexec load to execute Tushar Sugandhi 2024-02-14 15:38 ` Tushar Sugandhi 2024-02-14 20:58 ` Stefan Berger 2024-02-14 20:58 ` Stefan Berger 2024-02-22 1:47 ` Mimi Zohar 2024-02-22 1:47 ` Mimi Zohar 2024-05-09 6:32 ` Petr Tesařík 2024-05-09 6:32 ` Petr Tesařík 2024-02-14 15:38 ` [PATCH v5 6/8] ima: suspend measurements during buffer copy at kexec execute Tushar Sugandhi 2024-02-14 15:38 ` Tushar Sugandhi 2024-02-22 14:14 ` Mimi Zohar 2024-02-22 14:14 ` Mimi Zohar 2024-02-22 16:38 ` Mimi Zohar 2024-02-22 16:38 ` Mimi Zohar 2024-02-29 13:21 ` Petr Tesařík 2024-02-29 13:21 ` Petr Tesařík 2024-02-14 15:38 ` [PATCH v5 7/8] ima: make the kexec extra memory configurable Tushar Sugandhi 2024-02-14 15:38 ` Tushar Sugandhi 2024-02-22 14:13 ` Mimi Zohar 2024-02-22 14:13 ` Mimi Zohar 2024-02-14 15:38 ` [PATCH v5 8/8] ima: measure kexec load and exec events as critical data Tushar Sugandhi 2024-02-14 15:38 ` Tushar Sugandhi 2024-02-14 21:00 ` Stefan Berger 2024-02-14 21:00 ` Stefan Berger 2024-02-15 6:57 ` Tushar Sugandhi 2024-02-15 6:57 ` Tushar Sugandhi 2024-02-14 21:03 ` Stefan Berger 2024-02-14 21:03 ` Stefan Berger 2024-02-15 6:58 ` Tushar Sugandhi 2024-02-15 6:58 ` Tushar Sugandhi 2024-02-21 0:15 ` [PATCH v5 0/8] ima: kexec: measure events between kexec load and execute Mimi Zohar 2024-02-21 0:15 ` Mimi Zohar
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=844e1af9a5d0414d4b972c8c947b7f821a776d71.camel@linux.ibm.com \ --to=zohar@linux.ibm.com \ --cc=bauermann@kolabnow.com \ --cc=code@tyhicks.com \ --cc=ebiederm@xmission.com \ --cc=eric.snowberg@oracle.com \ --cc=kexec@lists.infradead.org \ --cc=linux-integrity@vger.kernel.org \ --cc=noodles@fb.com \ --cc=nramas@linux.microsoft.com \ --cc=paul@paul-moore.com \ --cc=roberto.sassu@huawei.com \ --cc=roberto.sassu@huaweicloud.com \ --cc=stefanb@linux.ibm.com \ --cc=tusharsu@linux.microsoft.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.