Re: [PATCH] kexec: Enable runtime allocation of crash_image
Ricardo,
On Mon, 28 Nov 2022 18:07:06 +0100
Ricardo Ribalda wrote:
> Hi Philipp
>
>
> Thanks for your review.
>
>
> On Mon, 28 Nov 2022 at 18:00, Philipp Rudo wrote:
> >
> > Hi Ricardo,
> >
> > On Thu, 24 Nov 2022 23:23:36 +0100
> > Ricardo Ribalda wrote:
> >
> > > Usually crash_image is defined statically via the crashkernel parameter
> > > or DT.
> > >
> > > But if the crash kernel is not used, or is smaller than then
> > > area pre-allocated that memory is wasted.
> > >
> > > Also, if the crash kernel was not defined at bootime, there is no way to
> > > use the crash kernel.
> > >
> > > Enable runtime allocation of the crash_image if the crash_image is not
> > > defined statically. Following the same memory allocation/validation path
> > > that for the reboot kexec kernel.
> > >
> > > Signed-off-by: Ricardo Ribalda
> >
> > I don't think this patch will work as intended. For one you omit
> > setting the image->type to KEXEC_TYPE_CRASH. But when you grep for that
> > type you will find that there is a lot of special handling done for it.
> > I don't believe that this can simply be skipped without causing
> > problems.
> >
> > Furthermore I think you have missed one important detail. The memory
> > reserved for the crash kernel is not just a buffer for the image but
> > the memory it runs in! For that it has to be a continuous piece of
> > physical memory with usually some additional arch specific limitations.
> > When allocated dynamically all those limitations need to be considered.
> > But a standard kexec doesn't care about those limitations as it doesn't
> > care about the os running before itself. It can simply overwrite the
> > memory when booting. But if the crash kernel does the same it will
> > corrupt the dump it is supposed to generate.
>
> Right now, I do not intend to use it to fetch a kdump, I am using it
> as the image that will run when the system crashes.
the crash_image is currently all about creating a dump. If you want to
change that you need to discuss the new behavior in the commit message!
Please update the commit message.
Thanks
Philipp
>
> It seems to work fine on the two devices that I am using for tests.
>
> >
> > Thanks
> > Philipp
> >
> > > ---
> > > kexec: Enable runtime allocation of crash_image
> > >
> > > To: Eric Biederman
> > > Cc: kexec@lists.infradead.org
> > > Cc: linux-ker...@vger.kernel.org
> > > Cc: Sergey Senozhatsky
> > > Cc: linux-ker...@vger.kernel.org
> > > Cc: Ross Zwisler
> > > Cc: Philipp Rudo
> > > Cc: Baoquan He
> > > ---
> > > include/linux/kexec.h | 1 +
> > > kernel/kexec.c| 9 +
> > > kernel/kexec_core.c | 5 +
> > > kernel/kexec_file.c | 7 ---
> > > 4 files changed, 15 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > > index 41a686996aaa..98ca9a32bc8e 100644
> > > --- a/include/linux/kexec.h
> > > +++ b/include/linux/kexec.h
> > > @@ -427,6 +427,7 @@ extern int kexec_load_disabled;
> > > extern bool kexec_in_progress;
> > >
> > > int crash_shrink_memory(unsigned long new_size);
> > > +bool __crash_memory_valid(void);
> > > ssize_t crash_get_memory_size(void);
> > >
> > > #ifndef arch_kexec_protect_crashkres
> > > diff --git a/kernel/kexec.c b/kernel/kexec.c
> > > index cb8e6e6f983c..b5c17db25e88 100644
> > > --- a/kernel/kexec.c
> > > +++ b/kernel/kexec.c
> > > @@ -28,7 +28,7 @@ static int kimage_alloc_init(struct kimage **rimage,
> > > unsigned long entry,
> > > struct kimage *image;
> > > bool kexec_on_panic = flags & KEXEC_ON_CRASH;
> > >
> > > - if (kexec_on_panic) {
> > > + if (kexec_on_panic && __crash_memory_valid()) {
> > > /* Verify we have a valid entry point */
> > > if ((entry < phys_to_boot_phys(crashk_res.start)) ||
> > > (entry > phys_to_boot_phys(crashk_res.end)))
> > > @@ -44,7 +44,7 @@ static int kimage_alloc_init(struct kimage **rimage,
> > > unsigned long entry,
> > > image->nr_segments = nr_segments;
> > > memcpy(image->segment, segments, nr_segments * sizeof(*segments));
> > >
> > > - if (kexec_on_panic) {
> > > + if (kexec_on_panic && __crash_memory_valid()) {
> > > /* Enable special crash kernel control page alloc policy. */
> > > image->control_page = crashk_res.start;
> > > image->type = KEXEC_TYPE_CRASH;
> > > @@ -101,7 +101,7 @@ static int do_kexec_load(unsigned long entry,
> > > unsigned long nr_segments,
> > >
> > > if (flags & KEXEC_ON_CRASH) {
> > > dest_image = &kexec_crash_image;
> > > - if (kexec_crash_image)
> > > + if (kexec_crash_image && __crash_memory_valid())
> > > arch_kexec_unprotect_crashkres();
> > > } else {
> > > dest_image = &kexec_image;
> > > @@ -157,7 +157,8 @@ static int do_kexec_load(unsigned long entry,
> > > unsigned long nr_segments,
> > > image = xchg(dest_i
Re: [PATCH v1 2/2] kexec: Introduce kexec_reboot_disabled
On Tue, 29 Nov 2022 14:44:50 +0100 Philipp Rudo 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 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v1 2/2] kexec: Introduce kexec_reboot_disabled
Hi Steven, On Mon, 28 Nov 2022 11:42:00 -0500 Steven Rostedt wrote: > On Thu, 24 Nov 2022 16:01:15 +0100 > Philipp Rudo 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-d78361e99...@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 > ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
