On 2/14/24 10:38, 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
as -> a
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@xxxxxxxxxxxxx>
Signed-off-by: Tushar Sugandhi <tusharsu@xxxxxxxxxxxxxxxxxxx>
---
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_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 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;
If you return -EINVAL then the caller should not look at the buffer but
at 'ret' (5/8). So I don't think it's necessary to initialize these
variables since the caller shouldn't look at them.
+ 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) {
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__);
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;
-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);
+ 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.
+ */
Actually, that's not quite right what I told you. "kexec owns
kexec_buffer due to ima->ima_buffer and it will vfree() this buffer."
+ ima_reset_kexec_file(&ima_kexec_file);
+
kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
kbuf.mem);
}
