On Thu, 2023-10-26 at 16:16 -0400, Mimi Zohar wrote:
> Hi Tushar,
>
> According to Documentation/process/submitting-patches.rst, the subject
> line should be between 70-75 characters.
>
> Perhaps something like "ima: define and call ima_alloc_kexec_buffer()".
>
> On Thu, 2023-10-05 at 11:25 -0700, Tushar Sugandhi wrote:
> > IMA allocates memory and dumps the measurement during kexec soft reboot
> > as a single function call ima_dump_measurement_list(). It gets called
> > during kexec 'load' operation. It results in the IMA measurements
> > between the window of kexec 'load' and 'execute' getting dropped when the
> > system boots into the new Kernel. One of the kexec requirements is the
> > segment size cannot change between the 'load' and the 'execute'.
> > Therefore, to address this problem, ima_dump_measurement_list() needs
> > to be refactored to allocate the memory at kexec 'load', and dump the
> > measurements at kexec 'execute'. The function that allocates the memory
> > should handle the scenario where the kexec load is called multiple times
>
> The above pragraph is unnecessary.
>
> > Refactor ima_dump_measurement_list() to move the memory allocation part
> > to a separate function ima_alloc_kexec_buf() to allocate buffer of size
> > 'kexec_segment_size' at kexec 'load'. Make the local variables in
> > function ima_dump_measurement_list() global, so that they can be accessed
> > from ima_alloc_kexec_buf(). Make necessary changes to the function
> > ima_add_kexec_buffer() to call the above two functions.
>
> Fix the wording based on the suggested changes below.
>
> > Signed-off-by: Tushar Sugandhi <tusharsu@xxxxxxxxxxxxxxxxxxx>
>
> - Before re-posting this patch set, verify there aren't any
> "checkpatch.pl --strict" issues.
> - After applying each patch, compile the kernel and verify it still
> works.
Doing this will detect whether or not the patch set is bisect safe.
> > ---
> > security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++--------
> > 1 file changed, 93 insertions(+), 33 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> > index 419dc405c831..307e07991865 100644
> > --- a/security/integrity/ima/ima_kexec.c
> > +++ b/security/integrity/ima/ima_kexec.c
> > @@ -15,61 +15,114 @@
> > #include "ima.h"
> >
> > #ifdef CONFIG_IMA_KEXEC
> > +struct seq_file ima_kexec_file;
>
> Define "ima_kexec_file" as static since it only used in this file.
> Since the variable does not need to be global, is there still a reason
> for changing its name? Minimize code change.
Adding "static" would make ima_kexec_file a global static variable.
Please ignore my comment about reverting the variable name change.
Mimi
>
> > +struct ima_kexec_hdr ima_khdr;
> > +
> > +void ima_clear_kexec_file(void)
>
> The opposite of "set" is "clear" (e.g. set_git, clear_bit). The
> opposite of "alloc" would be "free" (e.g. ima_alloc_kexec_buf,
> ima_free_kexec_buf)
>
> > +{
> > + vfree(ima_kexec_file.buf);
> > + ima_kexec_file.buf = NULL;
> > + ima_kexec_file.size = 0;
> > + ima_kexec_file.read_pos = 0;
> > + ima_kexec_file.count = 0;
> > +}
> > +
> > +static int ima_alloc_kexec_buf(size_t kexec_segment_size)
> > +{
> > + if ((kexec_segment_size == 0) ||
> > + (kexec_segment_size == ULONG_MAX) ||
> > + ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
> > + pr_err("%s: Invalid segment size for kexec: %zu\n",
> > + __func__, kexec_segment_size);
> > + return -EINVAL;
> > + }
>
> Please avoid code duplication. If moving the test here, then remove it
> from its original place.
>
> > +
> > + /*
> > + * If kexec load was called before, clear the existing buffer
> > + * before allocating a new one
> > + */
>
> > + if (ima_kexec_file.buf)
> > + ima_clear_kexec_file();
>
> Perhaps instead of always freeing the buffer, check and see whether the
> size has changed. If it hasn't changed, then simply return. If it has
> changed, perhaps use realloc(). Possible wording:
>
> "Kexec may be called multiple times. Free and re-alloc the buffer if
> the size changed."
>
> > +
> > + /* segment size can't change between kexec load and execute */
> > + ima_kexec_file.buf = vmalloc(kexec_segment_size);
> > + if (!ima_kexec_file.buf) {
> > + pr_err("%s: No memory for ima kexec measurement buffer\n",
> > + __func__);
> > + return -ENOMEM;
> > + }
> > +
> > + ima_kexec_file.size = kexec_segment_size;
> > + ima_kexec_file.read_pos = 0;
> > + ima_kexec_file.count = sizeof(ima_khdr); /* reserved space */
> > +
> > + memset(&ima_khdr, 0, sizeof(ima_khdr));
> > + ima_khdr.version = 1;
> > +
> > + 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) {
> > + 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 */
> > + /*
> > + * Ensure the kexec buffer is large enough to hold ima_khdr
> > + */
> > + if (ima_kexec_file.size < sizeof(ima_khdr)) {
> > + pr_err("%s: Kexec buffer size too low to hold ima_khdr\n",
> > + __func__);
> > + ima_clear_kexec_file();
> > + return -ENOMEM;
> > + }
>
> Is this necessary?
>
> > - memset(&khdr, 0, sizeof(khdr));
> > - khdr.version = 1;
> > + /*
> > + * If we reach here, then there is enough memory
> > + * of size kexec_segment_size in ima_kexec_file.buf
> > + * to copy at least partial IMA log.
> > + * Make best effort to copy as many IMA measurements
> > + * as possible.
> > + */
>
> Suggestion:
>
> /* Copy as many IMA measurements list records as possible */
>
> > list_for_each_entry_rcu(qe, &ima_measurements, later) {
> > - if (file.count < file.size) {
> > - khdr.count++;
> > - ima_measurements_show(&file, qe);
> > + if (ima_kexec_file.count < ima_kexec_file.size) {
> > + ima_khdr.count++;
> > + ima_measurements_show(&ima_kexec_file, qe);
> > } else {
> > - ret = -EINVAL;
> > + ret = EFBIG;
> > + 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;
> > + ima_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);
> > + ima_khdr.version = cpu_to_le16(ima_khdr.version);
> > + ima_khdr.count = cpu_to_le64(ima_khdr.count);
> > + ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size);
> > }
> > - memcpy(file.buf, &khdr, sizeof(khdr));
> > + memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_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);
> > + *buffer_size = ima_kexec_file.count;
> > + *buffer = ima_kexec_file.buf;
> > +
> > return ret;
> > }
>
