[PATCH v31 05/12] arm64: kdump: protect crash dump kernel memory

[AKASHI Takahiro]

James,

On Thu, Feb 02, 2017 at 10:45:58AM +0000, James Morse wrote:
> Hi Akashi, Mark,
> 
> On 01/02/17 18:25, Mark Rutland wrote:
> > On Wed, Feb 01, 2017 at 06:00:08PM +0000, Mark Rutland wrote:
> >> On Wed, Feb 01, 2017 at 09:46:24PM +0900, AKASHI Takahiro wrote:
> >>> arch_kexec_protect_crashkres() and arch_kexec_unprotect_crashkres()
> >>> are meant to be called around kexec_load() in order to protect
> >>> the memory allocated for crash dump kernel once after it's loaded.
> >>>
> >>> The protection is implemented here by unmapping the region rather than
> >>> making it read-only.
> >>> To make the things work correctly, we also have to
> >>> - put the region in an isolated, page-level mapping initially, and
> >>> - move copying kexec's control_code_page to machine_kexec_prepare()
> >>>
> >>> Note that page-level mapping is also required to allow for shrinking
> >>> the size of memory, through /sys/kernel/kexec_crash_size, by any number
> >>> of multiple pages.
> 
> >> Looking at kexec_crash_size_store(), I don't see where memory returned
> >> to the OS is mapped. AFAICT, if the region is protected when the user
> >> shrinks the region, the memory will not be mapped, yet handed over to
> >> the kernel for general allocation.
> 
> kernel/kexec_core.c:crash_shrink_memory() will bailout:
> >	if (kexec_crash_image) {
> >		ret = -ENOENT;
> >		goto unlock;
> >	}
> 
> So it should only be possible to return memory to the allocators when there is
> no crash image loaded, so the area is mapped.
> 
> What happens when we unload the crash image? It looks like an unload is a call

Thank you for this heads-up!
I've almost forgot this feature.

> to do_kexec_load with nr_segments == 0, do_kexec_load() has:
> >	if (flags & KEXEC_ON_CRASH) {
> >		dest_image = &kexec_crash_image;
> >		if (kexec_crash_image)
> >			arch_kexec_unprotect_crashkres();
> 
> So we unprotect the region when we unload the kernel causing it to be remapped.
> Provided the load/protect and {load,unload}/unprotect are kept in sync, I think
> this is safe.
> 
> 
> Given the core code can unload a crash image, this hunk has me worried:
> +void arch_kexec_unprotect_crashkres(void)
> +{
> +	/*
> +	 * We don't have to make page-level mappings here because
> +	 * the crash dump kernel memory is not allowed to be shrunk
> +	 * once the kernel is loaded.
> +	 */
> +	create_pgd_mapping(&init_mm, crashk_res.start,
> +			__phys_to_virt(crashk_res.start),
> +			resource_size(&crashk_res), PAGE_KERNEL,
> +			debug_pagealloc_enabled());
> 
> 
> I don't think this is true if the order is: load -> protect, unload ->
> unprotect, shrink. The shrink will happen with potentially non-page-size mappings.

So we have to always do page-mapping, here.

-Takahiro AKASHI

> 
> Thanks,
> 
> James
>