| Message ID | 20240214153827.1087657-5-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: kexec: measure events between kexec load and execute | expand |
On 2/14/24 10:38, Tushar Sugandhi wrote: > IMA log is copied to the new Kernel during kexec 'load' using > ima_dump_measurement_list(). The log copy at kexec 'load' may result in > loss of IMA measurements during kexec soft reboot. It needs to be copied > over during kexec 'execute'. Setup the needed infrastructure to move the > IMA log copy from kexec 'load' to 'execute'. > > Define a new IMA hook ima_update_kexec_buffer() as a stub function. > It will be used to call ima_dump_measurement_list() during kexec > 'execute'. > > Implement kimage_file_post_load() and ima_kexec_post_load() functions > to be invoked after the new Kernel image has been loaded for kexec. > ima_kexec_post_load() maps the IMA buffer to a segment in the newly > loaded Kernel. It also registers the reboot notifier_block to trigger > ima_update_kexec_buffer() at exec 'execute'. > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > --- > include/linux/ima.h | 3 ++ > kernel/kexec_file.c | 5 ++++ > security/integrity/ima/ima_kexec.c | 46 ++++++++++++++++++++++++++++++ > 3 files changed, 54 insertions(+) > > 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/kernel/kexec_file.c b/kernel/kexec_file.c > index 0e3689bfb0bb..fe59cb7c179d 100644 > --- a/kernel/kexec_file.c > +++ b/kernel/kexec_file.c > @@ -186,6 +186,11 @@ kimage_validate_signature(struct kimage *image) > } > #endif > > +void kimage_file_post_load(struct kimage *image) > +{ > + ima_kexec_post_load(image); > +} > + We get this here at this point but it disappears later -- missing header? kernel/kexec_file.c:189:6: warning: no previous prototype for ‘kimage_file_post_load’ [-Wmissing-prototypes] 189 | void kimage_file_post_load(struct kimage *image) | ^~~~~~~~~~~~~~~~~~~~~ > /* > * In file mode list of segments is prepared by kernel. Copy relevant > * data from user space, do error checking, prepare segment list > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c > index ccb072617c2d..1d4d6c122d82 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -12,10 +12,14 @@ > #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 > static struct seq_file ima_kexec_file; > +static void *ima_kexec_buffer; > +static bool ima_kexec_update_registered; > > static void ima_reset_kexec_file(struct seq_file *sf) > { > @@ -184,6 +188,48 @@ void ima_add_kexec_buffer(struct kimage *image) > kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n", > kbuf.mem); > } > + > +/* > + * Called during kexec execute so that IMA can update the measurement list. > + */ > +static int ima_update_kexec_buffer(struct notifier_block *self, > + unsigned long action, void *data) > +{ > + return NOTIFY_OK; > +} > + > +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 */ > > /* Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
On 2/14/24 12:47, Stefan Berger wrote: > > > On 2/14/24 10:38, Tushar Sugandhi wrote: ... <snip/> ... >> +void kimage_file_post_load(struct kimage *image) >> +{ >> + ima_kexec_post_load(image); >> +} >> + > > We get this here at this point but it disappears later -- missing header? > > kernel/kexec_file.c:189:6: warning: no previous prototype for > ‘kimage_file_post_load’ [-Wmissing-prototypes] > 189 | void kimage_file_post_load(struct kimage *image) > | ^~~~~~~~~~~~~~~~~~~~~ > > Thanks Stefan. I was also getting it. But couldn't figure out why. And I was puzzled why it was going away. Since kimage_file_post_load() is called from the same file in patch 5/8, I don't see a need of declaring it in a header file like include/linux/kexec.h. Making kimage_file_post_load() local static resolves the warning. But then it throws "defined but not used" warning. I will have to call it from kexec_file_load syscall in this patch (4/8) instead 5/8 to resolve that warning. I will make the function a stub function in this patch and make it call ima_kexec_post_load(image) in the next patch to avoid any potential bisect safe issues. It aligns with the goals of patch 4/8 and 5/8 anyways. +static void kimage_file_post_load(struct kimage *image) +{ + /* + * this will call ima_kexec_post_load(image) to map the segment + * and register the reboot notifier for moving the IMA log at + * kexec execute + */ +} + @@ -410,6 +410,9 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, kimage_terminate(image); + if (!(flags & KEXEC_FILE_ON_CRASH)) + kimage_file_post_load(image); + ... ...<snip/> ... > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> Thanks for the tag. I will apply it on the next version. ~Tushar
On Wed, 2024-02-14 at 22:55 -0800, Tushar Sugandhi wrote: > > On 2/14/24 12:47, Stefan Berger wrote: > > > > On 2/14/24 10:38, Tushar Sugandhi wrote: > ... > <snip/> > ... > > > +void kimage_file_post_load(struct kimage *image) > > > +{ > > > + ima_kexec_post_load(image); > > > +} > > > + > > > > We get this here at this point but it disappears later -- missing header? > > > > kernel/kexec_file.c:189:6: warning: no previous prototype for > > ‘kimage_file_post_load’ [-Wmissing-prototypes] > > 189 | void kimage_file_post_load(struct kimage *image) > > | ^~~~~~~~~~~~~~~~~~~~~ > > > > > Thanks Stefan. > I was also getting it. > But couldn't figure out why. And I was puzzled why it was going away. > > Since kimage_file_post_load() is called from the same file in patch 5/8, > I don't see a need of declaring it in a header file like > include/linux/kexec.h. Before trying to "fix" it, is there a need for a wrapper around ima_kexec_post_load(). ima_add_kexec_buffer() is called directly. Mimi > > Making kimage_file_post_load() local static resolves the warning. > But then it throws "defined but not used" warning. I will have to call > it from kexec_file_load syscall in this patch (4/8) instead 5/8 to > resolve that warning. > > I will make the function a stub function in this patch and > make it call ima_kexec_post_load(image) in the next patch to avoid any > potential bisect safe issues. > > It aligns with the goals of patch 4/8 and 5/8 anyways. > > +static void kimage_file_post_load(struct kimage *image) > +{ > + /* > + * this will call ima_kexec_post_load(image) to map the segment > + * and register the reboot notifier for moving the IMA log at > + * kexec execute > + */ > +} > + > > > @@ -410,6 +410,9 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, > int, initrd_fd, > kimage_terminate(image); > + if (!(flags & KEXEC_FILE_ON_CRASH)) > + kimage_file_post_load(image); > + > > ... > ...<snip/> > ... > > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > Thanks for the tag. I will apply it on the next version. > > ~Tushar
Additional comments ... > diff --git a/security/integrity/ima/ima_kexec.c > b/security/integrity/ima/ima_kexec.c > index ccb072617c2d..1d4d6c122d82 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -12,10 +12,14 @@ > #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 > static struct seq_file ima_kexec_file; > +static void *ima_kexec_buffer; > +static bool ima_kexec_update_registered; > > static void ima_reset_kexec_file(struct seq_file *sf) > { > @@ -184,6 +188,48 @@ void ima_add_kexec_buffer(struct kimage *image) > kexec_dprintk("kexec measurement buffer for the loaded kernel at > 0x%lx.\n", > kbuf.mem); > } > + > +/* > + * Called during kexec execute so that IMA can update the measurement list. > + */ > +static int ima_update_kexec_buffer(struct notifier_block *self, > + unsigned long action, void *data) > +{ > + return NOTIFY_OK; > +} > + > +struct notifier_block update_buffer_nb = { This should be defined as static. update_buffer_nb should be prefixed with 'ima_'. > + .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) > +{ In ima_alloc_kexec_file_buf(), the buffer is only re-allocated if the size changes. Here there doesn't seem to be way of detecting a size change. At least, add a comment here indicating that kexec 'load' may be called multiple times. Mimi > + 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 */ > > /*
On Wed, 14 Feb 2024 07:38:23 -0800 Tushar Sugandhi <tusharsu@linux.microsoft.com> wrote: > IMA log is copied to the new Kernel during kexec 'load' using > ima_dump_measurement_list(). The log copy at kexec 'load' may result in > loss of IMA measurements during kexec soft reboot. It needs to be copied > over during kexec 'execute'. Setup the needed infrastructure to move the > IMA log copy from kexec 'load' to 'execute'. > > Define a new IMA hook ima_update_kexec_buffer() as a stub function. > It will be used to call ima_dump_measurement_list() during kexec > 'execute'. > > Implement kimage_file_post_load() and ima_kexec_post_load() functions > to be invoked after the new Kernel image has been loaded for kexec. > ima_kexec_post_load() maps the IMA buffer to a segment in the newly > loaded Kernel. It also registers the reboot notifier_block to trigger > ima_update_kexec_buffer() at exec 'execute'. > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > --- > include/linux/ima.h | 3 ++ > kernel/kexec_file.c | 5 ++++ > security/integrity/ima/ima_kexec.c | 46 ++++++++++++++++++++++++++++++ > 3 files changed, 54 insertions(+) > > 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/kernel/kexec_file.c b/kernel/kexec_file.c > index 0e3689bfb0bb..fe59cb7c179d 100644 > --- a/kernel/kexec_file.c > +++ b/kernel/kexec_file.c > @@ -186,6 +186,11 @@ kimage_validate_signature(struct kimage *image) > } > #endif > > +void kimage_file_post_load(struct kimage *image) > +{ > + ima_kexec_post_load(image); > +} > + > /* > * In file mode list of segments is prepared by kernel. Copy relevant > * data from user space, do error checking, prepare segment list > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c > index ccb072617c2d..1d4d6c122d82 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -12,10 +12,14 @@ > #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 > static struct seq_file ima_kexec_file; > +static void *ima_kexec_buffer; > +static bool ima_kexec_update_registered; > > static void ima_reset_kexec_file(struct seq_file *sf) > { > @@ -184,6 +188,48 @@ void ima_add_kexec_buffer(struct kimage *image) > kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n", > kbuf.mem); > } > + > +/* > + * Called during kexec execute so that IMA can update the measurement list. > + */ > +static int ima_update_kexec_buffer(struct notifier_block *self, > + unsigned long action, void *data) > +{ > + return NOTIFY_OK; > +} > + > +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); Specifically this means that the notifier is run from kernel_kexec() through kernel_restart_prepare(). At that point the kernel is still running at full speed, i.e. new IMA measurements can be added to the list. I believe it would be better to add a "kexec late notifier list" which would be notified just before kmsg_dump() in kernel_kexec(). Functions on that list would be called in a more quiet context, i.e. after other CPUs and most devices have been shut down. I can see that the current proposal does not address passing the IMA measurements to a panic kernel, but I'm adding Baoquan nevertheless. Would it make sense to implement such a late notifier list for the crash kexec case. I know that in general, it is no good idea to run more code in a crashed kernel context, but what about self-contained things like copying IMA measurements? Petr T
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/kernel/kexec_file.c b/kernel/kexec_file.c index 0e3689bfb0bb..fe59cb7c179d 100644 --- a/kernel/kexec_file.c +++ b/kernel/kexec_file.c @@ -186,6 +186,11 @@ kimage_validate_signature(struct kimage *image) } #endif +void kimage_file_post_load(struct kimage *image) +{ + ima_kexec_post_load(image); +} + /* * In file mode list of segments is prepared by kernel. Copy relevant * data from user space, do error checking, prepare segment list diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c index ccb072617c2d..1d4d6c122d82 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -12,10 +12,14 @@ #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 static struct seq_file ima_kexec_file; +static void *ima_kexec_buffer; +static bool ima_kexec_update_registered; static void ima_reset_kexec_file(struct seq_file *sf) { @@ -184,6 +188,48 @@ void ima_add_kexec_buffer(struct kimage *image) kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n", kbuf.mem); } + +/* + * Called during kexec execute so that IMA can update the measurement list. + */ +static int ima_update_kexec_buffer(struct notifier_block *self, + unsigned long action, void *data) +{ + return NOTIFY_OK; +} + +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 */ /*
IMA log is copied to the new Kernel during kexec 'load' using ima_dump_measurement_list(). The log copy at kexec 'load' may result in loss of IMA measurements during kexec soft reboot. It needs to be copied over during kexec 'execute'. Setup the needed infrastructure to move the IMA log copy from kexec 'load' to 'execute'. Define a new IMA hook ima_update_kexec_buffer() as a stub function. It will be used to call ima_dump_measurement_list() during kexec 'execute'. Implement kimage_file_post_load() and ima_kexec_post_load() functions to be invoked after the new Kernel image has been loaded for kexec. ima_kexec_post_load() maps the IMA buffer to a segment in the newly loaded Kernel. It also registers the reboot notifier_block to trigger ima_update_kexec_buffer() at exec 'execute'. Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> --- include/linux/ima.h | 3 ++ kernel/kexec_file.c | 5 ++++ security/integrity/ima/ima_kexec.c | 46 ++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+)