Re: [PATCH 2/4] kexec: add CRASH_ZEROIZE to wipe secrets before kdump

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Calling ktime_get() in the panic path after machine_crash_shutdown() can permanently hang the crash transition, preventing kdump from working.
- [Low] Conditionally exposing a raw notifier list without an API wrapper forces subsystems to use #ifdef and risks build breaks.

Pre-existing issues:
- [High] ktime_get_real_seconds() in crash_save_vmcoreinfo() deadlocks on 32-bit systems if the local CPU interrupted a timekeeping update.
--

--- Patch [2]: [PATCH 2/4] kexec: add CRASH_ZEROIZE to wipe secrets before kdump ---
commit e20f9219f7505b929366dca07a7894f6b666a4b5
Author: Jan Sebastian Götte <[email protected]>
Subject: kexec: add CRASH_ZEROIZE to wipe secrets before kdump

This patch adds a new CRASH_ZEROIZE opt-in atomic notifier chain to scrub
sensitive secrets before launching a kdump kernel. The zeroization runs late
in __crash_kexec() after other CPUs are shut down.

> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index c1dee3f971a91..685ed8926966d 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h

[ ... ]

> @@ -34,6 +35,10 @@ static inline void arch_kexec_protect_crashkres(void) { }
>  static inline void arch_kexec_unprotect_crashkres(void) { }
>  #endif
>  
> +#ifdef CONFIG_CRASH_ZEROIZE
> +extern struct atomic_notifier_head crash_zeroize_notifier_list;
> +#endif
> +

[Severity: Low]
Does exposing this raw notifier list conditionally force subsystems to use
ifdefs when registering callbacks?

Usually, kernel APIs provide a wrapper (like
register_crash_zeroize_notifier()) with a static inline no-op for disabled
configurations to avoid breaking the build.

>  #ifndef arch_crash_handle_hotplug_event
>  static inline void arch_crash_handle_hotplug_event(struct kimage *image, void *arg) { }
>  #endif

[ ... ]

> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 4f21fc3b108b8..49192b20dc9f6 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c

[ ... ]

> @@ -33,6 +34,22 @@
>  /* Per cpu memory for storing cpu states in case of system crash. */
>  note_buf_t __percpu *crash_notes;
>  
> +#ifdef CONFIG_CRASH_ZEROIZE
> +ATOMIC_NOTIFIER_HEAD(crash_zeroize_notifier_list);
> +EXPORT_SYMBOL_GPL(crash_zeroize_notifier_list);
> +
> +static void crash_zeroize(void)
> +{
> +	ktime_t zeroize_start = ktime_get();

[Severity: High]
Can calling ktime_get() cause a deadlock here and on completion?

Since crash_zeroize() runs after machine_crash_shutdown() has forcefully
halted all other CPUs, if any of those halted CPUs were in the middle of a
timekeeping update and held the tk_core.seq write lock, this loop in
ktime_get():

kernel/time/timekeeping.c:ktime_get() {
    ...
    do {
        seq = read_seqcount_begin(&tk_core.seq);
        ...
    } while (read_seqcount_retry(&tk_core.seq, seq));
    ...
}

would spin indefinitely, preventing the kdump kernel from booting. Should this
use a lockless accessor like ktime_get_mono_fast_ns() instead?

> +
> +	pr_info("Wiping sensitive secrets...\n");
> +	atomic_notifier_call_chain(&crash_zeroize_notifier_list, 0, NULL);
> +	pr_info("Done in %lld us\n", ktime_us_delta(ktime_get(), zeroize_start));
> +}
> +#else
> +static inline void crash_zeroize(void) { }
> +#endif /* CONFIG_CRASH_ZEROIZE */
> +

[ ... ]

> @@ -142,6 +159,7 @@ void __noclone __crash_kexec(struct pt_regs *regs)
>  			crash_save_vmcoreinfo();

[Severity: High]
This is a pre-existing issue, but can crash_save_vmcoreinfo() also deadlock
during a panic?

If a local CPU interrupts a timekeeping update (holding the timekeeper write
seqcount) via an NMI or hardirq, and then triggers a panic, it will call
crash_save_vmcoreinfo() which eventually calls ktime_get_real_seconds():

kernel/vmcore_info.c:crash_save_vmcoreinfo() {
    ...
    vmcoreinfo_append_str("CRASHTIME=%lld\n", ktime_get_real_seconds());
    ...
}

On 32-bit platforms, this falls back to reading tk_core.seq, which the local
CPU already holds for write, leading to an infinite spin and preventing the
crash dump.

>  			machine_crash_shutdown(&fixed_regs);
>  			crash_cma_clear_pending_dma();
>  			crash_zeroize();
>  			machine_kexec(kexec_crash_image);
>  		}
>  		kexec_unlock();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2