diff mbox series

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

Message ID 20221104113000.487098-5-coxu@redhat.com
State New
Headers
Series Support kdump with LUKS encryption by reusing LUKS volume key |

Commit Message

Coiby Xu Nov. 4, 2022, 11:29 a.m. UTC 
  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 <kernelfans@gmail.com>
Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 arch/x86/kernel/machine_kexec_64.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Dave Hansen Nov. 4, 2022, 2:38 p.m. UTC | #1 
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.
Coiby Xu Nov. 7, 2022, 11:20 a.m. UTC | #2 
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/20221104113000.487098-5-coxu@redhat.com/t/
to read related emails for now.


>
diff mbox series

Patch

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)