Re: [PATCH v2 3/7] ima: kexec: map source pages containing IMA buffer to image post kexec load

[Tushar Sugandhi <tusharsu@linux.microsoft.com>]

On 10/12/23 17:29, Stefan Berger wrote:
> 
> On 10/5/23 14:25, Tushar Sugandhi wrote:
>> Currently, the mechanism to map and unmap segments to the kimage
>> structure is not available to the subsystems outside of kexec.  This
>> functionality is needed when IMA is allocating the memory segments
>> during kexec 'load' operation.
>>
>> Implement kimage_map_segment() which takes a kimage pointer, an address,
>> and a size.  Ensure that the entire segment is being mapped by comparing
>> the given address and size to each segment in the kimage's segment array.
>> Collect the source pages that correspond to the given address range,
>> allocate an array of pointers to these pages, and map them to a
>> contiguous range of virtual addresses.  If the mapping operation is
>> successful, the function returns the start of this range.  Otherwise, it
>> frees the page pointer array and returns NULL.
>>
>> Implement kimage_unmap_segment() that takes a pointer to a segment buffer
>> and unmaps it using vunmap().
>>
>> Implement function ima_kexec_post_load(), to be called by IMA after kexec
>> loads the new Kernel image.  ima_kexec_post_load() would map the IMA
>> buffer allocated during kexec 'load' to a segment in the loaded image.
>>
>> Finally, move for_each_kimage_entry() macro from kexec_core.c to kexec.h.
>>
>> Signed-off-by: Tushar Sugandhi<tusharsu@linux.microsoft.com>
>> ---
>>   include/linux/ima.h                |  3 ++
>>   include/linux/kexec.h              | 13 ++++++
>>   kernel/kexec_core.c                | 73 ++++++++++++++++++++++++++++--
>>   security/integrity/ima/ima_kexec.c | 32 +++++++++++++
>>   4 files changed, 116 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/ima.h b/include/linux/ima.h
>> index 86b57757c7b1..006db20f852d 100644
>> --- a/include/linux/ima.h
>> +++ b/include/linux/ima.h
>> @@ -49,6 +49,9 @@ static inline void ima_appraise_parse_cmdline(void) {}
>>   #ifdef CONFIG_IMA_KEXEC
>>   extern void ima_add_kexec_buffer(struct kimage *image);
>> +extern void ima_kexec_post_load(struct kimage *image);
>> +#else
>> +static inline void ima_kexec_post_load(struct kimage *image) {}
>>   #endif
>>   #else
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index 22b5cd24f581..e00b8101b53b 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -490,6 +490,15 @@ static inline int 
>> arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages, g
>>   static inline void arch_kexec_pre_free_pages(void *vaddr, unsigned 
>> int pages) { }
>>   #endif
>> +#define for_each_kimage_entry(image, ptr, entry) \
>> +    for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
>> +        ptr = (entry & IND_INDIRECTION) ? \
>> +            boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
>> +
>> +extern void *kimage_map_segment(struct kimage *image,
>> +                unsigned long addr, unsigned long size);
>> +extern void kimage_unmap_segment(void *buffer);
>> +
>>   #else /* !CONFIG_KEXEC_CORE */
>>   struct pt_regs;
>>   struct task_struct;
>> @@ -497,6 +506,10 @@ static inline void __crash_kexec(struct pt_regs 
>> *regs) { }
>>   static inline void crash_kexec(struct pt_regs *regs) { }
>>   static inline int kexec_should_crash(struct task_struct *p) { return 
>> 0; }
>>   static inline int kexec_crash_loaded(void) { return 0; }
>> +static inline void *kimage_map_segment(struct kimage *image,
>> +                       unsigned long addr, unsigned long size)
>> +{ return NULL; }
>> +static inline void kimage_unmap_segment(void *buffer) { }
>>   #define kexec_in_progress false
>>   #endif /* CONFIG_KEXEC_CORE */
>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>> index 3d578c6fefee..e01156f3c404 100644
>> --- a/kernel/kexec_core.c
>> +++ b/kernel/kexec_core.c
>> @@ -594,11 +594,6 @@ void kimage_terminate(struct kimage *image)
>>       *image->entry = IND_DONE;
>>   }
>> -#define for_each_kimage_entry(image, ptr, entry) \
>> -    for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
>> -        ptr = (entry & IND_INDIRECTION) ? \
>> -            boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
>> -
>>   static void kimage_free_entry(kimage_entry_t entry)
>>   {
>>       struct page *page;
>> @@ -921,6 +916,74 @@ int kimage_load_segment(struct kimage *image,
>>       return result;
>>   }
>> +void *kimage_map_segment(struct kimage *image,
>> +             unsigned long addr, unsigned long size)
>> +{
>> +    unsigned long eaddr = addr + size;
>> +    unsigned long src_page_addr, dest_page_addr;
>> +    struct page **src_pages;
>> +    int i, npages;
>> +    kimage_entry_t *ptr, entry;
>> +    void *vaddr = NULL;
>> +
>> +    /*
>> +     * Make sure that we are mapping a whole segment.
>> +     */
>> +    for (i = 0; i < image->nr_segments; i++) {
>> +        if (addr == image->segment[i].mem &&
>> +            size == image->segment[i].memsz) {
>> +            break;
>> +        }
>> +    }
> 
> I wonder whether this distrust in the segments or in the passed 
> pointer&size are justifyable since you call this function like this:
> 
>          ima_kexec_buffer = kimage_map_segment(image,
> image->ima_buffer_addr,
> image->ima_buffer_size);
> 
> and previously kexec_add_buffer() was also called:
> 
>          kbuf.buffer = kexec_buffer;
>          kbuf.bufsz = kexec_buffer_size;
>          kbuf.memsz = kexec_segment_size;
>          ret = kexec_add_buffer(&kbuf);
>          if (ret) {
>                  pr_err("Error passing over kexec measurement buffer.\n");
>                  vfree(kexec_buffer);
>                  return;
>          }
> 
>          image->ima_buffer_addr = kbuf.mem;
>          image->ima_buffer_size = kexec_segment_size;
> 
> So the one segment should be matching these addresses.
> 
> 
Yeh. This check is an overkill at this point.
I agree. I will remove this extra check.
>> +
>> +    if (i == image->nr_segments) {
>> +        pr_err("%s: No segment matching [%lx, %lx)\n", __func__,
>> +               addr, eaddr);
>> +        return NULL;
>> +    }
>> +
>> +    /*
>> +     * Collect the source pages and map them in a contiguous VA range.
>> +     */
>> +    npages = PFN_UP(eaddr) - PFN_DOWN(addr);
>> +    src_pages = kmalloc(sizeof(*src_pages) * npages, GFP_KERNEL);
>> +    if (!src_pages) {
>> +        pr_err("%s: Could not allocate ima pages array.\n", __func__);
>> +        return NULL;
>> +    }
>> +
>> +    i = 0;
>> +    for_each_kimage_entry(image, ptr, entry) {
>> +        if (entry & IND_DESTINATION)
>> +            dest_page_addr = entry & PAGE_MASK;
>> +        else if (entry & IND_SOURCE) {
>> +            if (dest_page_addr >= addr && dest_page_addr < eaddr) {
>> +                src_page_addr = entry & PAGE_MASK;
>> +                src_pages[i++] =
>> +                    virt_to_page(__va(src_page_addr));
>> +                if (i == npages)
>> +                    break;
>> +                dest_page_addr += PAGE_SIZE;
>> +            }
>> +        }
>> +    }
>> +
>> +    /* Sanity check. */
>> +    WARN_ON(i < npages);
>> +
>> +    vaddr = vmap(src_pages, npages, VM_MAP, PAGE_KERNEL);
> On either success or failure kfree(sec_pages); should go right here to 
> not have a memory leak.
Will do.

~Tushar
>> +    if (!vaddr) {
>> +        pr_err("%s: Could not map imap buffer.\n", __func__);
>> +        kfree(src_pages);
>> +    }
>> +    return vaddr;
>> +}
>> +
>> +void kimage_unmap_segment(void *segment_buffer)
>> +{
>> +    vunmap(segment_buffer);
>> +}
>> +
>>   struct kexec_load_limit {
>>       /* Mutex protects the limit count. */
>>       struct mutex mutex;
>> diff --git a/security/integrity/ima/ima_kexec.c 
>> b/security/integrity/ima/ima_kexec.c
>> index 2c11bbe6efef..13fbbb90319b 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -12,6 +12,8 @@
>>   #include <linux/kexec.h>
>>   #include <linux/of.h>
>>   #include <linux/ima.h>
>> +#include <linux/reboot.h>
>> +#include <asm/page.h>
>>   #include "ima.h"
>>   #ifdef CONFIG_IMA_KEXEC
>> @@ -19,6 +21,7 @@ struct seq_file ima_kexec_file;
>>   struct ima_kexec_hdr ima_khdr;
>>   static void *ima_kexec_buffer;
>>   static size_t kexec_segment_size;
>> +static bool ima_kexec_update_registered;
>>   void ima_clear_kexec_file(void)
>>   {
>> @@ -221,6 +224,7 @@ static int ima_update_kexec_buffer(struct 
>> notifier_block *self,
>>       }
>>       memcpy(ima_kexec_buffer, buf, buf_size);
>>   out:
>> +    kimage_unmap_segment(ima_kexec_buffer);
>>       ima_kexec_buffer = NULL;
>>       if (resume)
>> @@ -232,6 +236,34 @@ struct notifier_block update_buffer_nb = {
>>       .notifier_call = ima_update_kexec_buffer,
>>   };
>> +/*
>> + * Create a mapping for the source pages that contain the IMA buffer
>> + * so we can update it later.
>> + */
>> +void ima_kexec_post_load(struct kimage *image)
>> +{
>> +    if (ima_kexec_buffer) {
>> +        kimage_unmap_segment(ima_kexec_buffer);
>> +        ima_kexec_buffer = NULL;
>> +    }
>> +
>> +    if (!image->ima_buffer_addr)
>> +        return;
>> +
>> +    ima_kexec_buffer = kimage_map_segment(image,
>> +                          image->ima_buffer_addr,
>> +                          image->ima_buffer_size);
>> +    if (!ima_kexec_buffer) {
>> +        pr_err("%s: Could not map measurements buffer.\n", __func__);
>> +        return;
>> +    }
>> +
>> +    if (!ima_kexec_update_registered) {
>> +        register_reboot_notifier(&update_buffer_nb);
>> +        ima_kexec_update_registered = true;
>> +    }
>> +}
>> +
>>   #endif /* IMA_KEXEC */
>>   /*

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec