[v1,2/2] kexec: Introduce kexec_reboot_disabled

  Create a new toogle that disables LINUX_REBOOT_CMD_KEXEC, reducing the
attack surface to a system.

Without this toogle, an attacker can only reboot into a different kernel
if they can create a panic().

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
  

Hi Ricardo,

all in all I think this patch makes sense. However, there is one point
I don't like...

On Mon, 14 Nov 2022 14:18:39 +0100
Ricardo Ribalda <ribalda@chromium.org> wrote:

> Create a new toogle that disables LINUX_REBOOT_CMD_KEXEC, reducing the
> attack surface to a system.
> 
> Without this toogle, an attacker can only reboot into a different kernel
> if they can create a panic().
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> 
> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> index 97394bd9d065..25d019682d33 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -462,6 +462,17 @@ altered.
>  Generally used together with the `modules_disabled`_ sysctl.
>  
>  
> +kexec_reboot_disabled
> +=====================
> +
> +A toggle indicating if ``LINUX_REBOOT_CMD_KEXEC`` has been disabled.
> +This value defaults to 0 (false: ``LINUX_REBOOT_CMD_KEXEC`` enabled),
> +but can be set to 1 (true: ``LINUX_REBOOT_CMD_KEXEC`` disabled).
> +Once true, kexec can no longer be used for reboot and the toggle
> +cannot be set back to false.
> +This toggle does not affect the use of kexec during a crash.
> +
> +
>  kptr_restrict
>  =============
>  
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 41a686996aaa..15c3fad8918b 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -407,6 +407,7 @@ extern int kimage_crash_copy_vmcoreinfo(struct kimage *image);
>  extern struct kimage *kexec_image;
>  extern struct kimage *kexec_crash_image;
>  extern int kexec_load_disabled;
> +extern int kexec_reboot_disabled;
>  
>  #ifndef kexec_flush_icache_page
>  #define kexec_flush_icache_page(page)
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index cb8e6e6f983c..43063f803d81 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -196,6 +196,10 @@ static inline int kexec_load_check(unsigned long nr_segments,
>  	if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
>  		return -EPERM;
>  
> +	/* Check if the system admin has disabled kexec reboot. */
> +	if (!(flags & KEXEC_ON_CRASH) && kexec_reboot_disabled)
> +		return -EPERM;

... Allowing to load a crashkernel doesn't make sense in my opinion. If
an attacker is capable of creating a malicious kernel, planting it on
the victims system and then find a way to boot it via kexec this
attacker also knows how to load the malicious kernel as crashkernel and
trigger a panic. So you haven't really gained anything. That's why I
would simply drop this hunk (and the corresponding one from
kexec_file_load) and let users who worry about this use a combination of
kexec_load_disabled and kexec_reboot_disabled.

Thanks
Philipp

> +
>  	/* Permit LSMs and IMA to fail the kexec */
>  	result = security_kernel_load_data(LOADING_KEXEC_IMAGE, false);
>  	if (result < 0)
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index ca2743f9c634..fe82e2525705 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -929,6 +929,7 @@ int kimage_load_segment(struct kimage *image,
>  struct kimage *kexec_image;
>  struct kimage *kexec_crash_image;
>  int kexec_load_disabled;
> +int kexec_reboot_disabled;
>  #ifdef CONFIG_SYSCTL
>  static struct ctl_table kexec_core_sysctls[] = {
>  	{
> @@ -941,6 +942,16 @@ static struct ctl_table kexec_core_sysctls[] = {
>  		.extra1		= SYSCTL_ONE,
>  		.extra2		= SYSCTL_ONE,
>  	},
> +	{
> +		.procname	= "kexec_reboot_disabled",
> +		.data		= &kexec_reboot_disabled,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		/* only handle a transition from default "0" to "1" */
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= SYSCTL_ONE,
> +		.extra2		= SYSCTL_ONE,
> +	},
>  	{ }
>  };
>  
> @@ -1138,7 +1149,7 @@ int kernel_kexec(void)
>  
>  	if (!kexec_trylock())
>  		return -EBUSY;
> -	if (!kexec_image) {
> +	if (!kexec_image || kexec_reboot_disabled) {
>  		error = -EINVAL;
>  		goto Unlock;
>  	}
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 45637511e0de..583fba6de5cb 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -333,6 +333,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  	if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
>  		return -EPERM;
>  
> +	/* Check if the system admin has disabled kexec reboot. */
> +	if (!(flags & (KEXEC_FILE_ON_CRASH | KEXEC_FILE_UNLOAD))
> +	    && kexec_reboot_disabled)
> +		return -EPERM;
> +
>  	/* Make sure we have a legal set of flags */
>  	if (flags != (flags & KEXEC_FILE_FLAGS))
>  		return -EINVAL;
>
  

Hi Philipp

Thanks for your review!

On Thu, 17 Nov 2022 at 16:07, Philipp Rudo <prudo@redhat.com> wrote:
>
> Hi Ricardo,
>
> all in all I think this patch makes sense. However, there is one point
> I don't like...
>
> On Mon, 14 Nov 2022 14:18:39 +0100
> Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> > Create a new toogle that disables LINUX_REBOOT_CMD_KEXEC, reducing the
> > attack surface to a system.
> >
> > Without this toogle, an attacker can only reboot into a different kernel
> > if they can create a panic().
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >
> > diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> > index 97394bd9d065..25d019682d33 100644
> > --- a/Documentation/admin-guide/sysctl/kernel.rst
> > +++ b/Documentation/admin-guide/sysctl/kernel.rst
> > @@ -462,6 +462,17 @@ altered.
> >  Generally used together with the `modules_disabled`_ sysctl.
> >
> >
> > +kexec_reboot_disabled
> > +=====================
> > +
> > +A toggle indicating if ``LINUX_REBOOT_CMD_KEXEC`` has been disabled.
> > +This value defaults to 0 (false: ``LINUX_REBOOT_CMD_KEXEC`` enabled),
> > +but can be set to 1 (true: ``LINUX_REBOOT_CMD_KEXEC`` disabled).
> > +Once true, kexec can no longer be used for reboot and the toggle
> > +cannot be set back to false.
> > +This toggle does not affect the use of kexec during a crash.
> > +
> > +
> >  kptr_restrict
> >  =============
> >
> > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > index 41a686996aaa..15c3fad8918b 100644
> > --- a/include/linux/kexec.h
> > +++ b/include/linux/kexec.h
> > @@ -407,6 +407,7 @@ extern int kimage_crash_copy_vmcoreinfo(struct kimage *image);
> >  extern struct kimage *kexec_image;
> >  extern struct kimage *kexec_crash_image;
> >  extern int kexec_load_disabled;
> > +extern int kexec_reboot_disabled;
> >
> >  #ifndef kexec_flush_icache_page
> >  #define kexec_flush_icache_page(page)
> > diff --git a/kernel/kexec.c b/kernel/kexec.c
> > index cb8e6e6f983c..43063f803d81 100644
> > --- a/kernel/kexec.c
> > +++ b/kernel/kexec.c
> > @@ -196,6 +196,10 @@ static inline int kexec_load_check(unsigned long nr_segments,
> >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> >               return -EPERM;
> >
> > +     /* Check if the system admin has disabled kexec reboot. */
> > +     if (!(flags & KEXEC_ON_CRASH) && kexec_reboot_disabled)
> > +             return -EPERM;
>
> ... Allowing to load a crashkernel doesn't make sense in my opinion. If
> an attacker is capable of creating a malicious kernel, planting it on
> the victims system and then find a way to boot it via kexec this
> attacker also knows how to load the malicious kernel as crashkernel and
> trigger a panic. So you haven't really gained anything. That's why I
> would simply drop this hunk (and the corresponding one from
> kexec_file_load) and let users who worry about this use a combination of
> kexec_load_disabled and kexec_reboot_disabled.

If for whatever reason your sysadmin configured kexec_reboot_disabed
it can be nice that when a user try to load it they get a warning.
It is easier to debug than waiting two steps later when they run kexec -e....

That is why I added it. But i am also ok removing it

>
> Thanks
> Philipp
>
> > +
> >       /* Permit LSMs and IMA to fail the kexec */
> >       result = security_kernel_load_data(LOADING_KEXEC_IMAGE, false);
> >       if (result < 0)
> > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > index ca2743f9c634..fe82e2525705 100644
> > --- a/kernel/kexec_core.c
> > +++ b/kernel/kexec_core.c
> > @@ -929,6 +929,7 @@ int kimage_load_segment(struct kimage *image,
> >  struct kimage *kexec_image;
> >  struct kimage *kexec_crash_image;
> >  int kexec_load_disabled;
> > +int kexec_reboot_disabled;
> >  #ifdef CONFIG_SYSCTL
> >  static struct ctl_table kexec_core_sysctls[] = {
> >       {
> > @@ -941,6 +942,16 @@ static struct ctl_table kexec_core_sysctls[] = {
> >               .extra1         = SYSCTL_ONE,
> >               .extra2         = SYSCTL_ONE,
> >       },
> > +     {
> > +             .procname       = "kexec_reboot_disabled",
> > +             .data           = &kexec_reboot_disabled,
> > +             .maxlen         = sizeof(int),
> > +             .mode           = 0644,
> > +             /* only handle a transition from default "0" to "1" */
> > +             .proc_handler   = proc_dointvec_minmax,
> > +             .extra1         = SYSCTL_ONE,
> > +             .extra2         = SYSCTL_ONE,
> > +     },
> >       { }
> >  };
> >
> > @@ -1138,7 +1149,7 @@ int kernel_kexec(void)
> >
> >       if (!kexec_trylock())
> >               return -EBUSY;
> > -     if (!kexec_image) {
> > +     if (!kexec_image || kexec_reboot_disabled) {
> >               error = -EINVAL;
> >               goto Unlock;
> >       }
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index 45637511e0de..583fba6de5cb 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -333,6 +333,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> >               return -EPERM;
> >
> > +     /* Check if the system admin has disabled kexec reboot. */
> > +     if (!(flags & (KEXEC_FILE_ON_CRASH | KEXEC_FILE_UNLOAD))
> > +         && kexec_reboot_disabled)
> > +             return -EPERM;
> > +
> >       /* Make sure we have a legal set of flags */
> >       if (flags != (flags & KEXEC_FILE_FLAGS))
> >               return -EINVAL;
> >
>
  

Hi Ricardo,

On Thu, 17 Nov 2022 16:15:07 +0100
Ricardo Ribalda <ribalda@chromium.org> wrote:

> Hi Philipp
> 
> Thanks for your review!

happy to help.

> 
> On Thu, 17 Nov 2022 at 16:07, Philipp Rudo <prudo@redhat.com> wrote:
> >
> > Hi Ricardo,
> >
> > all in all I think this patch makes sense. However, there is one point
> > I don't like...
> >
> > On Mon, 14 Nov 2022 14:18:39 +0100
> > Ricardo Ribalda <ribalda@chromium.org> wrote:
> >  
> > > Create a new toogle that disables LINUX_REBOOT_CMD_KEXEC, reducing the
> > > attack surface to a system.
> > >
> > > Without this toogle, an attacker can only reboot into a different kernel
> > > if they can create a panic().
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > >
> > > diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> > > index 97394bd9d065..25d019682d33 100644
> > > --- a/Documentation/admin-guide/sysctl/kernel.rst
> > > +++ b/Documentation/admin-guide/sysctl/kernel.rst
> > > @@ -462,6 +462,17 @@ altered.
> > >  Generally used together with the `modules_disabled`_ sysctl.
> > >
> > >
> > > +kexec_reboot_disabled
> > > +=====================
> > > +
> > > +A toggle indicating if ``LINUX_REBOOT_CMD_KEXEC`` has been disabled.
> > > +This value defaults to 0 (false: ``LINUX_REBOOT_CMD_KEXEC`` enabled),
> > > +but can be set to 1 (true: ``LINUX_REBOOT_CMD_KEXEC`` disabled).
> > > +Once true, kexec can no longer be used for reboot and the toggle
> > > +cannot be set back to false.
> > > +This toggle does not affect the use of kexec during a crash.
> > > +
> > > +
> > >  kptr_restrict
> > >  =============
> > >
> > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > > index 41a686996aaa..15c3fad8918b 100644
> > > --- a/include/linux/kexec.h
> > > +++ b/include/linux/kexec.h
> > > @@ -407,6 +407,7 @@ extern int kimage_crash_copy_vmcoreinfo(struct kimage *image);
> > >  extern struct kimage *kexec_image;
> > >  extern struct kimage *kexec_crash_image;
> > >  extern int kexec_load_disabled;
> > > +extern int kexec_reboot_disabled;
> > >
> > >  #ifndef kexec_flush_icache_page
> > >  #define kexec_flush_icache_page(page)
> > > diff --git a/kernel/kexec.c b/kernel/kexec.c
> > > index cb8e6e6f983c..43063f803d81 100644
> > > --- a/kernel/kexec.c
> > > +++ b/kernel/kexec.c
> > > @@ -196,6 +196,10 @@ static inline int kexec_load_check(unsigned long nr_segments,
> > >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> > >               return -EPERM;
> > >
> > > +     /* Check if the system admin has disabled kexec reboot. */
> > > +     if (!(flags & KEXEC_ON_CRASH) && kexec_reboot_disabled)
> > > +             return -EPERM;  
> >
> > ... Allowing to load a crashkernel doesn't make sense in my opinion. If
> > an attacker is capable of creating a malicious kernel, planting it on
> > the victims system and then find a way to boot it via kexec this
> > attacker also knows how to load the malicious kernel as crashkernel and
> > trigger a panic. So you haven't really gained anything. That's why I
> > would simply drop this hunk (and the corresponding one from
> > kexec_file_load) and let users who worry about this use a combination of
> > kexec_load_disabled and kexec_reboot_disabled.  
> 
> If for whatever reason your sysadmin configured kexec_reboot_disabed
> it can be nice that when a user try to load it they get a warning.
> It is easier to debug than waiting two steps later when they run kexec -e....

I'm having second thoughts about this patch. My main problem is that I
don't see a real use case where kexec_reboot_disabled is advantageous
over kexec_load_disabled. The point is that disabling
LINUX_REBOOT_CMD_KEXEC is almost identical to toggling kexec_load_disabled without
a loaded kernel (when you don't have a kernel loaded you cannot reboot
into it). With this the main use case of kexec_reboot_disabled is
already covered by kexec_load_disabled.

However, there are two differences

1) with kexec_reboot_disable you can still (re-)load a crash kernel
e.g. to update the initramfs after a config change. But as discussed in
my first mail this comes on the cost that an attacker could still load a
malicious crash kernel and then 'panic into it'.
 
2) kexec_load_disabled also prevents unloading of a loaded kernel. So
once loaded kexec_load_disabled cannot prevent the reboot into this
kernel.


For 1) I doubt that this is desired at all. My expectation is that on
systems where a sysadmin restricts a user to reboot via kexec the
sysadmin also wants to prevent the user to load an arbitrary crash
kernel. Especially as this still keeps the loophole open you are trying
to close.

So only 2) is left as real benefit. But that is an extremely specific
scenario. How often does this scenario happen in real life? What
problem does kexec_reboot_disable solve different implementation
(also in userspace) cannot?

Sorry about being this pedantic but you want to introduce some new uapi
which will be hard if not impossible to change once introduced. That's
why I want to be a 100% sure it is really needed.

Thanks
Philipp


> That is why I added it. But i am also ok removing it
> 
> >
> > Thanks
> > Philipp
> >  
> > > +
> > >       /* Permit LSMs and IMA to fail the kexec */
> > >       result = security_kernel_load_data(LOADING_KEXEC_IMAGE, false);
> > >       if (result < 0)
> > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > > index ca2743f9c634..fe82e2525705 100644
> > > --- a/kernel/kexec_core.c
> > > +++ b/kernel/kexec_core.c
> > > @@ -929,6 +929,7 @@ int kimage_load_segment(struct kimage *image,
> > >  struct kimage *kexec_image;
> > >  struct kimage *kexec_crash_image;
> > >  int kexec_load_disabled;
> > > +int kexec_reboot_disabled;
> > >  #ifdef CONFIG_SYSCTL
> > >  static struct ctl_table kexec_core_sysctls[] = {
> > >       {
> > > @@ -941,6 +942,16 @@ static struct ctl_table kexec_core_sysctls[] = {
> > >               .extra1         = SYSCTL_ONE,
> > >               .extra2         = SYSCTL_ONE,
> > >       },
> > > +     {
> > > +             .procname       = "kexec_reboot_disabled",
> > > +             .data           = &kexec_reboot_disabled,
> > > +             .maxlen         = sizeof(int),
> > > +             .mode           = 0644,
> > > +             /* only handle a transition from default "0" to "1" */
> > > +             .proc_handler   = proc_dointvec_minmax,
> > > +             .extra1         = SYSCTL_ONE,
> > > +             .extra2         = SYSCTL_ONE,
> > > +     },
> > >       { }
> > >  };
> > >
> > > @@ -1138,7 +1149,7 @@ int kernel_kexec(void)
> > >
> > >       if (!kexec_trylock())
> > >               return -EBUSY;
> > > -     if (!kexec_image) {
> > > +     if (!kexec_image || kexec_reboot_disabled) {
> > >               error = -EINVAL;
> > >               goto Unlock;
> > >       }
> > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > index 45637511e0de..583fba6de5cb 100644
> > > --- a/kernel/kexec_file.c
> > > +++ b/kernel/kexec_file.c
> > > @@ -333,6 +333,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> > >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> > >               return -EPERM;
> > >
> > > +     /* Check if the system admin has disabled kexec reboot. */
> > > +     if (!(flags & (KEXEC_FILE_ON_CRASH | KEXEC_FILE_UNLOAD))
> > > +         && kexec_reboot_disabled)
> > > +             return -EPERM;
> > > +
> > >       /* Make sure we have a legal set of flags */
> > >       if (flags != (flags & KEXEC_FILE_FLAGS))
> > >               return -EINVAL;
> > >  
> >  
> 
>
  

Hi Philipp

Thanks for your review.

My scenario is a trusted system, where even if you are root, your
access to the system is very limited.

Let's assume LOADPIN and verity are enabled.

On Mon, 21 Nov 2022 at 15:10, Philipp Rudo <prudo@redhat.com> wrote:
>
> Hi Ricardo,
>
> On Thu, 17 Nov 2022 16:15:07 +0100
> Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> > Hi Philipp
> >
> > Thanks for your review!
>
> happy to help.
>
> >
> > On Thu, 17 Nov 2022 at 16:07, Philipp Rudo <prudo@redhat.com> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > all in all I think this patch makes sense. However, there is one point
> > > I don't like...
> > >
> > > On Mon, 14 Nov 2022 14:18:39 +0100
> > > Ricardo Ribalda <ribalda@chromium.org> wrote:
> > >
> > > > Create a new toogle that disables LINUX_REBOOT_CMD_KEXEC, reducing the
> > > > attack surface to a system.
> > > >
> > > > Without this toogle, an attacker can only reboot into a different kernel
> > > > if they can create a panic().
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > >
> > > > diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> > > > index 97394bd9d065..25d019682d33 100644
> > > > --- a/Documentation/admin-guide/sysctl/kernel.rst
> > > > +++ b/Documentation/admin-guide/sysctl/kernel.rst
> > > > @@ -462,6 +462,17 @@ altered.
> > > >  Generally used together with the `modules_disabled`_ sysctl.
> > > >
> > > >
> > > > +kexec_reboot_disabled
> > > > +=====================
> > > > +
> > > > +A toggle indicating if ``LINUX_REBOOT_CMD_KEXEC`` has been disabled.
> > > > +This value defaults to 0 (false: ``LINUX_REBOOT_CMD_KEXEC`` enabled),
> > > > +but can be set to 1 (true: ``LINUX_REBOOT_CMD_KEXEC`` disabled).
> > > > +Once true, kexec can no longer be used for reboot and the toggle
> > > > +cannot be set back to false.
> > > > +This toggle does not affect the use of kexec during a crash.
> > > > +
> > > > +
> > > >  kptr_restrict
> > > >  =============
> > > >
> > > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > > > index 41a686996aaa..15c3fad8918b 100644
> > > > --- a/include/linux/kexec.h
> > > > +++ b/include/linux/kexec.h
> > > > @@ -407,6 +407,7 @@ extern int kimage_crash_copy_vmcoreinfo(struct kimage *image);
> > > >  extern struct kimage *kexec_image;
> > > >  extern struct kimage *kexec_crash_image;
> > > >  extern int kexec_load_disabled;
> > > > +extern int kexec_reboot_disabled;
> > > >
> > > >  #ifndef kexec_flush_icache_page
> > > >  #define kexec_flush_icache_page(page)
> > > > diff --git a/kernel/kexec.c b/kernel/kexec.c
> > > > index cb8e6e6f983c..43063f803d81 100644
> > > > --- a/kernel/kexec.c
> > > > +++ b/kernel/kexec.c
> > > > @@ -196,6 +196,10 @@ static inline int kexec_load_check(unsigned long nr_segments,
> > > >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> > > >               return -EPERM;
> > > >
> > > > +     /* Check if the system admin has disabled kexec reboot. */
> > > > +     if (!(flags & KEXEC_ON_CRASH) && kexec_reboot_disabled)
> > > > +             return -EPERM;
> > >
> > > ... Allowing to load a crashkernel doesn't make sense in my opinion. If
> > > an attacker is capable of creating a malicious kernel, planting it on
> > > the victims system and then find a way to boot it via kexec this
> > > attacker also knows how to load the malicious kernel as crashkernel and
> > > trigger a panic. So you haven't really gained anything. That's why I
> > > would simply drop this hunk (and the corresponding one from
> > > kexec_file_load) and let users who worry about this use a combination of
> > > kexec_load_disabled and kexec_reboot_disabled.
> >
> > If for whatever reason your sysadmin configured kexec_reboot_disabed
> > it can be nice that when a user try to load it they get a warning.
> > It is easier to debug than waiting two steps later when they run kexec -e....
>
> I'm having second thoughts about this patch. My main problem is that I
> don't see a real use case where kexec_reboot_disabled is advantageous
> over kexec_load_disabled. The point is that disabling
> LINUX_REBOOT_CMD_KEXEC is almost identical to toggling kexec_load_disabled without
> a loaded kernel (when you don't have a kernel loaded you cannot reboot
> into it). With this the main use case of kexec_reboot_disabled is
> already covered by kexec_load_disabled.

>
> However, there are two differences
>
> 1) with kexec_reboot_disable you can still (re-)load a crash kernel
> e.g. to update the initramfs after a config change. But as discussed in
> my first mail this comes on the cost that an attacker could still load a
> malicious crash kernel and then 'panic into it'.

That crash kernel must be already in the signed malicious kernel.
which reduces the chances of attack.
Plus an attacker must be able to panic the current kernel at will,
instead of just call reset.

>
> 2) kexec_load_disabled also prevents unloading of a loaded kernel. So
> once loaded kexec_load_disabled cannot prevent the reboot into this
> kernel.
>
>
> For 1) I doubt that this is desired at all. My expectation is that on
> systems where a sysadmin restricts a user to reboot via kexec the
> sysadmin also wants to prevent the user to load an arbitrary crash
> kernel. Especially as this still keeps the loophole open you are trying
> to close.
>
> So only 2) is left as real benefit. But that is an extremely specific
> scenario. How often does this scenario happen in real life? What
> problem does kexec_reboot_disable solve different implementation
> (also in userspace) cannot?
>
> Sorry about being this pedantic but you want to introduce some new uapi
> which will be hard if not impossible to change once introduced. That's
> why I want to be a 100% sure it is really needed.

No worries. Completely understand :). Thanks for taking this seriously..


Best regards!
>
> Thanks
> Philipp
>
>
> > That is why I added it. But i am also ok removing it
> >
> > >
> > > Thanks
> > > Philipp
> > >
> > > > +
> > > >       /* Permit LSMs and IMA to fail the kexec */
> > > >       result = security_kernel_load_data(LOADING_KEXEC_IMAGE, false);
> > > >       if (result < 0)
> > > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > > > index ca2743f9c634..fe82e2525705 100644
> > > > --- a/kernel/kexec_core.c
> > > > +++ b/kernel/kexec_core.c
> > > > @@ -929,6 +929,7 @@ int kimage_load_segment(struct kimage *image,
> > > >  struct kimage *kexec_image;
> > > >  struct kimage *kexec_crash_image;
> > > >  int kexec_load_disabled;
> > > > +int kexec_reboot_disabled;
> > > >  #ifdef CONFIG_SYSCTL
> > > >  static struct ctl_table kexec_core_sysctls[] = {
> > > >       {
> > > > @@ -941,6 +942,16 @@ static struct ctl_table kexec_core_sysctls[] = {
> > > >               .extra1         = SYSCTL_ONE,
> > > >               .extra2         = SYSCTL_ONE,
> > > >       },
> > > > +     {
> > > > +             .procname       = "kexec_reboot_disabled",
> > > > +             .data           = &kexec_reboot_disabled,
> > > > +             .maxlen         = sizeof(int),
> > > > +             .mode           = 0644,
> > > > +             /* only handle a transition from default "0" to "1" */
> > > > +             .proc_handler   = proc_dointvec_minmax,
> > > > +             .extra1         = SYSCTL_ONE,
> > > > +             .extra2         = SYSCTL_ONE,
> > > > +     },
> > > >       { }
> > > >  };
> > > >
> > > > @@ -1138,7 +1149,7 @@ int kernel_kexec(void)
> > > >
> > > >       if (!kexec_trylock())
> > > >               return -EBUSY;
> > > > -     if (!kexec_image) {
> > > > +     if (!kexec_image || kexec_reboot_disabled) {
> > > >               error = -EINVAL;
> > > >               goto Unlock;
> > > >       }
> > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > index 45637511e0de..583fba6de5cb 100644
> > > > --- a/kernel/kexec_file.c
> > > > +++ b/kernel/kexec_file.c
> > > > @@ -333,6 +333,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> > > >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> > > >               return -EPERM;
> > > >
> > > > +     /* Check if the system admin has disabled kexec reboot. */
> > > > +     if (!(flags & (KEXEC_FILE_ON_CRASH | KEXEC_FILE_UNLOAD))
> > > > +         && kexec_reboot_disabled)
> > > > +             return -EPERM;
> > > > +
> > > >       /* Make sure we have a legal set of flags */
> > > >       if (flags != (flags & KEXEC_FILE_FLAGS))
> > > >               return -EINVAL;
> > > >
> > >
> >
> >
>
  

On 11/14/22 at 02:18pm, Ricardo Ribalda wrote:
> Create a new toogle that disables LINUX_REBOOT_CMD_KEXEC, reducing the
               ~ toggle
> attack surface to a system.
> 
> Without this toogle, an attacker can only reboot into a different kernel
  ~~s/without/with/
> if they can create a panic().

The log just tells what it's doing, but miss the most important why this
has to be needed, the motivation.

I roughly read the talking between you and Philipp, wondering why do you
believe panicked kernel, if you worry about the untrusted kernel kexec
rebooted into. People can change scripts in initramfs, e.g drop into
emergency shell and switch into rootfs, there are a lot of things people
can do in there too.

> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> 
> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> index 97394bd9d065..25d019682d33 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -462,6 +462,17 @@ altered.
>  Generally used together with the `modules_disabled`_ sysctl.
>  
>  
> +kexec_reboot_disabled
> +=====================
> +
> +A toggle indicating if ``LINUX_REBOOT_CMD_KEXEC`` has been disabled.
> +This value defaults to 0 (false: ``LINUX_REBOOT_CMD_KEXEC`` enabled),
> +but can be set to 1 (true: ``LINUX_REBOOT_CMD_KEXEC`` disabled).
> +Once true, kexec can no longer be used for reboot and the toggle
> +cannot be set back to false.
> +This toggle does not affect the use of kexec during a crash.
> +
> +
>  kptr_restrict
>  =============
>  
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 41a686996aaa..15c3fad8918b 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -407,6 +407,7 @@ extern int kimage_crash_copy_vmcoreinfo(struct kimage *image);
>  extern struct kimage *kexec_image;
>  extern struct kimage *kexec_crash_image;
>  extern int kexec_load_disabled;
> +extern int kexec_reboot_disabled;
>  
>  #ifndef kexec_flush_icache_page
>  #define kexec_flush_icache_page(page)
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index cb8e6e6f983c..43063f803d81 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -196,6 +196,10 @@ static inline int kexec_load_check(unsigned long nr_segments,
>  	if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
>  		return -EPERM;
>  
> +	/* Check if the system admin has disabled kexec reboot. */
> +	if (!(flags & KEXEC_ON_CRASH) && kexec_reboot_disabled)
> +		return -EPERM;
> +
>  	/* Permit LSMs and IMA to fail the kexec */
>  	result = security_kernel_load_data(LOADING_KEXEC_IMAGE, false);
>  	if (result < 0)
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index ca2743f9c634..fe82e2525705 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -929,6 +929,7 @@ int kimage_load_segment(struct kimage *image,
>  struct kimage *kexec_image;
>  struct kimage *kexec_crash_image;
>  int kexec_load_disabled;
> +int kexec_reboot_disabled;
>  #ifdef CONFIG_SYSCTL
>  static struct ctl_table kexec_core_sysctls[] = {
>  	{
> @@ -941,6 +942,16 @@ static struct ctl_table kexec_core_sysctls[] = {
>  		.extra1		= SYSCTL_ONE,
>  		.extra2		= SYSCTL_ONE,
>  	},
> +	{
> +		.procname	= "kexec_reboot_disabled",
> +		.data		= &kexec_reboot_disabled,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		/* only handle a transition from default "0" to "1" */
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= SYSCTL_ONE,
> +		.extra2		= SYSCTL_ONE,
> +	},
>  	{ }
>  };
>  
> @@ -1138,7 +1149,7 @@ int kernel_kexec(void)
>  
>  	if (!kexec_trylock())
>  		return -EBUSY;
> -	if (!kexec_image) {
> +	if (!kexec_image || kexec_reboot_disabled) {
>  		error = -EINVAL;
>  		goto Unlock;
>  	}
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 45637511e0de..583fba6de5cb 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -333,6 +333,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  	if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
>  		return -EPERM;
>  
> +	/* Check if the system admin has disabled kexec reboot. */
> +	if (!(flags & (KEXEC_FILE_ON_CRASH | KEXEC_FILE_UNLOAD))
> +	    && kexec_reboot_disabled)
> +		return -EPERM;
> +
>  	/* Make sure we have a legal set of flags */
>  	if (flags != (flags & KEXEC_FILE_FLAGS))
>  		return -EINVAL;
> 
> -- 
> b4 0.11.0-dev-d93f8
>
  

Hi Ricardo,

On Wed, 23 Nov 2022 09:58:08 +0100
Ricardo Ribalda <ribalda@chromium.org> wrote:

> Hi Philipp
> 
> Thanks for your review.
> 
> My scenario is a trusted system, where even if you are root, your
> access to the system is very limited.
> 
> Let's assume LOADPIN and verity are enabled.

My point is that on such systems I expect that a sysadmin also wants to
control the crash kernel including its initramfs (which also has to be part
of the signed kernel?). But if that's the case a sysadmin can simply arm
kdump early during boot and then toggle kexec_load_disabled. With that
LINUX_REBOOT_CMD_KEXEC also gets disabled as no kexec kernel can be loaded
while kdump works. Thus there is no need to add the new interface. Or am
I missing anything?

Thanks
Philipp

> 
> On Mon, 21 Nov 2022 at 15:10, Philipp Rudo <prudo@redhat.com> wrote:
> >
> > Hi Ricardo,
> >
> > On Thu, 17 Nov 2022 16:15:07 +0100
> > Ricardo Ribalda <ribalda@chromium.org> wrote:
> >  
> > > Hi Philipp
> > >
> > > Thanks for your review!  
> >
> > happy to help.
> >  
> > >
> > > On Thu, 17 Nov 2022 at 16:07, Philipp Rudo <prudo@redhat.com> wrote:  
> > > >
> > > > Hi Ricardo,
> > > >
> > > > all in all I think this patch makes sense. However, there is one point
> > > > I don't like...
> > > >
> > > > On Mon, 14 Nov 2022 14:18:39 +0100
> > > > Ricardo Ribalda <ribalda@chromium.org> wrote:
> > > >  
> > > > > Create a new toogle that disables LINUX_REBOOT_CMD_KEXEC, reducing the
> > > > > attack surface to a system.
> > > > >
> > > > > Without this toogle, an attacker can only reboot into a different kernel
> > > > > if they can create a panic().
> > > > >
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > >
> > > > > diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> > > > > index 97394bd9d065..25d019682d33 100644
> > > > > --- a/Documentation/admin-guide/sysctl/kernel.rst
> > > > > +++ b/Documentation/admin-guide/sysctl/kernel.rst
> > > > > @@ -462,6 +462,17 @@ altered.
> > > > >  Generally used together with the `modules_disabled`_ sysctl.
> > > > >
> > > > >
> > > > > +kexec_reboot_disabled
> > > > > +=====================
> > > > > +
> > > > > +A toggle indicating if ``LINUX_REBOOT_CMD_KEXEC`` has been disabled.
> > > > > +This value defaults to 0 (false: ``LINUX_REBOOT_CMD_KEXEC`` enabled),
> > > > > +but can be set to 1 (true: ``LINUX_REBOOT_CMD_KEXEC`` disabled).
> > > > > +Once true, kexec can no longer be used for reboot and the toggle
> > > > > +cannot be set back to false.
> > > > > +This toggle does not affect the use of kexec during a crash.
> > > > > +
> > > > > +
> > > > >  kptr_restrict
> > > > >  =============
> > > > >
> > > > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > > > > index 41a686996aaa..15c3fad8918b 100644
> > > > > --- a/include/linux/kexec.h
> > > > > +++ b/include/linux/kexec.h
> > > > > @@ -407,6 +407,7 @@ extern int kimage_crash_copy_vmcoreinfo(struct kimage *image);
> > > > >  extern struct kimage *kexec_image;
> > > > >  extern struct kimage *kexec_crash_image;
> > > > >  extern int kexec_load_disabled;
> > > > > +extern int kexec_reboot_disabled;
> > > > >
> > > > >  #ifndef kexec_flush_icache_page
> > > > >  #define kexec_flush_icache_page(page)
> > > > > diff --git a/kernel/kexec.c b/kernel/kexec.c
> > > > > index cb8e6e6f983c..43063f803d81 100644
> > > > > --- a/kernel/kexec.c
> > > > > +++ b/kernel/kexec.c
> > > > > @@ -196,6 +196,10 @@ static inline int kexec_load_check(unsigned long nr_segments,
> > > > >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> > > > >               return -EPERM;
> > > > >
> > > > > +     /* Check if the system admin has disabled kexec reboot. */
> > > > > +     if (!(flags & KEXEC_ON_CRASH) && kexec_reboot_disabled)
> > > > > +             return -EPERM;  
> > > >
> > > > ... Allowing to load a crashkernel doesn't make sense in my opinion. If
> > > > an attacker is capable of creating a malicious kernel, planting it on
> > > > the victims system and then find a way to boot it via kexec this
> > > > attacker also knows how to load the malicious kernel as crashkernel and
> > > > trigger a panic. So you haven't really gained anything. That's why I
> > > > would simply drop this hunk (and the corresponding one from
> > > > kexec_file_load) and let users who worry about this use a combination of
> > > > kexec_load_disabled and kexec_reboot_disabled.  
> > >
> > > If for whatever reason your sysadmin configured kexec_reboot_disabed
> > > it can be nice that when a user try to load it they get a warning.
> > > It is easier to debug than waiting two steps later when they run kexec -e....  
> >
> > I'm having second thoughts about this patch. My main problem is that I
> > don't see a real use case where kexec_reboot_disabled is advantageous
> > over kexec_load_disabled. The point is that disabling
> > LINUX_REBOOT_CMD_KEXEC is almost identical to toggling kexec_load_disabled without
> > a loaded kernel (when you don't have a kernel loaded you cannot reboot
> > into it). With this the main use case of kexec_reboot_disabled is
> > already covered by kexec_load_disabled.  
> 
> >
> > However, there are two differences
> >
> > 1) with kexec_reboot_disable you can still (re-)load a crash kernel
> > e.g. to update the initramfs after a config change. But as discussed in
> > my first mail this comes on the cost that an attacker could still load a
> > malicious crash kernel and then 'panic into it'.  
> 
> That crash kernel must be already in the signed malicious kernel.
> which reduces the chances of attack.
> Plus an attacker must be able to panic the current kernel at will,
> instead of just call reset.
> 
> >
> > 2) kexec_load_disabled also prevents unloading of a loaded kernel. So
> > once loaded kexec_load_disabled cannot prevent the reboot into this
> > kernel.
> >
> >
> > For 1) I doubt that this is desired at all. My expectation is that on
> > systems where a sysadmin restricts a user to reboot via kexec the
> > sysadmin also wants to prevent the user to load an arbitrary crash
> > kernel. Especially as this still keeps the loophole open you are trying
> > to close.
> >
> > So only 2) is left as real benefit. But that is an extremely specific
> > scenario. How often does this scenario happen in real life? What
> > problem does kexec_reboot_disable solve different implementation
> > (also in userspace) cannot?
> >
> > Sorry about being this pedantic but you want to introduce some new uapi
> > which will be hard if not impossible to change once introduced. That's
> > why I want to be a 100% sure it is really needed.  
> 
> No worries. Completely understand :). Thanks for taking this seriously..
> 
> 
> Best regards!
> >
> > Thanks
> > Philipp
> >
> >  
> > > That is why I added it. But i am also ok removing it
> > >  
> > > >
> > > > Thanks
> > > > Philipp
> > > >  
> > > > > +
> > > > >       /* Permit LSMs and IMA to fail the kexec */
> > > > >       result = security_kernel_load_data(LOADING_KEXEC_IMAGE, false);
> > > > >       if (result < 0)
> > > > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > > > > index ca2743f9c634..fe82e2525705 100644
> > > > > --- a/kernel/kexec_core.c
> > > > > +++ b/kernel/kexec_core.c
> > > > > @@ -929,6 +929,7 @@ int kimage_load_segment(struct kimage *image,
> > > > >  struct kimage *kexec_image;
> > > > >  struct kimage *kexec_crash_image;
> > > > >  int kexec_load_disabled;
> > > > > +int kexec_reboot_disabled;
> > > > >  #ifdef CONFIG_SYSCTL
> > > > >  static struct ctl_table kexec_core_sysctls[] = {
> > > > >       {
> > > > > @@ -941,6 +942,16 @@ static struct ctl_table kexec_core_sysctls[] = {
> > > > >               .extra1         = SYSCTL_ONE,
> > > > >               .extra2         = SYSCTL_ONE,
> > > > >       },
> > > > > +     {
> > > > > +             .procname       = "kexec_reboot_disabled",
> > > > > +             .data           = &kexec_reboot_disabled,
> > > > > +             .maxlen         = sizeof(int),
> > > > > +             .mode           = 0644,
> > > > > +             /* only handle a transition from default "0" to "1" */
> > > > > +             .proc_handler   = proc_dointvec_minmax,
> > > > > +             .extra1         = SYSCTL_ONE,
> > > > > +             .extra2         = SYSCTL_ONE,
> > > > > +     },
> > > > >       { }
> > > > >  };
> > > > >
> > > > > @@ -1138,7 +1149,7 @@ int kernel_kexec(void)
> > > > >
> > > > >       if (!kexec_trylock())
> > > > >               return -EBUSY;
> > > > > -     if (!kexec_image) {
> > > > > +     if (!kexec_image || kexec_reboot_disabled) {
> > > > >               error = -EINVAL;
> > > > >               goto Unlock;
> > > > >       }
> > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > > index 45637511e0de..583fba6de5cb 100644
> > > > > --- a/kernel/kexec_file.c
> > > > > +++ b/kernel/kexec_file.c
> > > > > @@ -333,6 +333,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> > > > >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> > > > >               return -EPERM;
> > > > >
> > > > > +     /* Check if the system admin has disabled kexec reboot. */
> > > > > +     if (!(flags & (KEXEC_FILE_ON_CRASH | KEXEC_FILE_UNLOAD))
> > > > > +         && kexec_reboot_disabled)
> > > > > +             return -EPERM;
> > > > > +
> > > > >       /* Make sure we have a legal set of flags */
> > > > >       if (flags != (flags & KEXEC_FILE_FLAGS))
> > > > >               return -EINVAL;
> > > > >  
> > > >  
> > >
> > >  
> >  
> 
>
  

On Thu, 24 Nov 2022 at 12:40, Philipp Rudo <prudo@redhat.com> wrote:
>
> Hi Ricardo,
>
> On Wed, 23 Nov 2022 09:58:08 +0100
> Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> > Hi Philipp
> >
> > Thanks for your review.
> >
> > My scenario is a trusted system, where even if you are root, your
> > access to the system is very limited.
> >
> > Let's assume LOADPIN and verity are enabled.
>
> My point is that on such systems I expect that a sysadmin also wants to
> control the crash kernel including its initramfs (which also has to be part
> of the signed kernel?). But if that's the case a sysadmin can simply arm
> kdump early during boot and then toggle kexec_load_disabled. With that
> LINUX_REBOOT_CMD_KEXEC also gets disabled as no kexec kernel can be loaded
> while kdump works. Thus there is no need to add the new interface. Or am
> I missing anything?

Let's say that you have a script that does something like this


kexec -p dump_kernel
echo 1 > /proc/sys/kernel/kexec_load_disabled

If an attacker can DDos the system and make that script crash... then
kexec is still accessible

On the other hand, if you load the kernel with the commandline

sysctl.kernel.kexec_load_disabled=1

Then even if the script crashes, the only way to abuse kexec is by
panicing the running kernel....


Would it make you more comfortable if I model this as a kernel config
instead of a runtime option?

Thanks!


>
> Thanks
> Philipp
>
> >
> > On Mon, 21 Nov 2022 at 15:10, Philipp Rudo <prudo@redhat.com> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > On Thu, 17 Nov 2022 16:15:07 +0100
> > > Ricardo Ribalda <ribalda@chromium.org> wrote:
> > >
> > > > Hi Philipp
> > > >
> > > > Thanks for your review!
> > >
> > > happy to help.
> > >
> > > >
> > > > On Thu, 17 Nov 2022 at 16:07, Philipp Rudo <prudo@redhat.com> wrote:
> > > > >
> > > > > Hi Ricardo,
> > > > >
> > > > > all in all I think this patch makes sense. However, there is one point
> > > > > I don't like...
> > > > >
> > > > > On Mon, 14 Nov 2022 14:18:39 +0100
> > > > > Ricardo Ribalda <ribalda@chromium.org> wrote:
> > > > >
> > > > > > Create a new toogle that disables LINUX_REBOOT_CMD_KEXEC, reducing the
> > > > > > attack surface to a system.
> > > > > >
> > > > > > Without this toogle, an attacker can only reboot into a different kernel
> > > > > > if they can create a panic().
> > > > > >
> > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > >
> > > > > > diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> > > > > > index 97394bd9d065..25d019682d33 100644
> > > > > > --- a/Documentation/admin-guide/sysctl/kernel.rst
> > > > > > +++ b/Documentation/admin-guide/sysctl/kernel.rst
> > > > > > @@ -462,6 +462,17 @@ altered.
> > > > > >  Generally used together with the `modules_disabled`_ sysctl.
> > > > > >
> > > > > >
> > > > > > +kexec_reboot_disabled
> > > > > > +=====================
> > > > > > +
> > > > > > +A toggle indicating if ``LINUX_REBOOT_CMD_KEXEC`` has been disabled.
> > > > > > +This value defaults to 0 (false: ``LINUX_REBOOT_CMD_KEXEC`` enabled),
> > > > > > +but can be set to 1 (true: ``LINUX_REBOOT_CMD_KEXEC`` disabled).
> > > > > > +Once true, kexec can no longer be used for reboot and the toggle
> > > > > > +cannot be set back to false.
> > > > > > +This toggle does not affect the use of kexec during a crash.
> > > > > > +
> > > > > > +
> > > > > >  kptr_restrict
> > > > > >  =============
> > > > > >
> > > > > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > > > > > index 41a686996aaa..15c3fad8918b 100644
> > > > > > --- a/include/linux/kexec.h
> > > > > > +++ b/include/linux/kexec.h
> > > > > > @@ -407,6 +407,7 @@ extern int kimage_crash_copy_vmcoreinfo(struct kimage *image);
> > > > > >  extern struct kimage *kexec_image;
> > > > > >  extern struct kimage *kexec_crash_image;
> > > > > >  extern int kexec_load_disabled;
> > > > > > +extern int kexec_reboot_disabled;
> > > > > >
> > > > > >  #ifndef kexec_flush_icache_page
> > > > > >  #define kexec_flush_icache_page(page)
> > > > > > diff --git a/kernel/kexec.c b/kernel/kexec.c
> > > > > > index cb8e6e6f983c..43063f803d81 100644
> > > > > > --- a/kernel/kexec.c
> > > > > > +++ b/kernel/kexec.c
> > > > > > @@ -196,6 +196,10 @@ static inline int kexec_load_check(unsigned long nr_segments,
> > > > > >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> > > > > >               return -EPERM;
> > > > > >
> > > > > > +     /* Check if the system admin has disabled kexec reboot. */
> > > > > > +     if (!(flags & KEXEC_ON_CRASH) && kexec_reboot_disabled)
> > > > > > +             return -EPERM;
> > > > >
> > > > > ... Allowing to load a crashkernel doesn't make sense in my opinion. If
> > > > > an attacker is capable of creating a malicious kernel, planting it on
> > > > > the victims system and then find a way to boot it via kexec this
> > > > > attacker also knows how to load the malicious kernel as crashkernel and
> > > > > trigger a panic. So you haven't really gained anything. That's why I
> > > > > would simply drop this hunk (and the corresponding one from
> > > > > kexec_file_load) and let users who worry about this use a combination of
> > > > > kexec_load_disabled and kexec_reboot_disabled.
> > > >
> > > > If for whatever reason your sysadmin configured kexec_reboot_disabed
> > > > it can be nice that when a user try to load it they get a warning.
> > > > It is easier to debug than waiting two steps later when they run kexec -e....
> > >
> > > I'm having second thoughts about this patch. My main problem is that I
> > > don't see a real use case where kexec_reboot_disabled is advantageous
> > > over kexec_load_disabled. The point is that disabling
> > > LINUX_REBOOT_CMD_KEXEC is almost identical to toggling kexec_load_disabled without
> > > a loaded kernel (when you don't have a kernel loaded you cannot reboot
> > > into it). With this the main use case of kexec_reboot_disabled is
> > > already covered by kexec_load_disabled.
> >
> > >
> > > However, there are two differences
> > >
> > > 1) with kexec_reboot_disable you can still (re-)load a crash kernel
> > > e.g. to update the initramfs after a config change. But as discussed in
> > > my first mail this comes on the cost that an attacker could still load a
> > > malicious crash kernel and then 'panic into it'.
> >
> > That crash kernel must be already in the signed malicious kernel.
> > which reduces the chances of attack.
> > Plus an attacker must be able to panic the current kernel at will,
> > instead of just call reset.
> >
> > >
> > > 2) kexec_load_disabled also prevents unloading of a loaded kernel. So
> > > once loaded kexec_load_disabled cannot prevent the reboot into this
> > > kernel.
> > >
> > >
> > > For 1) I doubt that this is desired at all. My expectation is that on
> > > systems where a sysadmin restricts a user to reboot via kexec the
> > > sysadmin also wants to prevent the user to load an arbitrary crash
> > > kernel. Especially as this still keeps the loophole open you are trying
> > > to close.
> > >
> > > So only 2) is left as real benefit. But that is an extremely specific
> > > scenario. How often does this scenario happen in real life? What
> > > problem does kexec_reboot_disable solve different implementation
> > > (also in userspace) cannot?
> > >
> > > Sorry about being this pedantic but you want to introduce some new uapi
> > > which will be hard if not impossible to change once introduced. That's
> > > why I want to be a 100% sure it is really needed.
> >
> > No worries. Completely understand :). Thanks for taking this seriously..
> >
> >
> > Best regards!
> > >
> > > Thanks
> > > Philipp
> > >
> > >
> > > > That is why I added it. But i am also ok removing it
> > > >
> > > > >
> > > > > Thanks
> > > > > Philipp
> > > > >
> > > > > > +
> > > > > >       /* Permit LSMs and IMA to fail the kexec */
> > > > > >       result = security_kernel_load_data(LOADING_KEXEC_IMAGE, false);
> > > > > >       if (result < 0)
> > > > > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > > > > > index ca2743f9c634..fe82e2525705 100644
> > > > > > --- a/kernel/kexec_core.c
> > > > > > +++ b/kernel/kexec_core.c
> > > > > > @@ -929,6 +929,7 @@ int kimage_load_segment(struct kimage *image,
> > > > > >  struct kimage *kexec_image;
> > > > > >  struct kimage *kexec_crash_image;
> > > > > >  int kexec_load_disabled;
> > > > > > +int kexec_reboot_disabled;
> > > > > >  #ifdef CONFIG_SYSCTL
> > > > > >  static struct ctl_table kexec_core_sysctls[] = {
> > > > > >       {
> > > > > > @@ -941,6 +942,16 @@ static struct ctl_table kexec_core_sysctls[] = {
> > > > > >               .extra1         = SYSCTL_ONE,
> > > > > >               .extra2         = SYSCTL_ONE,
> > > > > >       },
> > > > > > +     {
> > > > > > +             .procname       = "kexec_reboot_disabled",
> > > > > > +             .data           = &kexec_reboot_disabled,
> > > > > > +             .maxlen         = sizeof(int),
> > > > > > +             .mode           = 0644,
> > > > > > +             /* only handle a transition from default "0" to "1" */
> > > > > > +             .proc_handler   = proc_dointvec_minmax,
> > > > > > +             .extra1         = SYSCTL_ONE,
> > > > > > +             .extra2         = SYSCTL_ONE,
> > > > > > +     },
> > > > > >       { }
> > > > > >  };
> > > > > >
> > > > > > @@ -1138,7 +1149,7 @@ int kernel_kexec(void)
> > > > > >
> > > > > >       if (!kexec_trylock())
> > > > > >               return -EBUSY;
> > > > > > -     if (!kexec_image) {
> > > > > > +     if (!kexec_image || kexec_reboot_disabled) {
> > > > > >               error = -EINVAL;
> > > > > >               goto Unlock;
> > > > > >       }
> > > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > > > index 45637511e0de..583fba6de5cb 100644
> > > > > > --- a/kernel/kexec_file.c
> > > > > > +++ b/kernel/kexec_file.c
> > > > > > @@ -333,6 +333,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> > > > > >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> > > > > >               return -EPERM;
> > > > > >
> > > > > > +     /* Check if the system admin has disabled kexec reboot. */
> > > > > > +     if (!(flags & (KEXEC_FILE_ON_CRASH | KEXEC_FILE_UNLOAD))
> > > > > > +         && kexec_reboot_disabled)
> > > > > > +             return -EPERM;
> > > > > > +
> > > > > >       /* Make sure we have a legal set of flags */
> > > > > >       if (flags != (flags & KEXEC_FILE_FLAGS))
> > > > > >               return -EINVAL;
> > > > > >
> > > > >
> > > >
> > > >
> > >
> >
> >
>
  

On Thu, 24 Nov 2022 13:52:58 +0100
Ricardo Ribalda <ribalda@chromium.org> wrote:

> On Thu, 24 Nov 2022 at 12:40, Philipp Rudo <prudo@redhat.com> wrote:
> >
> > Hi Ricardo,
> >
> > On Wed, 23 Nov 2022 09:58:08 +0100
> > Ricardo Ribalda <ribalda@chromium.org> wrote:
> >  
> > > Hi Philipp
> > >
> > > Thanks for your review.
> > >
> > > My scenario is a trusted system, where even if you are root, your
> > > access to the system is very limited.
> > >
> > > Let's assume LOADPIN and verity are enabled.  
> >
> > My point is that on such systems I expect that a sysadmin also wants to
> > control the crash kernel including its initramfs (which also has to be part
> > of the signed kernel?). But if that's the case a sysadmin can simply arm
> > kdump early during boot and then toggle kexec_load_disabled. With that
> > LINUX_REBOOT_CMD_KEXEC also gets disabled as no kexec kernel can be loaded
> > while kdump works. Thus there is no need to add the new interface. Or am
> > I missing anything?  
> 
> Let's say that you have a script that does something like this
> 
> 
> kexec -p dump_kernel
> echo 1 > /proc/sys/kernel/kexec_load_disabled
> 
> If an attacker can DDos the system and make that script crash... then
> kexec is still accessible
> 
> On the other hand, if you load the kernel with the commandline
> 
> sysctl.kernel.kexec_load_disabled=1
		      ^^^^
		      reboot?
Otherwise you shouldn't be able to load the crash kernel at all.

> Then even if the script crashes, the only way to abuse kexec is by
> panicing the running kernel....

True. But  when an attacker can DDos the system the final workload is
already running. So wouldn't it be enough to make sure that the script
above has finished before starting you workload. E.g. by setting an
appropriate Before=/After= in the systemd.unit?

Furthermore, I don't think that restricting kexec reboot alone is
sufficient when the attacker can still control the crash kernel. At
least my assumption is that triggering a panic instead of just
rebooting is just a mild inconvenience for somebody who is able to pull
off an attack like that.

> Would it make you more comfortable if I model this as a kernel config
> instead of a runtime option?

No, I think the implementation is fine. I'm currently only struggling
to understand what problem kexec_reboot_disabled solves that cannot be
solved by kexec_load_disabled.

> Thanks!

Happy to help.

Thanks
Philipp

> 
> 
> >
> > Thanks
> > Philipp
> >  
> > >
> > > On Mon, 21 Nov 2022 at 15:10, Philipp Rudo <prudo@redhat.com> wrote:  
> > > >
> > > > Hi Ricardo,
> > > >
> > > > On Thu, 17 Nov 2022 16:15:07 +0100
> > > > Ricardo Ribalda <ribalda@chromium.org> wrote:
> > > >  
> > > > > Hi Philipp
> > > > >
> > > > > Thanks for your review!  
> > > >
> > > > happy to help.
> > > >  
> > > > >
> > > > > On Thu, 17 Nov 2022 at 16:07, Philipp Rudo <prudo@redhat.com> wrote:  
> > > > > >
> > > > > > Hi Ricardo,
> > > > > >
> > > > > > all in all I think this patch makes sense. However, there is one point
> > > > > > I don't like...
> > > > > >
> > > > > > On Mon, 14 Nov 2022 14:18:39 +0100
> > > > > > Ricardo Ribalda <ribalda@chromium.org> wrote:
> > > > > >  
> > > > > > > Create a new toogle that disables LINUX_REBOOT_CMD_KEXEC, reducing the
> > > > > > > attack surface to a system.
> > > > > > >
> > > > > > > Without this toogle, an attacker can only reboot into a different kernel
> > > > > > > if they can create a panic().
> > > > > > >
> > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > >
> > > > > > > diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> > > > > > > index 97394bd9d065..25d019682d33 100644
> > > > > > > --- a/Documentation/admin-guide/sysctl/kernel.rst
> > > > > > > +++ b/Documentation/admin-guide/sysctl/kernel.rst
> > > > > > > @@ -462,6 +462,17 @@ altered.
> > > > > > >  Generally used together with the `modules_disabled`_ sysctl.
> > > > > > >
> > > > > > >
> > > > > > > +kexec_reboot_disabled
> > > > > > > +=====================
> > > > > > > +
> > > > > > > +A toggle indicating if ``LINUX_REBOOT_CMD_KEXEC`` has been disabled.
> > > > > > > +This value defaults to 0 (false: ``LINUX_REBOOT_CMD_KEXEC`` enabled),
> > > > > > > +but can be set to 1 (true: ``LINUX_REBOOT_CMD_KEXEC`` disabled).
> > > > > > > +Once true, kexec can no longer be used for reboot and the toggle
> > > > > > > +cannot be set back to false.
> > > > > > > +This toggle does not affect the use of kexec during a crash.
> > > > > > > +
> > > > > > > +
> > > > > > >  kptr_restrict
> > > > > > >  =============
> > > > > > >
> > > > > > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > > > > > > index 41a686996aaa..15c3fad8918b 100644
> > > > > > > --- a/include/linux/kexec.h
> > > > > > > +++ b/include/linux/kexec.h
> > > > > > > @@ -407,6 +407,7 @@ extern int kimage_crash_copy_vmcoreinfo(struct kimage *image);
> > > > > > >  extern struct kimage *kexec_image;
> > > > > > >  extern struct kimage *kexec_crash_image;
> > > > > > >  extern int kexec_load_disabled;
> > > > > > > +extern int kexec_reboot_disabled;
> > > > > > >
> > > > > > >  #ifndef kexec_flush_icache_page
> > > > > > >  #define kexec_flush_icache_page(page)
> > > > > > > diff --git a/kernel/kexec.c b/kernel/kexec.c
> > > > > > > index cb8e6e6f983c..43063f803d81 100644
> > > > > > > --- a/kernel/kexec.c
> > > > > > > +++ b/kernel/kexec.c
> > > > > > > @@ -196,6 +196,10 @@ static inline int kexec_load_check(unsigned long nr_segments,
> > > > > > >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> > > > > > >               return -EPERM;
> > > > > > >
> > > > > > > +     /* Check if the system admin has disabled kexec reboot. */
> > > > > > > +     if (!(flags & KEXEC_ON_CRASH) && kexec_reboot_disabled)
> > > > > > > +             return -EPERM;  
> > > > > >
> > > > > > ... Allowing to load a crashkernel doesn't make sense in my opinion. If
> > > > > > an attacker is capable of creating a malicious kernel, planting it on
> > > > > > the victims system and then find a way to boot it via kexec this
> > > > > > attacker also knows how to load the malicious kernel as crashkernel and
> > > > > > trigger a panic. So you haven't really gained anything. That's why I
> > > > > > would simply drop this hunk (and the corresponding one from
> > > > > > kexec_file_load) and let users who worry about this use a combination of
> > > > > > kexec_load_disabled and kexec_reboot_disabled.  
> > > > >
> > > > > If for whatever reason your sysadmin configured kexec_reboot_disabed
> > > > > it can be nice that when a user try to load it they get a warning.
> > > > > It is easier to debug than waiting two steps later when they run kexec -e....  
> > > >
> > > > I'm having second thoughts about this patch. My main problem is that I
> > > > don't see a real use case where kexec_reboot_disabled is advantageous
> > > > over kexec_load_disabled. The point is that disabling
> > > > LINUX_REBOOT_CMD_KEXEC is almost identical to toggling kexec_load_disabled without
> > > > a loaded kernel (when you don't have a kernel loaded you cannot reboot
> > > > into it). With this the main use case of kexec_reboot_disabled is
> > > > already covered by kexec_load_disabled.  
> > >  
> > > >
> > > > However, there are two differences
> > > >
> > > > 1) with kexec_reboot_disable you can still (re-)load a crash kernel
> > > > e.g. to update the initramfs after a config change. But as discussed in
> > > > my first mail this comes on the cost that an attacker could still load a
> > > > malicious crash kernel and then 'panic into it'.  
> > >
> > > That crash kernel must be already in the signed malicious kernel.
> > > which reduces the chances of attack.
> > > Plus an attacker must be able to panic the current kernel at will,
> > > instead of just call reset.
> > >  
> > > >
> > > > 2) kexec_load_disabled also prevents unloading of a loaded kernel. So
> > > > once loaded kexec_load_disabled cannot prevent the reboot into this
> > > > kernel.
> > > >
> > > >
> > > > For 1) I doubt that this is desired at all. My expectation is that on
> > > > systems where a sysadmin restricts a user to reboot via kexec the
> > > > sysadmin also wants to prevent the user to load an arbitrary crash
> > > > kernel. Especially as this still keeps the loophole open you are trying
> > > > to close.
> > > >
> > > > So only 2) is left as real benefit. But that is an extremely specific
> > > > scenario. How often does this scenario happen in real life? What
> > > > problem does kexec_reboot_disable solve different implementation
> > > > (also in userspace) cannot?
> > > >
> > > > Sorry about being this pedantic but you want to introduce some new uapi
> > > > which will be hard if not impossible to change once introduced. That's
> > > > why I want to be a 100% sure it is really needed.  
> > >
> > > No worries. Completely understand :). Thanks for taking this seriously..
> > >
> > >
> > > Best regards!  
> > > >
> > > > Thanks
> > > > Philipp
> > > >
> > > >  
> > > > > That is why I added it. But i am also ok removing it
> > > > >  
> > > > > >
> > > > > > Thanks
> > > > > > Philipp
> > > > > >  
> > > > > > > +
> > > > > > >       /* Permit LSMs and IMA to fail the kexec */
> > > > > > >       result = security_kernel_load_data(LOADING_KEXEC_IMAGE, false);
> > > > > > >       if (result < 0)
> > > > > > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > > > > > > index ca2743f9c634..fe82e2525705 100644
> > > > > > > --- a/kernel/kexec_core.c
> > > > > > > +++ b/kernel/kexec_core.c
> > > > > > > @@ -929,6 +929,7 @@ int kimage_load_segment(struct kimage *image,
> > > > > > >  struct kimage *kexec_image;
> > > > > > >  struct kimage *kexec_crash_image;
> > > > > > >  int kexec_load_disabled;
> > > > > > > +int kexec_reboot_disabled;
> > > > > > >  #ifdef CONFIG_SYSCTL
> > > > > > >  static struct ctl_table kexec_core_sysctls[] = {
> > > > > > >       {
> > > > > > > @@ -941,6 +942,16 @@ static struct ctl_table kexec_core_sysctls[] = {
> > > > > > >               .extra1         = SYSCTL_ONE,
> > > > > > >               .extra2         = SYSCTL_ONE,
> > > > > > >       },
> > > > > > > +     {
> > > > > > > +             .procname       = "kexec_reboot_disabled",
> > > > > > > +             .data           = &kexec_reboot_disabled,
> > > > > > > +             .maxlen         = sizeof(int),
> > > > > > > +             .mode           = 0644,
> > > > > > > +             /* only handle a transition from default "0" to "1" */
> > > > > > > +             .proc_handler   = proc_dointvec_minmax,
> > > > > > > +             .extra1         = SYSCTL_ONE,
> > > > > > > +             .extra2         = SYSCTL_ONE,
> > > > > > > +     },
> > > > > > >       { }
> > > > > > >  };
> > > > > > >
> > > > > > > @@ -1138,7 +1149,7 @@ int kernel_kexec(void)
> > > > > > >
> > > > > > >       if (!kexec_trylock())
> > > > > > >               return -EBUSY;
> > > > > > > -     if (!kexec_image) {
> > > > > > > +     if (!kexec_image || kexec_reboot_disabled) {
> > > > > > >               error = -EINVAL;
> > > > > > >               goto Unlock;
> > > > > > >       }
> > > > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > > > > index 45637511e0de..583fba6de5cb 100644
> > > > > > > --- a/kernel/kexec_file.c
> > > > > > > +++ b/kernel/kexec_file.c
> > > > > > > @@ -333,6 +333,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> > > > > > >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> > > > > > >               return -EPERM;
> > > > > > >
> > > > > > > +     /* Check if the system admin has disabled kexec reboot. */
> > > > > > > +     if (!(flags & (KEXEC_FILE_ON_CRASH | KEXEC_FILE_UNLOAD))
> > > > > > > +         && kexec_reboot_disabled)
> > > > > > > +             return -EPERM;
> > > > > > > +
> > > > > > >       /* Make sure we have a legal set of flags */
> > > > > > >       if (flags != (flags & KEXEC_FILE_FLAGS))
> > > > > > >               return -EINVAL;
> > > > > > >  
> > > > > >  
> > > > >
> > > > >  
> > > >  
> > >
> > >  
> >  
> 
>
  

Hi Philipp


On Thu, 24 Nov 2022 at 16:01, Philipp Rudo <prudo@redhat.com> wrote:
>
> On Thu, 24 Nov 2022 13:52:58 +0100
> Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> > On Thu, 24 Nov 2022 at 12:40, Philipp Rudo <prudo@redhat.com> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > On Wed, 23 Nov 2022 09:58:08 +0100
> > > Ricardo Ribalda <ribalda@chromium.org> wrote:
> > >
> > > > Hi Philipp
> > > >
> > > > Thanks for your review.
> > > >
> > > > My scenario is a trusted system, where even if you are root, your
> > > > access to the system is very limited.
> > > >
> > > > Let's assume LOADPIN and verity are enabled.
> > >
> > > My point is that on such systems I expect that a sysadmin also wants to
> > > control the crash kernel including its initramfs (which also has to be part
> > > of the signed kernel?). But if that's the case a sysadmin can simply arm
> > > kdump early during boot and then toggle kexec_load_disabled. With that
> > > LINUX_REBOOT_CMD_KEXEC also gets disabled as no kexec kernel can be loaded
> > > while kdump works. Thus there is no need to add the new interface. Or am
> > > I missing anything?
> >
> > Let's say that you have a script that does something like this
> >
> >
> > kexec -p dump_kernel
> > echo 1 > /proc/sys/kernel/kexec_load_disabled
> >
> > If an attacker can DDos the system and make that script crash... then
> > kexec is still accessible
> >
> > On the other hand, if you load the kernel with the commandline
> >
> > sysctl.kernel.kexec_load_disabled=1
>                       ^^^^
>                       reboot?

yes :)  thanks!

> Otherwise you shouldn't be able to load the crash kernel at all.
>
> > Then even if the script crashes, the only way to abuse kexec is by
> > panicing the running kernel....
>
> True. But  when an attacker can DDos the system the final workload is
> already running. So wouldn't it be enough to make sure that the script
> above has finished before starting you workload. E.g. by setting an
> appropriate Before=/After= in the systemd.unit?

What if the kexec binary crashes and the unit will never succeed?

Or worse, your distro does not use systemd !!!

>
> Furthermore, I don't think that restricting kexec reboot alone is
> sufficient when the attacker can still control the crash kernel. At
> least my assumption is that triggering a panic instead of just
> rebooting is just a mild inconvenience for somebody who is able to pull
> off an attack like that.

The attacker does not control the crash kernel completely. loadpin is
still in place.
Yes, they can downgrade the whole system to a vulnerable kernel image.
But the choices are limited :)

With physical access to the device panicing a kernel is easily doable
(but not trivial). But remotely, it is more challenging.

>
> > Would it make you more comfortable if I model this as a kernel config
> > instead of a runtime option?
>
> No, I think the implementation is fine. I'm currently only struggling
> to understand what problem kexec_reboot_disabled solves that cannot be
> solved by kexec_load_disabled.
>
> > Thanks!
>
> Happy to help.
>
> Thanks
> Philipp
>
> >
> >
> > >
> > > Thanks
> > > Philipp
> > >
> > > >
> > > > On Mon, 21 Nov 2022 at 15:10, Philipp Rudo <prudo@redhat.com> wrote:
> > > > >
> > > > > Hi Ricardo,
> > > > >
> > > > > On Thu, 17 Nov 2022 16:15:07 +0100
> > > > > Ricardo Ribalda <ribalda@chromium.org> wrote:
> > > > >
> > > > > > Hi Philipp
> > > > > >
> > > > > > Thanks for your review!
> > > > >
> > > > > happy to help.
> > > > >
> > > > > >
> > > > > > On Thu, 17 Nov 2022 at 16:07, Philipp Rudo <prudo@redhat.com> wrote:
> > > > > > >
> > > > > > > Hi Ricardo,
> > > > > > >
> > > > > > > all in all I think this patch makes sense. However, there is one point
> > > > > > > I don't like...
> > > > > > >
> > > > > > > On Mon, 14 Nov 2022 14:18:39 +0100
> > > > > > > Ricardo Ribalda <ribalda@chromium.org> wrote:
> > > > > > >
> > > > > > > > Create a new toogle that disables LINUX_REBOOT_CMD_KEXEC, reducing the
> > > > > > > > attack surface to a system.
> > > > > > > >
> > > > > > > > Without this toogle, an attacker can only reboot into a different kernel
> > > > > > > > if they can create a panic().
> > > > > > > >
> > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > > >
> > > > > > > > diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> > > > > > > > index 97394bd9d065..25d019682d33 100644
> > > > > > > > --- a/Documentation/admin-guide/sysctl/kernel.rst
> > > > > > > > +++ b/Documentation/admin-guide/sysctl/kernel.rst
> > > > > > > > @@ -462,6 +462,17 @@ altered.
> > > > > > > >  Generally used together with the `modules_disabled`_ sysctl.
> > > > > > > >
> > > > > > > >
> > > > > > > > +kexec_reboot_disabled
> > > > > > > > +=====================
> > > > > > > > +
> > > > > > > > +A toggle indicating if ``LINUX_REBOOT_CMD_KEXEC`` has been disabled.
> > > > > > > > +This value defaults to 0 (false: ``LINUX_REBOOT_CMD_KEXEC`` enabled),
> > > > > > > > +but can be set to 1 (true: ``LINUX_REBOOT_CMD_KEXEC`` disabled).
> > > > > > > > +Once true, kexec can no longer be used for reboot and the toggle
> > > > > > > > +cannot be set back to false.
> > > > > > > > +This toggle does not affect the use of kexec during a crash.
> > > > > > > > +
> > > > > > > > +
> > > > > > > >  kptr_restrict
> > > > > > > >  =============
> > > > > > > >
> > > > > > > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > > > > > > > index 41a686996aaa..15c3fad8918b 100644
> > > > > > > > --- a/include/linux/kexec.h
> > > > > > > > +++ b/include/linux/kexec.h
> > > > > > > > @@ -407,6 +407,7 @@ extern int kimage_crash_copy_vmcoreinfo(struct kimage *image);
> > > > > > > >  extern struct kimage *kexec_image;
> > > > > > > >  extern struct kimage *kexec_crash_image;
> > > > > > > >  extern int kexec_load_disabled;
> > > > > > > > +extern int kexec_reboot_disabled;
> > > > > > > >
> > > > > > > >  #ifndef kexec_flush_icache_page
> > > > > > > >  #define kexec_flush_icache_page(page)
> > > > > > > > diff --git a/kernel/kexec.c b/kernel/kexec.c
> > > > > > > > index cb8e6e6f983c..43063f803d81 100644
> > > > > > > > --- a/kernel/kexec.c
> > > > > > > > +++ b/kernel/kexec.c
> > > > > > > > @@ -196,6 +196,10 @@ static inline int kexec_load_check(unsigned long nr_segments,
> > > > > > > >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> > > > > > > >               return -EPERM;
> > > > > > > >
> > > > > > > > +     /* Check if the system admin has disabled kexec reboot. */
> > > > > > > > +     if (!(flags & KEXEC_ON_CRASH) && kexec_reboot_disabled)
> > > > > > > > +             return -EPERM;
> > > > > > >
> > > > > > > ... Allowing to load a crashkernel doesn't make sense in my opinion. If
> > > > > > > an attacker is capable of creating a malicious kernel, planting it on
> > > > > > > the victims system and then find a way to boot it via kexec this
> > > > > > > attacker also knows how to load the malicious kernel as crashkernel and
> > > > > > > trigger a panic. So you haven't really gained anything. That's why I
> > > > > > > would simply drop this hunk (and the corresponding one from
> > > > > > > kexec_file_load) and let users who worry about this use a combination of
> > > > > > > kexec_load_disabled and kexec_reboot_disabled.
> > > > > >
> > > > > > If for whatever reason your sysadmin configured kexec_reboot_disabed
> > > > > > it can be nice that when a user try to load it they get a warning.
> > > > > > It is easier to debug than waiting two steps later when they run kexec -e....
> > > > >
> > > > > I'm having second thoughts about this patch. My main problem is that I
> > > > > don't see a real use case where kexec_reboot_disabled is advantageous
> > > > > over kexec_load_disabled. The point is that disabling
> > > > > LINUX_REBOOT_CMD_KEXEC is almost identical to toggling kexec_load_disabled without
> > > > > a loaded kernel (when you don't have a kernel loaded you cannot reboot
> > > > > into it). With this the main use case of kexec_reboot_disabled is
> > > > > already covered by kexec_load_disabled.
> > > >
> > > > >
> > > > > However, there are two differences
> > > > >
> > > > > 1) with kexec_reboot_disable you can still (re-)load a crash kernel
> > > > > e.g. to update the initramfs after a config change. But as discussed in
> > > > > my first mail this comes on the cost that an attacker could still load a
> > > > > malicious crash kernel and then 'panic into it'.
> > > >
> > > > That crash kernel must be already in the signed malicious kernel.
> > > > which reduces the chances of attack.
> > > > Plus an attacker must be able to panic the current kernel at will,
> > > > instead of just call reset.
> > > >
> > > > >
> > > > > 2) kexec_load_disabled also prevents unloading of a loaded kernel. So
> > > > > once loaded kexec_load_disabled cannot prevent the reboot into this
> > > > > kernel.
> > > > >
> > > > >
> > > > > For 1) I doubt that this is desired at all. My expectation is that on
> > > > > systems where a sysadmin restricts a user to reboot via kexec the
> > > > > sysadmin also wants to prevent the user to load an arbitrary crash
> > > > > kernel. Especially as this still keeps the loophole open you are trying
> > > > > to close.
> > > > >
> > > > > So only 2) is left as real benefit. But that is an extremely specific
> > > > > scenario. How often does this scenario happen in real life? What
> > > > > problem does kexec_reboot_disable solve different implementation
> > > > > (also in userspace) cannot?
> > > > >
> > > > > Sorry about being this pedantic but you want to introduce some new uapi
> > > > > which will be hard if not impossible to change once introduced. That's
> > > > > why I want to be a 100% sure it is really needed.
> > > >
> > > > No worries. Completely understand :). Thanks for taking this seriously..
> > > >
> > > >
> > > > Best regards!
> > > > >
> > > > > Thanks
> > > > > Philipp
> > > > >
> > > > >
> > > > > > That is why I added it. But i am also ok removing it
> > > > > >
> > > > > > >
> > > > > > > Thanks
> > > > > > > Philipp
> > > > > > >
> > > > > > > > +
> > > > > > > >       /* Permit LSMs and IMA to fail the kexec */
> > > > > > > >       result = security_kernel_load_data(LOADING_KEXEC_IMAGE, false);
> > > > > > > >       if (result < 0)
> > > > > > > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > > > > > > > index ca2743f9c634..fe82e2525705 100644
> > > > > > > > --- a/kernel/kexec_core.c
> > > > > > > > +++ b/kernel/kexec_core.c
> > > > > > > > @@ -929,6 +929,7 @@ int kimage_load_segment(struct kimage *image,
> > > > > > > >  struct kimage *kexec_image;
> > > > > > > >  struct kimage *kexec_crash_image;
> > > > > > > >  int kexec_load_disabled;
> > > > > > > > +int kexec_reboot_disabled;
> > > > > > > >  #ifdef CONFIG_SYSCTL
> > > > > > > >  static struct ctl_table kexec_core_sysctls[] = {
> > > > > > > >       {
> > > > > > > > @@ -941,6 +942,16 @@ static struct ctl_table kexec_core_sysctls[] = {
> > > > > > > >               .extra1         = SYSCTL_ONE,
> > > > > > > >               .extra2         = SYSCTL_ONE,
> > > > > > > >       },
> > > > > > > > +     {
> > > > > > > > +             .procname       = "kexec_reboot_disabled",
> > > > > > > > +             .data           = &kexec_reboot_disabled,
> > > > > > > > +             .maxlen         = sizeof(int),
> > > > > > > > +             .mode           = 0644,
> > > > > > > > +             /* only handle a transition from default "0" to "1" */
> > > > > > > > +             .proc_handler   = proc_dointvec_minmax,
> > > > > > > > +             .extra1         = SYSCTL_ONE,
> > > > > > > > +             .extra2         = SYSCTL_ONE,
> > > > > > > > +     },
> > > > > > > >       { }
> > > > > > > >  };
> > > > > > > >
> > > > > > > > @@ -1138,7 +1149,7 @@ int kernel_kexec(void)
> > > > > > > >
> > > > > > > >       if (!kexec_trylock())
> > > > > > > >               return -EBUSY;
> > > > > > > > -     if (!kexec_image) {
> > > > > > > > +     if (!kexec_image || kexec_reboot_disabled) {
> > > > > > > >               error = -EINVAL;
> > > > > > > >               goto Unlock;
> > > > > > > >       }
> > > > > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > > > > > index 45637511e0de..583fba6de5cb 100644
> > > > > > > > --- a/kernel/kexec_file.c
> > > > > > > > +++ b/kernel/kexec_file.c
> > > > > > > > @@ -333,6 +333,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> > > > > > > >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> > > > > > > >               return -EPERM;
> > > > > > > >
> > > > > > > > +     /* Check if the system admin has disabled kexec reboot. */
> > > > > > > > +     if (!(flags & (KEXEC_FILE_ON_CRASH | KEXEC_FILE_UNLOAD))
> > > > > > > > +         && kexec_reboot_disabled)
> > > > > > > > +             return -EPERM;
> > > > > > > > +
> > > > > > > >       /* Make sure we have a legal set of flags */
> > > > > > > >       if (flags != (flags & KEXEC_FILE_FLAGS))
> > > > > > > >               return -EINVAL;
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > >
> >
> >
>
  

Hi Ricardo,

On Thu, 24 Nov 2022 23:32:34 +0100
Ricardo Ribalda <ribalda@chromium.org> wrote:

> Hi Philipp
> 
> 
> On Thu, 24 Nov 2022 at 16:01, Philipp Rudo <prudo@redhat.com> wrote:
> >
> > On Thu, 24 Nov 2022 13:52:58 +0100
> > Ricardo Ribalda <ribalda@chromium.org> wrote:
> >  
> > > On Thu, 24 Nov 2022 at 12:40, Philipp Rudo <prudo@redhat.com> wrote:  
> > > >
> > > > Hi Ricardo,
> > > >
> > > > On Wed, 23 Nov 2022 09:58:08 +0100
> > > > Ricardo Ribalda <ribalda@chromium.org> wrote:
> > > >  
> > > > > Hi Philipp
> > > > >
> > > > > Thanks for your review.
> > > > >
> > > > > My scenario is a trusted system, where even if you are root, your
> > > > > access to the system is very limited.
> > > > >
> > > > > Let's assume LOADPIN and verity are enabled.  
> > > >
> > > > My point is that on such systems I expect that a sysadmin also wants to
> > > > control the crash kernel including its initramfs (which also has to be part
> > > > of the signed kernel?). But if that's the case a sysadmin can simply arm
> > > > kdump early during boot and then toggle kexec_load_disabled. With that
> > > > LINUX_REBOOT_CMD_KEXEC also gets disabled as no kexec kernel can be loaded
> > > > while kdump works. Thus there is no need to add the new interface. Or am
> > > > I missing anything?  
> > >
> > > Let's say that you have a script that does something like this
> > >
> > >
> > > kexec -p dump_kernel
> > > echo 1 > /proc/sys/kernel/kexec_load_disabled
> > >
> > > If an attacker can DDos the system and make that script crash... then
> > > kexec is still accessible
> > >
> > > On the other hand, if you load the kernel with the commandline
> > >
> > > sysctl.kernel.kexec_load_disabled=1  
> >                       ^^^^
> >                       reboot?  
> 
> yes :)  thanks!
> 
> > Otherwise you shouldn't be able to load the crash kernel at all.
> >  
> > > Then even if the script crashes, the only way to abuse kexec is by
> > > panicing the running kernel....  
> >
> > True. But  when an attacker can DDos the system the final workload is
> > already running. So wouldn't it be enough to make sure that the script
> > above has finished before starting you workload. E.g. by setting an
> > appropriate Before=/After= in the systemd.unit?  
> 
> What if the kexec binary crashes and the unit will never succeed?

Then there are options like OnFailure= or FailureAction= the sysadmin
can use do what ever he seems appropriate.

> Or worse, your distro does not use systemd !!!

In that case there are other ways to achieve the same. The two options
are only examples. Why can't this be used as a replacement?

> > Furthermore, I don't think that restricting kexec reboot alone is
> > sufficient when the attacker can still control the crash kernel. At
> > least my assumption is that triggering a panic instead of just
> > rebooting is just a mild inconvenience for somebody who is able to pull
> > off an attack like that.  
> 
> The attacker does not control the crash kernel completely. loadpin is
> still in place.
> Yes, they can downgrade the whole system to a vulnerable kernel image.
> But the choices are limited :)
> 
> With physical access to the device panicing a kernel is easily doable
> (but not trivial). But remotely, it is more challenging.


Well the same holds for kexec. So the only difference is triggering the
panic where I'm still not convinced it's a huge obstacle for someone
who is able to pull off all the steps before for such an attack.

To be honest I don't think we make a progress here at the moment. I
would like to hear from others what they think about this.

Thanks
Philipp

> 
> >  
> > > Would it make you more comfortable if I model this as a kernel config
> > > instead of a runtime option?  
> >
> > No, I think the implementation is fine. I'm currently only struggling
> > to understand what problem kexec_reboot_disabled solves that cannot be
> > solved by kexec_load_disabled.
> >  
> > > Thanks!  
> >
> > Happy to help.
> >
> > Thanks
> > Philipp
> >  
> > >
> > >  
> > > >
> > > > Thanks
> > > > Philipp
> > > >  
> > > > >
> > > > > On Mon, 21 Nov 2022 at 15:10, Philipp Rudo <prudo@redhat.com> wrote:  
> > > > > >
> > > > > > Hi Ricardo,
> > > > > >
> > > > > > On Thu, 17 Nov 2022 16:15:07 +0100
> > > > > > Ricardo Ribalda <ribalda@chromium.org> wrote:
> > > > > >  
> > > > > > > Hi Philipp
> > > > > > >
> > > > > > > Thanks for your review!  
> > > > > >
> > > > > > happy to help.
> > > > > >  
> > > > > > >
> > > > > > > On Thu, 17 Nov 2022 at 16:07, Philipp Rudo <prudo@redhat.com> wrote:  
> > > > > > > >
> > > > > > > > Hi Ricardo,
> > > > > > > >
> > > > > > > > all in all I think this patch makes sense. However, there is one point
> > > > > > > > I don't like...
> > > > > > > >
> > > > > > > > On Mon, 14 Nov 2022 14:18:39 +0100
> > > > > > > > Ricardo Ribalda <ribalda@chromium.org> wrote:
> > > > > > > >  
> > > > > > > > > Create a new toogle that disables LINUX_REBOOT_CMD_KEXEC, reducing the
> > > > > > > > > attack surface to a system.
> > > > > > > > >
> > > > > > > > > Without this toogle, an attacker can only reboot into a different kernel
> > > > > > > > > if they can create a panic().
> > > > > > > > >
> > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > > > >
> > > > > > > > > diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> > > > > > > > > index 97394bd9d065..25d019682d33 100644
> > > > > > > > > --- a/Documentation/admin-guide/sysctl/kernel.rst
> > > > > > > > > +++ b/Documentation/admin-guide/sysctl/kernel.rst
> > > > > > > > > @@ -462,6 +462,17 @@ altered.
> > > > > > > > >  Generally used together with the `modules_disabled`_ sysctl.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > +kexec_reboot_disabled
> > > > > > > > > +=====================
> > > > > > > > > +
> > > > > > > > > +A toggle indicating if ``LINUX_REBOOT_CMD_KEXEC`` has been disabled.
> > > > > > > > > +This value defaults to 0 (false: ``LINUX_REBOOT_CMD_KEXEC`` enabled),
> > > > > > > > > +but can be set to 1 (true: ``LINUX_REBOOT_CMD_KEXEC`` disabled).
> > > > > > > > > +Once true, kexec can no longer be used for reboot and the toggle
> > > > > > > > > +cannot be set back to false.
> > > > > > > > > +This toggle does not affect the use of kexec during a crash.
> > > > > > > > > +
> > > > > > > > > +
> > > > > > > > >  kptr_restrict
> > > > > > > > >  =============
> > > > > > > > >
> > > > > > > > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > > > > > > > > index 41a686996aaa..15c3fad8918b 100644
> > > > > > > > > --- a/include/linux/kexec.h
> > > > > > > > > +++ b/include/linux/kexec.h
> > > > > > > > > @@ -407,6 +407,7 @@ extern int kimage_crash_copy_vmcoreinfo(struct kimage *image);
> > > > > > > > >  extern struct kimage *kexec_image;
> > > > > > > > >  extern struct kimage *kexec_crash_image;
> > > > > > > > >  extern int kexec_load_disabled;
> > > > > > > > > +extern int kexec_reboot_disabled;
> > > > > > > > >
> > > > > > > > >  #ifndef kexec_flush_icache_page
> > > > > > > > >  #define kexec_flush_icache_page(page)
> > > > > > > > > diff --git a/kernel/kexec.c b/kernel/kexec.c
> > > > > > > > > index cb8e6e6f983c..43063f803d81 100644
> > > > > > > > > --- a/kernel/kexec.c
> > > > > > > > > +++ b/kernel/kexec.c
> > > > > > > > > @@ -196,6 +196,10 @@ static inline int kexec_load_check(unsigned long nr_segments,
> > > > > > > > >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> > > > > > > > >               return -EPERM;
> > > > > > > > >
> > > > > > > > > +     /* Check if the system admin has disabled kexec reboot. */
> > > > > > > > > +     if (!(flags & KEXEC_ON_CRASH) && kexec_reboot_disabled)
> > > > > > > > > +             return -EPERM;  
> > > > > > > >
> > > > > > > > ... Allowing to load a crashkernel doesn't make sense in my opinion. If
> > > > > > > > an attacker is capable of creating a malicious kernel, planting it on
> > > > > > > > the victims system and then find a way to boot it via kexec this
> > > > > > > > attacker also knows how to load the malicious kernel as crashkernel and
> > > > > > > > trigger a panic. So you haven't really gained anything. That's why I
> > > > > > > > would simply drop this hunk (and the corresponding one from
> > > > > > > > kexec_file_load) and let users who worry about this use a combination of
> > > > > > > > kexec_load_disabled and kexec_reboot_disabled.  
> > > > > > >
> > > > > > > If for whatever reason your sysadmin configured kexec_reboot_disabed
> > > > > > > it can be nice that when a user try to load it they get a warning.
> > > > > > > It is easier to debug than waiting two steps later when they run kexec -e....  
> > > > > >
> > > > > > I'm having second thoughts about this patch. My main problem is that I
> > > > > > don't see a real use case where kexec_reboot_disabled is advantageous
> > > > > > over kexec_load_disabled. The point is that disabling
> > > > > > LINUX_REBOOT_CMD_KEXEC is almost identical to toggling kexec_load_disabled without
> > > > > > a loaded kernel (when you don't have a kernel loaded you cannot reboot
> > > > > > into it). With this the main use case of kexec_reboot_disabled is
> > > > > > already covered by kexec_load_disabled.  
> > > > >  
> > > > > >
> > > > > > However, there are two differences
> > > > > >
> > > > > > 1) with kexec_reboot_disable you can still (re-)load a crash kernel
> > > > > > e.g. to update the initramfs after a config change. But as discussed in
> > > > > > my first mail this comes on the cost that an attacker could still load a
> > > > > > malicious crash kernel and then 'panic into it'.  
> > > > >
> > > > > That crash kernel must be already in the signed malicious kernel.
> > > > > which reduces the chances of attack.
> > > > > Plus an attacker must be able to panic the current kernel at will,
> > > > > instead of just call reset.
> > > > >  
> > > > > >
> > > > > > 2) kexec_load_disabled also prevents unloading of a loaded kernel. So
> > > > > > once loaded kexec_load_disabled cannot prevent the reboot into this
> > > > > > kernel.
> > > > > >
> > > > > >
> > > > > > For 1) I doubt that this is desired at all. My expectation is that on
> > > > > > systems where a sysadmin restricts a user to reboot via kexec the
> > > > > > sysadmin also wants to prevent the user to load an arbitrary crash
> > > > > > kernel. Especially as this still keeps the loophole open you are trying
> > > > > > to close.
> > > > > >
> > > > > > So only 2) is left as real benefit. But that is an extremely specific
> > > > > > scenario. How often does this scenario happen in real life? What
> > > > > > problem does kexec_reboot_disable solve different implementation
> > > > > > (also in userspace) cannot?
> > > > > >
> > > > > > Sorry about being this pedantic but you want to introduce some new uapi
> > > > > > which will be hard if not impossible to change once introduced. That's
> > > > > > why I want to be a 100% sure it is really needed.  
> > > > >
> > > > > No worries. Completely understand :). Thanks for taking this seriously..
> > > > >
> > > > >
> > > > > Best regards!  
> > > > > >
> > > > > > Thanks
> > > > > > Philipp
> > > > > >
> > > > > >  
> > > > > > > That is why I added it. But i am also ok removing it
> > > > > > >  
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > > Philipp
> > > > > > > >  
> > > > > > > > > +
> > > > > > > > >       /* Permit LSMs and IMA to fail the kexec */
> > > > > > > > >       result = security_kernel_load_data(LOADING_KEXEC_IMAGE, false);
> > > > > > > > >       if (result < 0)
> > > > > > > > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > > > > > > > > index ca2743f9c634..fe82e2525705 100644
> > > > > > > > > --- a/kernel/kexec_core.c
> > > > > > > > > +++ b/kernel/kexec_core.c
> > > > > > > > > @@ -929,6 +929,7 @@ int kimage_load_segment(struct kimage *image,
> > > > > > > > >  struct kimage *kexec_image;
> > > > > > > > >  struct kimage *kexec_crash_image;
> > > > > > > > >  int kexec_load_disabled;
> > > > > > > > > +int kexec_reboot_disabled;
> > > > > > > > >  #ifdef CONFIG_SYSCTL
> > > > > > > > >  static struct ctl_table kexec_core_sysctls[] = {
> > > > > > > > >       {
> > > > > > > > > @@ -941,6 +942,16 @@ static struct ctl_table kexec_core_sysctls[] = {
> > > > > > > > >               .extra1         = SYSCTL_ONE,
> > > > > > > > >               .extra2         = SYSCTL_ONE,
> > > > > > > > >       },
> > > > > > > > > +     {
> > > > > > > > > +             .procname       = "kexec_reboot_disabled",
> > > > > > > > > +             .data           = &kexec_reboot_disabled,
> > > > > > > > > +             .maxlen         = sizeof(int),
> > > > > > > > > +             .mode           = 0644,
> > > > > > > > > +             /* only handle a transition from default "0" to "1" */
> > > > > > > > > +             .proc_handler   = proc_dointvec_minmax,
> > > > > > > > > +             .extra1         = SYSCTL_ONE,
> > > > > > > > > +             .extra2         = SYSCTL_ONE,
> > > > > > > > > +     },
> > > > > > > > >       { }
> > > > > > > > >  };
> > > > > > > > >
> > > > > > > > > @@ -1138,7 +1149,7 @@ int kernel_kexec(void)
> > > > > > > > >
> > > > > > > > >       if (!kexec_trylock())
> > > > > > > > >               return -EBUSY;
> > > > > > > > > -     if (!kexec_image) {
> > > > > > > > > +     if (!kexec_image || kexec_reboot_disabled) {
> > > > > > > > >               error = -EINVAL;
> > > > > > > > >               goto Unlock;
> > > > > > > > >       }
> > > > > > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > > > > > > index 45637511e0de..583fba6de5cb 100644
> > > > > > > > > --- a/kernel/kexec_file.c
> > > > > > > > > +++ b/kernel/kexec_file.c
> > > > > > > > > @@ -333,6 +333,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> > > > > > > > >       if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> > > > > > > > >               return -EPERM;
> > > > > > > > >
> > > > > > > > > +     /* Check if the system admin has disabled kexec reboot. */
> > > > > > > > > +     if (!(flags & (KEXEC_FILE_ON_CRASH | KEXEC_FILE_UNLOAD))
> > > > > > > > > +         && kexec_reboot_disabled)
> > > > > > > > > +             return -EPERM;
> > > > > > > > > +
> > > > > > > > >       /* Make sure we have a legal set of flags */
> > > > > > > > >       if (flags != (flags & KEXEC_FILE_FLAGS))
> > > > > > > > >               return -EINVAL;
> > > > > > > > >  
> > > > > > > >  
> > > > > > >
> > > > > > >  
> > > > > >  
> > > > >
> > > > >  
> > > >  
> > >
> > >  
> >  
> 
>
  

On Thu, 24 Nov 2022 16:01:15 +0100
Philipp Rudo <prudo@redhat.com> wrote:

> No, I think the implementation is fine. I'm currently only struggling
> to understand what problem kexec_reboot_disabled solves that cannot be
> solved by kexec_load_disabled.

Hi Philipp,

Thanks for working with us on this.

Let me try to explain our use case. We want kexec/kdump enabled, but we
really do not want kexec used for any other purpose. We must have the kexec
kernel loaded at boot up and not afterward.

Your recommendation of:

  kexec -p dump_kernel
  echo 1 > /proc/sys/kernel/kexec_load_disabled

can work, and we will probably add it. But we are taking the paranoid
approach, and what I learned in security 101 ;-) and that is, only open up
the minimal attack surface as possible.

Yes, it's highly unlikely that the above would crash. But as with most
security vulnerabilities, it's not going to be an attacker that creates a
new gadget here, but probably another script in the future that causes this
to be delayed or something, and a new window of opportunity will arise for
an attacker. Maybe, that new window only works for non panic kernels. Yes,
this is a contrived scenario, but the work vs risk is very low in adding
this feature.

Perhaps the attack surface that a reboot kexec could be, is that the
attacker gets the ability at boot up to load the kexec for reboot and not panic.
Then the attack must wait for the victim to reboot their machine before
they have access to the new kernel. Again, I admit this is contrived, but
just because I can't think of a real situation that this could be a problem
doesn't mean that one doesn't exist.

In other words, if we never want to allow a kexec reboot, why allow it at
all from the beginning? The above allows it, until we don't. That alone
makes us nervous. Whereas this patch is rather trivial and doesn't add
complexity.

Thanks for your time, we appreciate it.

-- Steve
  

On Mon, 28 Nov 2022 17:28:55 +0100
Philipp Rudo <prudo@redhat.com> wrote:

> To be honest I don't think we make a progress here at the moment. I
> would like to hear from others what they think about this.

Not sure if you missed my reply.

  https://lore.kernel.org/all/20221128114200.72b3e2fe@gandalf.local.home/

-- Steve
  

Hi Steven,

On Mon, 28 Nov 2022 11:42:00 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 24 Nov 2022 16:01:15 +0100
> Philipp Rudo <prudo@redhat.com> wrote:
> 
> > No, I think the implementation is fine. I'm currently only struggling
> > to understand what problem kexec_reboot_disabled solves that cannot be
> > solved by kexec_load_disabled.  
> 
> Hi Philipp,
> 
> Thanks for working with us on this.
> 
> Let me try to explain our use case. We want kexec/kdump enabled, but we
> really do not want kexec used for any other purpose. We must have the kexec
> kernel loaded at boot up and not afterward.
> 
> Your recommendation of:
> 
>   kexec -p dump_kernel
>   echo 1 > /proc/sys/kernel/kexec_load_disabled
> 
> can work, and we will probably add it. But we are taking the paranoid
> approach, and what I learned in security 101 ;-) and that is, only open up
> the minimal attack surface as possible.

Well that's sort of my problem. When you go all in on paranoia I would
expect that you also restrict the crash kernel. Otherwise you keep most
of the attack surface. But disabling 'reboot' of the crash kernel is
quite intrusive and probably not what you want. That's why I think it
is better do the restriction on the 'load' rather than the 'reboot'
path.

One solution for that is the script above. But that pushes all the
responsibilities concerning syncing and error handling to the sysadmin.
Depending on your level of paranoia that might be too risky. Personally
I think it's fine as the kernel alone cannot fix all potential security
problems. In my opinion this has to be done in the layer that is
responsible for the task done. So when a security problem arises due to
ill syncing when starting different services it's the job of the init
system to fix the issue.

An alternative approach and sort of compromise I see is to convert
kexec_load_disabled from a simple on/off switch to a counter on how
often a kexec load can be made (in practice a tristate on/off/one-shot
should be sufficient). Ideally the reboot and panic path will
have separate counters. With that you could for example use
kexec_load_limit.reboot=0 and kexec_load_limit.panic=1 to disable the
load of images for reboot while still allow to load a crash kernel
once. With this you have the flexibility you need while also preventing
a race where an attacker overwrites your crash kernel before you can
toggle the switch. What do you think?

> Yes, it's highly unlikely that the above would crash. But as with most
> security vulnerabilities, it's not going to be an attacker that creates a
> new gadget here, but probably another script in the future that causes this
> to be delayed or something, and a new window of opportunity will arise for
> an attacker. Maybe, that new window only works for non panic kernels. Yes,
> this is a contrived scenario, but the work vs risk is very low in adding
> this feature.

True, but that problem is not limited to userspace. For example see
Ricardos other patch [1] where he treats the load of a crash kernel
just like a standard load. In my opinion he creates such a gadget in
that patch.

[1] https://lore.kernel.org/all/20221124-kexec-noalloc-v1-0-d78361e99aec@chromium.org/

Thanks
Philipp

> Perhaps the attack surface that a reboot kexec could be, is that the
> attacker gets the ability at boot up to load the kexec for reboot and not panic.
> Then the attack must wait for the victim to reboot their machine before
> they have access to the new kernel. Again, I admit this is contrived, but
> just because I can't think of a real situation that this could be a problem
> doesn't mean that one doesn't exist.
> 
> In other words, if we never want to allow a kexec reboot, why allow it at
> all from the beginning? The above allows it, until we don't. That alone
> makes us nervous. Whereas this patch is rather trivial and doesn't add
> complexity.
> 
> Thanks for your time, we appreciate it.
> 
> -- Steve
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>
  

On Tue, 29 Nov 2022 14:44:50 +0100
Philipp Rudo <prudo@redhat.com> wrote:

> An alternative approach and sort of compromise I see is to convert
> kexec_load_disabled from a simple on/off switch to a counter on how
> often a kexec load can be made (in practice a tristate on/off/one-shot
> should be sufficient). Ideally the reboot and panic path will
> have separate counters. With that you could for example use
> kexec_load_limit.reboot=0 and kexec_load_limit.panic=1 to disable the
> load of images for reboot while still allow to load a crash kernel
> once. With this you have the flexibility you need while also preventing
> a race where an attacker overwrites your crash kernel before you can
> toggle the switch. What do you think?

I actually like this idea :-)

-- Steve
  

Hi Philipp

On Tue, 29 Nov 2022 at 15:32, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 29 Nov 2022 14:44:50 +0100
> Philipp Rudo <prudo@redhat.com> wrote:
>
> > An alternative approach and sort of compromise I see is to convert
> > kexec_load_disabled from a simple on/off switch to a counter on how
> > often a kexec load can be made (in practice a tristate on/off/one-shot
> > should be sufficient). Ideally the reboot and panic path will
> > have separate counters. With that you could for example use
> > kexec_load_limit.reboot=0 and kexec_load_limit.panic=1 to disable the
> > load of images for reboot while still allow to load a crash kernel
> > once. With this you have the flexibility you need while also preventing
> > a race where an attacker overwrites your crash kernel before you can
> > toggle the switch. What do you think?
>
> I actually like this idea :-)

In case you missed it.  I sent an initial implementation of this at
https://lore.kernel.org/lkml/20221114-disable-kexec-reset-v2-0-c498313c1bb5@chromium.org/

Regards!

>
> -- Steve