2022-11-04 11:44:10

by Coiby Xu

Subject: [RFC v2 4/5] x86/crash: make the page that stores the LUKS volume key inaccessible

This adds an addition layer of protection for the saved copy of LUKS
volume key. Trying to access the saved copy will cause page fault.

Suggested-by: Pingfan Liu <[email protected]>
Signed-off-by: Coiby Xu <[email protected]>
---
arch/x86/kernel/machine_kexec_64.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index 0611fd83858e..f3d51c38a1c9 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -557,9 +557,25 @@ static void kexec_mark_crashkres(bool protect)
kexec_mark_range(control, crashk_res.end, protect);
}

+static void kexec_mark_luks_volume_key_inaccessible(void)
+{
+ unsigned long start, end;
+ struct page *page;
+ unsigned int nr_pages;
+
+ if (kexec_crash_image->luks_volume_key_addr) {
+ start = kexec_crash_image->luks_volume_key_addr;
+ end = start + kexec_crash_image->luks_volume_key_sz - 1;
+ page = pfn_to_page(start >> PAGE_SHIFT);
+ nr_pages = (end >> PAGE_SHIFT) - (start >> PAGE_SHIFT) + 1;
+ set_memory_np((unsigned long)page_address(page), nr_pages);
+ }
+}
+
void arch_kexec_protect_crashkres(void)
{
kexec_mark_crashkres(true);
+ kexec_mark_luks_volume_key_inaccessible();
}

void arch_kexec_unprotect_crashkres(void)
--
2.37.3

2022-11-04 15:21:42

by Dave Hansen

Subject: Re: [RFC v2 4/5] x86/crash: make the page that stores the LUKS volume key inaccessible

On 11/4/22 04:29, Coiby Xu wrote:
> + if (kexec_crash_image->luks_volume_key_addr) {
> + start = kexec_crash_image->luks_volume_key_addr;
> + end = start + kexec_crash_image->luks_volume_key_sz - 1;
> + page = pfn_to_page(start >> PAGE_SHIFT);
> + nr_pages = (end >> PAGE_SHIFT) - (start >> PAGE_SHIFT) + 1;
> + set_memory_np((unsigned long)page_address(page), nr_pages);
> + }

Why does this go pfn -> page -> vaddr? What good does having the page
do? Can you just do phys_to_virt() on the start address? Maybe:

start_paddr = kexec_crash_image->luks_volume_key_addr;
end_paddr = start_paddr + kexec_crash_image->luks_volume_key_sz - 1;
nr_pages = (PAGE_ALIGN(end_paddr) - PAGE_ALIGN_DOWN(start_paddr))/
PAGE_SIZE;
set_memory_np((unsigned long)phys_to_virt(start_paddr), nr_pages);

Also, if you resend this, please just cc the x86 folks on the series.
The other patches and cover letter have desperately needed context
around this.

2022-11-07 11:45:18

by Coiby Xu

Subject: Re: [RFC v2 4/5] x86/crash: make the page that stores the LUKS volume key inaccessible

Hi Dave,

Thanks for the quick review!

On Fri, Nov 04, 2022 at 07:38:17AM -0700, Dave Hansen wrote:
>On 11/4/22 04:29, Coiby Xu wrote:
>> + if (kexec_crash_image->luks_volume_key_addr) {
>> + start = kexec_crash_image->luks_volume_key_addr;
>> + end = start + kexec_crash_image->luks_volume_key_sz - 1;
>> + page = pfn_to_page(start >> PAGE_SHIFT);
>> + nr_pages = (end >> PAGE_SHIFT) - (start >> PAGE_SHIFT) + 1;
>> + set_memory_np((unsigned long)page_address(page), nr_pages);
>> + }
>
>Why does this go pfn -> page -> vaddr? What good does having the page
>do?

Sorry it's an imitation of kexec_mark_crashkres.

> Can you just do phys_to_virt() on the start address? Maybe:
>
> start_paddr = kexec_crash_image->luks_volume_key_addr;
> end_paddr = start_paddr + kexec_crash_image->luks_volume_key_sz - 1;
> nr_pages = (PAGE_ALIGN(end_paddr) - PAGE_ALIGN_DOWN(start_paddr))/
>PAGE_SIZE;
> set_memory_np((unsigned long)phys_to_virt(start_paddr), nr_pages);

Thanks for suggesting a smarter implementation! I'll apply it to next
version.

>
>Also, if you resend this, please just cc the x86 folks on the series.
>The other patches and cover letter have desperately needed context
>around this.

Sure, I'll cc the x86 list the complete patch set next time. Sorry
you'll have to go to
https://lore.kernel.org/lkml/[email protected]/t/
to read related emails for now.


>

--
Best regards,
Coiby