[PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Coiby Xu]

v1
 - "Put the luks key handling related to crash_dump out into a separate
   file kernel/crash_dump_luks.c" [Baoquan]
 - Put the generic luks handling code before the x86 specific code to
   make it easier for other arches to follow suit [Baoquan]
 - Use phys_to_virt instead of "pfn -> page -> vaddr" [Dave Hansen]
 - Drop the RFC prefix [Dave Young]
 - Rebase the code onto latest Linus tree (6.4.0-rc4)

RFC v2
 - libcryptsetup interacts with the kernel via sysfs instead of "hacking"
   dm-crypt 
   - to save a kdump copy of the LUKS volume key in 1st kernel
   - to add a logon key using the copy for libcryptsetup in kdump kernel [Milan]
   - to avoid the incorrect usage of LUKS master key in dm-crypt [Milan]
 - save the kdump copy of LUKS volume key randomly [Jan]
 - mark the kdump copy inaccessible [Pingfan]
 - Miscellaneous
   - explain when operations related to the LUKS volume key happen [Jan]
   - s/master key/volume key/g
   - use crash_ instead of kexec_ as function prefix
   - fix commit subject prefixes e.g. "x86, kdump" to x86/crash

LUKS is the standard for Linux disk encryption. Many users choose LUKS
and in some use cases like Confidential VM it's mandated. With kdump
enabled, when the 1st kernel crashes, the system could boot into the
kdump/crash kernel and dump the memory image i.e. /proc/vmcore to a
specified target. Currently, when dumping vmcore to a LUKS
encrypted device, there are two problems,

 - Kdump kernel may not be able to decrypt the LUKS partition. For some
   machines, a system administrator may not have a chance to enter the
   password to decrypt the device in kdump initramfs after the 1st kernel
   crashes; For cloud confidential VMs, the kdump kernel can't unseal
   the key with TPM.

 - LUKS2 by default use the memory-hard Argon2 key derivation function
   which is quite memory-consuming compared to the limited memory reserved
   for kdump. Take Fedora example, by default, only 256M is reserved for
   systems having memory between 4G-64G. With LUKS enabled, ~1300M needs
   to be reserved for kdump. Note if the memory reserved for kdump can't
   be used by 1st kernel i.e. an user sees ~1300M memory missing in the
   1st kernel. 
 
Besides users (at least for Fedora) usually expect kdump to work out of
the box i.e. no manual password input is needed. And it doesn't make
sense to derivate the key again in kdump kernel which seems to be
redundant work.

Based on Milan's feedback [1] on Kairui's ideas to support kdump with
LUKS encryption, this patch set addresses the above issues by reusing
the LUKS volume key in kdump kernel and here is the life cycle of this
kdump copy of LUKS volume key,

 1. After the 1st kernel loads the initramfs during boot, systemd
    asks for a passphrase from the user and uses it to de-crypt the LUKS
    volume key

 2. After the 1st kernel saving the volume key as a logon key,
    libcrytpsetup notifies the kernel to read this logon key and store a
    temporary copy by writing the key description to
    /sys/kernel/crash_luks_volume_key

 3. After switching to the real root fs, kdump.serivce is started and it 
    loads the kdump kernel using the kexec_file_load syscall

 4. The kexec_file_load syscall saves the temporary copy of the volume
    key to kdump reserved memory and wipe the copy.

 5. When the 1st kernel crashes and kdump kernel is booted,
    libcryptsetup asks the kdump kernel to add a logon key using
    the volume key stored in kdump reserved memory by writing the key
    description to /sys/kernel/crash_luks_volume_key

 6. The system gets rebooted to the 1st kernel after dumping vmcore to
    the LUKS encrypted device is finished

Note the kdump copy of LUKS volume key never leaves the kernel space and
is saved in the memory area exclusively reserved for kdump where even
the 1st kernel has no direct access. 

Milan's major concern [2] on previous version is "storing the encryption
key to yet another place are creating another attack vector". To further
secure this copy, two additional protections are added,
 - save the copy randomly in kdump reserved memory as suggested by Jan
 - clear the _PAGE_PRESENT flag of the page that stores the copy as
   suggested by Pingfan

If there is no further security concern with this approach or any other
concern, I will drop the following assumptions,
  - only x86 is supported
  - there is only one LUKS device for the system

to extend the support to other architectures including POWER, ARM and
s390x and address the case of multiple LUKS devices. Any feedback will be 
appreciated, thanks!

For a proof of concept, I've patched cryptsetup [3] in a quick-and-dirty
way to support a new option "--kdump-kernel-master-key"
and hacked systemd [4]. It works for Fedora 37.

[1] https://yhbt.net/lore/all/e5abd089-3398-fdb4-7991-0019be434b79@gmail.com/
[2] https://lwn.net/ml/linux-kernel/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
[3] https://gitlab.com/coxu/cryptsetup/-/commit/750a46d933fac82e0c994b5c41de40a0b8cac647
[4] https://github.com/coiby/systemd/tree/reuse_kdump_master_key

Coiby Xu (5):
  kexec_file: allow to place kexec_buf randomly
  crash_dump: save the LUKS volume key temporarily
  crash_dump: retrieve LUKS volume key in kdump kernel
  x86/crash: pass the LUKS volume key to kdump kernel
  x86/crash: make the page that stores the LUKS volume key inaccessible

 arch/x86/include/asm/crash.h       |   1 +
 arch/x86/kernel/crash.c            |  47 ++++++-
 arch/x86/kernel/kexec-bzimage64.c  |   7 +
 arch/x86/kernel/machine_kexec_64.c |  14 ++
 include/linux/crash_core.h         |   2 +
 include/linux/crash_dump.h         |   2 +
 include/linux/kexec.h              |   6 +
 kernel/Makefile                    |   2 +-
 kernel/crash_dump_luks.c           | 202 +++++++++++++++++++++++++++++
 kernel/kexec_file.c                |  15 +++
 kernel/ksysfs.c                    |  19 +++
 11 files changed, 315 insertions(+), 2 deletions(-)
 create mode 100644 kernel/crash_dump_luks.c

-- 
2.40.1

[PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Coiby Xu]

v1
 - "Put the luks key handling related to crash_dump out into a separate
   file kernel/crash_dump_luks.c" [Baoquan]
 - Put the generic luks handling code before the x86 specific code to
   make it easier for other arches to follow suit [Baoquan]
 - Use phys_to_virt instead of "pfn -> page -> vaddr" [Dave Hansen]
 - Drop the RFC prefix [Dave Young]
 - Rebase the code onto latest Linus tree (6.4.0-rc4)

RFC v2
 - libcryptsetup interacts with the kernel via sysfs instead of "hacking"
   dm-crypt 
   - to save a kdump copy of the LUKS volume key in 1st kernel
   - to add a logon key using the copy for libcryptsetup in kdump kernel [Milan]
   - to avoid the incorrect usage of LUKS master key in dm-crypt [Milan]
 - save the kdump copy of LUKS volume key randomly [Jan]
 - mark the kdump copy inaccessible [Pingfan]
 - Miscellaneous
   - explain when operations related to the LUKS volume key happen [Jan]
   - s/master key/volume key/g
   - use crash_ instead of kexec_ as function prefix
   - fix commit subject prefixes e.g. "x86, kdump" to x86/crash

LUKS is the standard for Linux disk encryption. Many users choose LUKS
and in some use cases like Confidential VM it's mandated. With kdump
enabled, when the 1st kernel crashes, the system could boot into the
kdump/crash kernel and dump the memory image i.e. /proc/vmcore to a
specified target. Currently, when dumping vmcore to a LUKS
encrypted device, there are two problems,

 - Kdump kernel may not be able to decrypt the LUKS partition. For some
   machines, a system administrator may not have a chance to enter the
   password to decrypt the device in kdump initramfs after the 1st kernel
   crashes; For cloud confidential VMs, the kdump kernel can't unseal
   the key with TPM.

 - LUKS2 by default use the memory-hard Argon2 key derivation function
   which is quite memory-consuming compared to the limited memory reserved
   for kdump. Take Fedora example, by default, only 256M is reserved for
   systems having memory between 4G-64G. With LUKS enabled, ~1300M needs
   to be reserved for kdump. Note if the memory reserved for kdump can't
   be used by 1st kernel i.e. an user sees ~1300M memory missing in the
   1st kernel. 
 
Besides users (at least for Fedora) usually expect kdump to work out of
the box i.e. no manual password input is needed. And it doesn't make
sense to derivate the key again in kdump kernel which seems to be
redundant work.

Based on Milan's feedback [1] on Kairui's ideas to support kdump with
LUKS encryption, this patch set addresses the above issues by reusing
the LUKS volume key in kdump kernel and here is the life cycle of this
kdump copy of LUKS volume key,

 1. After the 1st kernel loads the initramfs during boot, systemd
    asks for a passphrase from the user and uses it to de-crypt the LUKS
    volume key

 2. After the 1st kernel saving the volume key as a logon key,
    libcrytpsetup notifies the kernel to read this logon key and store a
    temporary copy by writing the key description to
    /sys/kernel/crash_luks_volume_key

 3. After switching to the real root fs, kdump.serivce is started and it 
    loads the kdump kernel using the kexec_file_load syscall

 4. The kexec_file_load syscall saves the temporary copy of the volume
    key to kdump reserved memory and wipe the copy.

 5. When the 1st kernel crashes and kdump kernel is booted,
    libcryptsetup asks the kdump kernel to add a logon key using
    the volume key stored in kdump reserved memory by writing the key
    description to /sys/kernel/crash_luks_volume_key

 6. The system gets rebooted to the 1st kernel after dumping vmcore to
    the LUKS encrypted device is finished

Note the kdump copy of LUKS volume key never leaves the kernel space and
is saved in the memory area exclusively reserved for kdump where even
the 1st kernel has no direct access. 

Milan's major concern [2] on previous version is "storing the encryption
key to yet another place are creating another attack vector". To further
secure this copy, two additional protections are added,
 - save the copy randomly in kdump reserved memory as suggested by Jan
 - clear the _PAGE_PRESENT flag of the page that stores the copy as
   suggested by Pingfan

If there is no further security concern with this approach or any other
concern, I will drop the following assumptions,
  - only x86 is supported
  - there is only one LUKS device for the system

to extend the support to other architectures including POWER, ARM and
s390x and address the case of multiple LUKS devices. Any feedback will be 
appreciated, thanks!

For a proof of concept, I've patched cryptsetup [3] in a quick-and-dirty
way to support a new option "--kdump-kernel-master-key"
and hacked systemd [4]. It works for Fedora 37.

[1] https://yhbt.net/lore/all/e5abd089-3398-fdb4-7991-0019be434b79@gmail.com/
[2] https://lwn.net/ml/linux-kernel/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
[3] https://gitlab.com/coxu/cryptsetup/-/commit/750a46d933fac82e0c994b5c41de40a0b8cac647
[4] https://github.com/coiby/systemd/tree/reuse_kdump_master_key

Coiby Xu (5):
  kexec_file: allow to place kexec_buf randomly
  crash_dump: save the LUKS volume key temporarily
  crash_dump: retrieve LUKS volume key in kdump kernel
  x86/crash: pass the LUKS volume key to kdump kernel
  x86/crash: make the page that stores the LUKS volume key inaccessible

 arch/x86/include/asm/crash.h       |   1 +
 arch/x86/kernel/crash.c            |  47 ++++++-
 arch/x86/kernel/kexec-bzimage64.c  |   7 +
 arch/x86/kernel/machine_kexec_64.c |  14 ++
 include/linux/crash_core.h         |   2 +
 include/linux/crash_dump.h         |   2 +
 include/linux/kexec.h              |   6 +
 kernel/Makefile                    |   2 +-
 kernel/crash_dump_luks.c           | 202 +++++++++++++++++++++++++++++
 kernel/kexec_file.c                |  15 +++
 kernel/ksysfs.c                    |  19 +++
 11 files changed, 315 insertions(+), 2 deletions(-)
 create mode 100644 kernel/crash_dump_luks.c

-- 
2.40.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[dm-devel] [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Coiby Xu]

v1
 - "Put the luks key handling related to crash_dump out into a separate
   file kernel/crash_dump_luks.c" [Baoquan]
 - Put the generic luks handling code before the x86 specific code to
   make it easier for other arches to follow suit [Baoquan]
 - Use phys_to_virt instead of "pfn -> page -> vaddr" [Dave Hansen]
 - Drop the RFC prefix [Dave Young]
 - Rebase the code onto latest Linus tree (6.4.0-rc4)

RFC v2
 - libcryptsetup interacts with the kernel via sysfs instead of "hacking"
   dm-crypt 
   - to save a kdump copy of the LUKS volume key in 1st kernel
   - to add a logon key using the copy for libcryptsetup in kdump kernel [Milan]
   - to avoid the incorrect usage of LUKS master key in dm-crypt [Milan]
 - save the kdump copy of LUKS volume key randomly [Jan]
 - mark the kdump copy inaccessible [Pingfan]
 - Miscellaneous
   - explain when operations related to the LUKS volume key happen [Jan]
   - s/master key/volume key/g
   - use crash_ instead of kexec_ as function prefix
   - fix commit subject prefixes e.g. "x86, kdump" to x86/crash

LUKS is the standard for Linux disk encryption. Many users choose LUKS
and in some use cases like Confidential VM it's mandated. With kdump
enabled, when the 1st kernel crashes, the system could boot into the
kdump/crash kernel and dump the memory image i.e. /proc/vmcore to a
specified target. Currently, when dumping vmcore to a LUKS
encrypted device, there are two problems,

 - Kdump kernel may not be able to decrypt the LUKS partition. For some
   machines, a system administrator may not have a chance to enter the
   password to decrypt the device in kdump initramfs after the 1st kernel
   crashes; For cloud confidential VMs, the kdump kernel can't unseal
   the key with TPM.

 - LUKS2 by default use the memory-hard Argon2 key derivation function
   which is quite memory-consuming compared to the limited memory reserved
   for kdump. Take Fedora example, by default, only 256M is reserved for
   systems having memory between 4G-64G. With LUKS enabled, ~1300M needs
   to be reserved for kdump. Note if the memory reserved for kdump can't
   be used by 1st kernel i.e. an user sees ~1300M memory missing in the
   1st kernel. 
 
Besides users (at least for Fedora) usually expect kdump to work out of
the box i.e. no manual password input is needed. And it doesn't make
sense to derivate the key again in kdump kernel which seems to be
redundant work.

Based on Milan's feedback [1] on Kairui's ideas to support kdump with
LUKS encryption, this patch set addresses the above issues by reusing
the LUKS volume key in kdump kernel and here is the life cycle of this
kdump copy of LUKS volume key,

 1. After the 1st kernel loads the initramfs during boot, systemd
    asks for a passphrase from the user and uses it to de-crypt the LUKS
    volume key

 2. After the 1st kernel saving the volume key as a logon key,
    libcrytpsetup notifies the kernel to read this logon key and store a
    temporary copy by writing the key description to
    /sys/kernel/crash_luks_volume_key

 3. After switching to the real root fs, kdump.serivce is started and it 
    loads the kdump kernel using the kexec_file_load syscall

 4. The kexec_file_load syscall saves the temporary copy of the volume
    key to kdump reserved memory and wipe the copy.

 5. When the 1st kernel crashes and kdump kernel is booted,
    libcryptsetup asks the kdump kernel to add a logon key using
    the volume key stored in kdump reserved memory by writing the key
    description to /sys/kernel/crash_luks_volume_key

 6. The system gets rebooted to the 1st kernel after dumping vmcore to
    the LUKS encrypted device is finished

Note the kdump copy of LUKS volume key never leaves the kernel space and
is saved in the memory area exclusively reserved for kdump where even
the 1st kernel has no direct access. 

Milan's major concern [2] on previous version is "storing the encryption
key to yet another place are creating another attack vector". To further
secure this copy, two additional protections are added,
 - save the copy randomly in kdump reserved memory as suggested by Jan
 - clear the _PAGE_PRESENT flag of the page that stores the copy as
   suggested by Pingfan

If there is no further security concern with this approach or any other
concern, I will drop the following assumptions,
  - only x86 is supported
  - there is only one LUKS device for the system

to extend the support to other architectures including POWER, ARM and
s390x and address the case of multiple LUKS devices. Any feedback will be 
appreciated, thanks!

For a proof of concept, I've patched cryptsetup [3] in a quick-and-dirty
way to support a new option "--kdump-kernel-master-key"
and hacked systemd [4]. It works for Fedora 37.

[1] https://yhbt.net/lore/all/e5abd089-3398-fdb4-7991-0019be434b79@gmail.com/
[2] https://lwn.net/ml/linux-kernel/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
[3] https://gitlab.com/coxu/cryptsetup/-/commit/750a46d933fac82e0c994b5c41de40a0b8cac647
[4] https://github.com/coiby/systemd/tree/reuse_kdump_master_key

Coiby Xu (5):
  kexec_file: allow to place kexec_buf randomly
  crash_dump: save the LUKS volume key temporarily
  crash_dump: retrieve LUKS volume key in kdump kernel
  x86/crash: pass the LUKS volume key to kdump kernel
  x86/crash: make the page that stores the LUKS volume key inaccessible

 arch/x86/include/asm/crash.h       |   1 +
 arch/x86/kernel/crash.c            |  47 ++++++-
 arch/x86/kernel/kexec-bzimage64.c  |   7 +
 arch/x86/kernel/machine_kexec_64.c |  14 ++
 include/linux/crash_core.h         |   2 +
 include/linux/crash_dump.h         |   2 +
 include/linux/kexec.h              |   6 +
 kernel/Makefile                    |   2 +-
 kernel/crash_dump_luks.c           | 202 +++++++++++++++++++++++++++++
 kernel/kexec_file.c                |  15 +++
 kernel/ksysfs.c                    |  19 +++
 11 files changed, 315 insertions(+), 2 deletions(-)
 create mode 100644 kernel/crash_dump_luks.c

-- 
2.40.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 1/5] kexec_file: allow to place kexec_buf randomly

[Coiby Xu]

Currently, kexec_buf is placed in order which means for the same
machine, the info in the kexec_buf is always located at the same
position each time the machine is booted. This may cause a risk for
sensitive information like LUKS volume key. Now struct kexec_buf has a
new field random which indicates it's supposed to be placed in a random
position.

Suggested-by: Jan Pazdziora <jpazdziora@redhat.com>
Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 include/linux/kexec.h |  2 ++
 kernel/kexec_file.c   | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 22b5cd24f581..5b2440444112 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -172,6 +172,7 @@ int kexec_image_post_load_cleanup_default(struct kimage *image);
  * @buf_min:	The buffer can't be placed below this address.
  * @buf_max:	The buffer can't be placed above this address.
  * @top_down:	Allocate from top of memory.
+ * @random:	Place the buffer at a random position.
  */
 struct kexec_buf {
 	struct kimage *image;
@@ -183,6 +184,7 @@ struct kexec_buf {
 	unsigned long buf_min;
 	unsigned long buf_max;
 	bool top_down;
+	bool random;
 };
 
 int kexec_load_purgatory(struct kimage *image, struct kexec_buf *kbuf);
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index f989f5f1933b..5dbfc119eb6a 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -25,6 +25,7 @@
 #include <linux/elfcore.h>
 #include <linux/kernel.h>
 #include <linux/kernel_read_file.h>
+#include <linux/prandom.h>
 #include <linux/syscalls.h>
 #include <linux/vmalloc.h>
 #include "kexec_internal.h"
@@ -419,6 +420,16 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 	return ret;
 }
 
+static unsigned long kexec_random_start(unsigned long start, unsigned long end)
+{
+	unsigned long temp_start;
+	unsigned short i;
+
+	get_random_bytes(&i, sizeof(unsigned short));
+	temp_start = start + (end - start) / USHRT_MAX * i;
+	return temp_start;
+}
+
 static int locate_mem_hole_top_down(unsigned long start, unsigned long end,
 				    struct kexec_buf *kbuf)
 {
@@ -427,6 +438,8 @@ static int locate_mem_hole_top_down(unsigned long start, unsigned long end,
 
 	temp_end = min(end, kbuf->buf_max);
 	temp_start = temp_end - kbuf->memsz;
+	if (kbuf->random)
+		temp_start = kexec_random_start(temp_start, temp_end);
 
 	do {
 		/* align down start */
@@ -464,6 +477,8 @@ static int locate_mem_hole_bottom_up(unsigned long start, unsigned long end,
 	unsigned long temp_start, temp_end;
 
 	temp_start = max(start, kbuf->buf_min);
+	if (kbuf->random)
+		temp_start = kexec_random_start(temp_start, end);
 
 	do {
 		temp_start = ALIGN(temp_start, kbuf->buf_align);
-- 
2.40.1

[PATCH 1/5] kexec_file: allow to place kexec_buf randomly

[Coiby Xu]

Currently, kexec_buf is placed in order which means for the same
machine, the info in the kexec_buf is always located at the same
position each time the machine is booted. This may cause a risk for
sensitive information like LUKS volume key. Now struct kexec_buf has a
new field random which indicates it's supposed to be placed in a random
position.

Suggested-by: Jan Pazdziora <jpazdziora@redhat.com>
Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 include/linux/kexec.h |  2 ++
 kernel/kexec_file.c   | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 22b5cd24f581..5b2440444112 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -172,6 +172,7 @@ int kexec_image_post_load_cleanup_default(struct kimage *image);
  * @buf_min:	The buffer can't be placed below this address.
  * @buf_max:	The buffer can't be placed above this address.
  * @top_down:	Allocate from top of memory.
+ * @random:	Place the buffer at a random position.
  */
 struct kexec_buf {
 	struct kimage *image;
@@ -183,6 +184,7 @@ struct kexec_buf {
 	unsigned long buf_min;
 	unsigned long buf_max;
 	bool top_down;
+	bool random;
 };
 
 int kexec_load_purgatory(struct kimage *image, struct kexec_buf *kbuf);
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index f989f5f1933b..5dbfc119eb6a 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -25,6 +25,7 @@
 #include <linux/elfcore.h>
 #include <linux/kernel.h>
 #include <linux/kernel_read_file.h>
+#include <linux/prandom.h>
 #include <linux/syscalls.h>
 #include <linux/vmalloc.h>
 #include "kexec_internal.h"
@@ -419,6 +420,16 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 	return ret;
 }
 
+static unsigned long kexec_random_start(unsigned long start, unsigned long end)
+{
+	unsigned long temp_start;
+	unsigned short i;
+
+	get_random_bytes(&i, sizeof(unsigned short));
+	temp_start = start + (end - start) / USHRT_MAX * i;
+	return temp_start;
+}
+
 static int locate_mem_hole_top_down(unsigned long start, unsigned long end,
 				    struct kexec_buf *kbuf)
 {
@@ -427,6 +438,8 @@ static int locate_mem_hole_top_down(unsigned long start, unsigned long end,
 
 	temp_end = min(end, kbuf->buf_max);
 	temp_start = temp_end - kbuf->memsz;
+	if (kbuf->random)
+		temp_start = kexec_random_start(temp_start, temp_end);
 
 	do {
 		/* align down start */
@@ -464,6 +477,8 @@ static int locate_mem_hole_bottom_up(unsigned long start, unsigned long end,
 	unsigned long temp_start, temp_end;
 
 	temp_start = max(start, kbuf->buf_min);
+	if (kbuf->random)
+		temp_start = kexec_random_start(temp_start, end);
 
 	do {
 		temp_start = ALIGN(temp_start, kbuf->buf_align);
-- 
2.40.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[dm-devel] [PATCH 1/5] kexec_file: allow to place kexec_buf randomly

[Coiby Xu]

Currently, kexec_buf is placed in order which means for the same
machine, the info in the kexec_buf is always located at the same
position each time the machine is booted. This may cause a risk for
sensitive information like LUKS volume key. Now struct kexec_buf has a
new field random which indicates it's supposed to be placed in a random
position.

Suggested-by: Jan Pazdziora <jpazdziora@redhat.com>
Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 include/linux/kexec.h |  2 ++
 kernel/kexec_file.c   | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 22b5cd24f581..5b2440444112 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -172,6 +172,7 @@ int kexec_image_post_load_cleanup_default(struct kimage *image);
  * @buf_min:	The buffer can't be placed below this address.
  * @buf_max:	The buffer can't be placed above this address.
  * @top_down:	Allocate from top of memory.
+ * @random:	Place the buffer at a random position.
  */
 struct kexec_buf {
 	struct kimage *image;
@@ -183,6 +184,7 @@ struct kexec_buf {
 	unsigned long buf_min;
 	unsigned long buf_max;
 	bool top_down;
+	bool random;
 };
 
 int kexec_load_purgatory(struct kimage *image, struct kexec_buf *kbuf);
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index f989f5f1933b..5dbfc119eb6a 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -25,6 +25,7 @@
 #include <linux/elfcore.h>
 #include <linux/kernel.h>
 #include <linux/kernel_read_file.h>
+#include <linux/prandom.h>
 #include <linux/syscalls.h>
 #include <linux/vmalloc.h>
 #include "kexec_internal.h"
@@ -419,6 +420,16 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 	return ret;
 }
 
+static unsigned long kexec_random_start(unsigned long start, unsigned long end)
+{
+	unsigned long temp_start;
+	unsigned short i;
+
+	get_random_bytes(&i, sizeof(unsigned short));
+	temp_start = start + (end - start) / USHRT_MAX * i;
+	return temp_start;
+}
+
 static int locate_mem_hole_top_down(unsigned long start, unsigned long end,
 				    struct kexec_buf *kbuf)
 {
@@ -427,6 +438,8 @@ static int locate_mem_hole_top_down(unsigned long start, unsigned long end,
 
 	temp_end = min(end, kbuf->buf_max);
 	temp_start = temp_end - kbuf->memsz;
+	if (kbuf->random)
+		temp_start = kexec_random_start(temp_start, temp_end);
 
 	do {
 		/* align down start */
@@ -464,6 +477,8 @@ static int locate_mem_hole_bottom_up(unsigned long start, unsigned long end,
 	unsigned long temp_start, temp_end;
 
 	temp_start = max(start, kbuf->buf_min);
+	if (kbuf->random)
+		temp_start = kexec_random_start(temp_start, end);
 
 	do {
 		temp_start = ALIGN(temp_start, kbuf->buf_align);
-- 
2.40.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 2/5] crash_dump: save the LUKS volume key temporarily

[Coiby Xu]

After having the volume key, crytpsetup/systemd-cryptsetup saves the
volume key as a logon key to its thread keyring and this key is
destroyed immediately with the terminated thread. So a temporary copy of
the volume key is needed in order to later save it to kdump reserved
memory when the crash kernel is loaded later.

crytpsetup/systemd-cryptsetup will write the key description to
/sys/kernel/crash_luks_volume_key so the kernel will read the logon key
and save a temporary copy for later user. kdump has 1 hour at maximum to
get the temporary copy before the key gets wiped. And after kdump
retrieves the key, the key will be wiped immediately.

Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 include/linux/crash_core.h |  2 +
 kernel/Makefile            |  2 +-
 kernel/crash_dump_luks.c   | 90 ++++++++++++++++++++++++++++++++++++++
 kernel/ksysfs.c            | 19 ++++++++
 4 files changed, 112 insertions(+), 1 deletion(-)
 create mode 100644 kernel/crash_dump_luks.c

diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index de62a722431e..596d83b8f362 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -83,5 +83,7 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
 		unsigned long long *crash_size, unsigned long long *crash_base);
 int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
 		unsigned long long *crash_size, unsigned long long *crash_base);
+int crash_sysfs_luks_volume_key_write(const char *key_des, size_t count);
+int crash_pass_temp_luks_volume_key(void **addr, unsigned long *sz);
 
 #endif /* LINUX_CRASH_CORE_H */
diff --git a/kernel/Makefile b/kernel/Makefile
index b69c95315480..8412afa4a9f0 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -118,7 +118,7 @@ obj-$(CONFIG_PERF_EVENTS) += events/
 
 obj-$(CONFIG_USER_RETURN_NOTIFIER) += user-return-notifier.o
 obj-$(CONFIG_PADATA) += padata.o
-obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
+obj-$(CONFIG_CRASH_DUMP) += crash_dump.o crash_dump_luks.o
 obj-$(CONFIG_JUMP_LABEL) += jump_label.o
 obj-$(CONFIG_CONTEXT_TRACKING) += context_tracking.o
 obj-$(CONFIG_TORTURE_TEST) += torture.o
diff --git a/kernel/crash_dump_luks.c b/kernel/crash_dump_luks.c
new file mode 100644
index 000000000000..2d88b77a93f8
--- /dev/null
+++ b/kernel/crash_dump_luks.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <keys/user-type.h>
+#include <linux/crash_dump.h>
+
+static u8 *luks_volume_key;
+static unsigned int luks_volume_key_size;
+
+void wipe_luks_volume_key(void)
+{
+	if (luks_volume_key) {
+		memset(luks_volume_key, 0, luks_volume_key_size * sizeof(u8));
+		kfree(luks_volume_key);
+		luks_volume_key = NULL;
+	}
+}
+
+static void _wipe_luks_volume_key(struct work_struct *dummy)
+{
+	wipe_luks_volume_key();
+}
+
+static DECLARE_DELAYED_WORK(wipe_luks_volume_key_work, _wipe_luks_volume_key);
+
+static unsigned __read_mostly wipe_key_delay = 3600; /* 1 hour */
+
+static int crash_save_temp_luks_volume_key(const char *key_desc, size_t count)
+{
+	const struct user_key_payload *ukp;
+	struct key *key;
+
+
+	if (luks_volume_key) {
+		memset(luks_volume_key, 0, luks_volume_key_size * sizeof(u8));
+		kfree(luks_volume_key);
+	}
+
+	pr_debug("Requesting key %s", key_desc);
+	key = request_key(&key_type_logon, key_desc, NULL);
+
+	if (IS_ERR(key)) {
+		pr_debug("No such key %s", key_desc);
+		return PTR_ERR(key);
+	}
+
+	ukp = user_key_payload_locked(key);
+	if (!ukp)
+		return -EKEYREVOKED;
+
+	luks_volume_key = kmalloc(ukp->datalen, GFP_KERNEL);
+	if (!luks_volume_key)
+		return -ENOMEM;
+	memcpy(luks_volume_key, ukp->data, ukp->datalen);
+	luks_volume_key_size = ukp->datalen;
+	pr_debug("LUKS master key (size=%u): %8ph\n", luks_volume_key_size, luks_volume_key);
+	schedule_delayed_work(&wipe_luks_volume_key_work,
+			      round_jiffies_relative(wipe_key_delay * HZ));
+	return 0;
+}
+
+int crash_sysfs_luks_volume_key_write(const char *key_desc, size_t count)
+{
+	if (!is_kdump_kernel())
+		return crash_save_temp_luks_volume_key(key_desc, count);
+	return -EINVAL;
+}
+EXPORT_SYMBOL(crash_sysfs_luks_volume_key_write);
+
+int crash_pass_temp_luks_volume_key(void **addr, unsigned long *sz)
+{
+	unsigned long luks_key_sz;
+	unsigned char *buf;
+	unsigned int *size_ptr;
+
+	if (!luks_volume_key)
+		return -EINVAL;
+
+	luks_key_sz = sizeof(unsigned int) + luks_volume_key_size * sizeof(u8);
+
+	buf = vzalloc(luks_key_sz);
+	if (!buf)
+		return -ENOMEM;
+
+	size_ptr = (unsigned int *)buf;
+	memcpy(size_ptr, &luks_volume_key_size, sizeof(unsigned int));
+	memcpy(size_ptr + 1, luks_volume_key, luks_volume_key_size * sizeof(u8));
+	*addr = buf;
+	*sz = luks_key_sz;
+	wipe_luks_volume_key();
+	return 0;
+}
diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index aad7a3bfd846..cc64a895c334 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -165,6 +165,24 @@ static ssize_t vmcoreinfo_show(struct kobject *kobj,
 }
 KERNEL_ATTR_RO(vmcoreinfo);
 
+static ssize_t crash_luks_volume_key_show(struct kobject *kobj,
+					  struct kobj_attribute *attr,
+					  char *buf)
+{
+	return 0;
+}
+
+static ssize_t crash_luks_volume_key_store(struct kobject *kobj,
+					   struct kobj_attribute *attr,
+					   const char *buf, size_t count)
+{
+	int ret;
+
+	ret = crash_sysfs_luks_volume_key_write(buf, count);
+	return ret < 0 ? ret : count;
+}
+KERNEL_ATTR_RW(crash_luks_volume_key);
+
 #endif /* CONFIG_CRASH_CORE */
 
 /* whether file capabilities are enabled */
@@ -255,6 +273,7 @@ static struct attribute * kernel_attrs[] = {
 #endif
 #ifdef CONFIG_CRASH_CORE
 	&vmcoreinfo_attr.attr,
+	&crash_luks_volume_key_attr.attr,
 #endif
 #ifndef CONFIG_TINY_RCU
 	&rcu_expedited_attr.attr,
-- 
2.40.1

[PATCH 2/5] crash_dump: save the LUKS volume key temporarily

[Coiby Xu]

After having the volume key, crytpsetup/systemd-cryptsetup saves the
volume key as a logon key to its thread keyring and this key is
destroyed immediately with the terminated thread. So a temporary copy of
the volume key is needed in order to later save it to kdump reserved
memory when the crash kernel is loaded later.

crytpsetup/systemd-cryptsetup will write the key description to
/sys/kernel/crash_luks_volume_key so the kernel will read the logon key
and save a temporary copy for later user. kdump has 1 hour at maximum to
get the temporary copy before the key gets wiped. And after kdump
retrieves the key, the key will be wiped immediately.

Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 include/linux/crash_core.h |  2 +
 kernel/Makefile            |  2 +-
 kernel/crash_dump_luks.c   | 90 ++++++++++++++++++++++++++++++++++++++
 kernel/ksysfs.c            | 19 ++++++++
 4 files changed, 112 insertions(+), 1 deletion(-)
 create mode 100644 kernel/crash_dump_luks.c

diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index de62a722431e..596d83b8f362 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -83,5 +83,7 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
 		unsigned long long *crash_size, unsigned long long *crash_base);
 int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
 		unsigned long long *crash_size, unsigned long long *crash_base);
+int crash_sysfs_luks_volume_key_write(const char *key_des, size_t count);
+int crash_pass_temp_luks_volume_key(void **addr, unsigned long *sz);
 
 #endif /* LINUX_CRASH_CORE_H */
diff --git a/kernel/Makefile b/kernel/Makefile
index b69c95315480..8412afa4a9f0 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -118,7 +118,7 @@ obj-$(CONFIG_PERF_EVENTS) += events/
 
 obj-$(CONFIG_USER_RETURN_NOTIFIER) += user-return-notifier.o
 obj-$(CONFIG_PADATA) += padata.o
-obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
+obj-$(CONFIG_CRASH_DUMP) += crash_dump.o crash_dump_luks.o
 obj-$(CONFIG_JUMP_LABEL) += jump_label.o
 obj-$(CONFIG_CONTEXT_TRACKING) += context_tracking.o
 obj-$(CONFIG_TORTURE_TEST) += torture.o
diff --git a/kernel/crash_dump_luks.c b/kernel/crash_dump_luks.c
new file mode 100644
index 000000000000..2d88b77a93f8
--- /dev/null
+++ b/kernel/crash_dump_luks.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <keys/user-type.h>
+#include <linux/crash_dump.h>
+
+static u8 *luks_volume_key;
+static unsigned int luks_volume_key_size;
+
+void wipe_luks_volume_key(void)
+{
+	if (luks_volume_key) {
+		memset(luks_volume_key, 0, luks_volume_key_size * sizeof(u8));
+		kfree(luks_volume_key);
+		luks_volume_key = NULL;
+	}
+}
+
+static void _wipe_luks_volume_key(struct work_struct *dummy)
+{
+	wipe_luks_volume_key();
+}
+
+static DECLARE_DELAYED_WORK(wipe_luks_volume_key_work, _wipe_luks_volume_key);
+
+static unsigned __read_mostly wipe_key_delay = 3600; /* 1 hour */
+
+static int crash_save_temp_luks_volume_key(const char *key_desc, size_t count)
+{
+	const struct user_key_payload *ukp;
+	struct key *key;
+
+
+	if (luks_volume_key) {
+		memset(luks_volume_key, 0, luks_volume_key_size * sizeof(u8));
+		kfree(luks_volume_key);
+	}
+
+	pr_debug("Requesting key %s", key_desc);
+	key = request_key(&key_type_logon, key_desc, NULL);
+
+	if (IS_ERR(key)) {
+		pr_debug("No such key %s", key_desc);
+		return PTR_ERR(key);
+	}
+
+	ukp = user_key_payload_locked(key);
+	if (!ukp)
+		return -EKEYREVOKED;
+
+	luks_volume_key = kmalloc(ukp->datalen, GFP_KERNEL);
+	if (!luks_volume_key)
+		return -ENOMEM;
+	memcpy(luks_volume_key, ukp->data, ukp->datalen);
+	luks_volume_key_size = ukp->datalen;
+	pr_debug("LUKS master key (size=%u): %8ph\n", luks_volume_key_size, luks_volume_key);
+	schedule_delayed_work(&wipe_luks_volume_key_work,
+			      round_jiffies_relative(wipe_key_delay * HZ));
+	return 0;
+}
+
+int crash_sysfs_luks_volume_key_write(const char *key_desc, size_t count)
+{
+	if (!is_kdump_kernel())
+		return crash_save_temp_luks_volume_key(key_desc, count);
+	return -EINVAL;
+}
+EXPORT_SYMBOL(crash_sysfs_luks_volume_key_write);
+
+int crash_pass_temp_luks_volume_key(void **addr, unsigned long *sz)
+{
+	unsigned long luks_key_sz;
+	unsigned char *buf;
+	unsigned int *size_ptr;
+
+	if (!luks_volume_key)
+		return -EINVAL;
+
+	luks_key_sz = sizeof(unsigned int) + luks_volume_key_size * sizeof(u8);
+
+	buf = vzalloc(luks_key_sz);
+	if (!buf)
+		return -ENOMEM;
+
+	size_ptr = (unsigned int *)buf;
+	memcpy(size_ptr, &luks_volume_key_size, sizeof(unsigned int));
+	memcpy(size_ptr + 1, luks_volume_key, luks_volume_key_size * sizeof(u8));
+	*addr = buf;
+	*sz = luks_key_sz;
+	wipe_luks_volume_key();
+	return 0;
+}
diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index aad7a3bfd846..cc64a895c334 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -165,6 +165,24 @@ static ssize_t vmcoreinfo_show(struct kobject *kobj,
 }
 KERNEL_ATTR_RO(vmcoreinfo);
 
+static ssize_t crash_luks_volume_key_show(struct kobject *kobj,
+					  struct kobj_attribute *attr,
+					  char *buf)
+{
+	return 0;
+}
+
+static ssize_t crash_luks_volume_key_store(struct kobject *kobj,
+					   struct kobj_attribute *attr,
+					   const char *buf, size_t count)
+{
+	int ret;
+
+	ret = crash_sysfs_luks_volume_key_write(buf, count);
+	return ret < 0 ? ret : count;
+}
+KERNEL_ATTR_RW(crash_luks_volume_key);
+
 #endif /* CONFIG_CRASH_CORE */
 
 /* whether file capabilities are enabled */
@@ -255,6 +273,7 @@ static struct attribute * kernel_attrs[] = {
 #endif
 #ifdef CONFIG_CRASH_CORE
 	&vmcoreinfo_attr.attr,
+	&crash_luks_volume_key_attr.attr,
 #endif
 #ifndef CONFIG_TINY_RCU
 	&rcu_expedited_attr.attr,
-- 
2.40.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[dm-devel] [PATCH 2/5] crash_dump: save the LUKS volume key temporarily

[Coiby Xu]

After having the volume key, crytpsetup/systemd-cryptsetup saves the
volume key as a logon key to its thread keyring and this key is
destroyed immediately with the terminated thread. So a temporary copy of
the volume key is needed in order to later save it to kdump reserved
memory when the crash kernel is loaded later.

crytpsetup/systemd-cryptsetup will write the key description to
/sys/kernel/crash_luks_volume_key so the kernel will read the logon key
and save a temporary copy for later user. kdump has 1 hour at maximum to
get the temporary copy before the key gets wiped. And after kdump
retrieves the key, the key will be wiped immediately.

Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 include/linux/crash_core.h |  2 +
 kernel/Makefile            |  2 +-
 kernel/crash_dump_luks.c   | 90 ++++++++++++++++++++++++++++++++++++++
 kernel/ksysfs.c            | 19 ++++++++
 4 files changed, 112 insertions(+), 1 deletion(-)
 create mode 100644 kernel/crash_dump_luks.c

diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index de62a722431e..596d83b8f362 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -83,5 +83,7 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
 		unsigned long long *crash_size, unsigned long long *crash_base);
 int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
 		unsigned long long *crash_size, unsigned long long *crash_base);
+int crash_sysfs_luks_volume_key_write(const char *key_des, size_t count);
+int crash_pass_temp_luks_volume_key(void **addr, unsigned long *sz);
 
 #endif /* LINUX_CRASH_CORE_H */
diff --git a/kernel/Makefile b/kernel/Makefile
index b69c95315480..8412afa4a9f0 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -118,7 +118,7 @@ obj-$(CONFIG_PERF_EVENTS) += events/
 
 obj-$(CONFIG_USER_RETURN_NOTIFIER) += user-return-notifier.o
 obj-$(CONFIG_PADATA) += padata.o
-obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
+obj-$(CONFIG_CRASH_DUMP) += crash_dump.o crash_dump_luks.o
 obj-$(CONFIG_JUMP_LABEL) += jump_label.o
 obj-$(CONFIG_CONTEXT_TRACKING) += context_tracking.o
 obj-$(CONFIG_TORTURE_TEST) += torture.o
diff --git a/kernel/crash_dump_luks.c b/kernel/crash_dump_luks.c
new file mode 100644
index 000000000000..2d88b77a93f8
--- /dev/null
+++ b/kernel/crash_dump_luks.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <keys/user-type.h>
+#include <linux/crash_dump.h>
+
+static u8 *luks_volume_key;
+static unsigned int luks_volume_key_size;
+
+void wipe_luks_volume_key(void)
+{
+	if (luks_volume_key) {
+		memset(luks_volume_key, 0, luks_volume_key_size * sizeof(u8));
+		kfree(luks_volume_key);
+		luks_volume_key = NULL;
+	}
+}
+
+static void _wipe_luks_volume_key(struct work_struct *dummy)
+{
+	wipe_luks_volume_key();
+}
+
+static DECLARE_DELAYED_WORK(wipe_luks_volume_key_work, _wipe_luks_volume_key);
+
+static unsigned __read_mostly wipe_key_delay = 3600; /* 1 hour */
+
+static int crash_save_temp_luks_volume_key(const char *key_desc, size_t count)
+{
+	const struct user_key_payload *ukp;
+	struct key *key;
+
+
+	if (luks_volume_key) {
+		memset(luks_volume_key, 0, luks_volume_key_size * sizeof(u8));
+		kfree(luks_volume_key);
+	}
+
+	pr_debug("Requesting key %s", key_desc);
+	key = request_key(&key_type_logon, key_desc, NULL);
+
+	if (IS_ERR(key)) {
+		pr_debug("No such key %s", key_desc);
+		return PTR_ERR(key);
+	}
+
+	ukp = user_key_payload_locked(key);
+	if (!ukp)
+		return -EKEYREVOKED;
+
+	luks_volume_key = kmalloc(ukp->datalen, GFP_KERNEL);
+	if (!luks_volume_key)
+		return -ENOMEM;
+	memcpy(luks_volume_key, ukp->data, ukp->datalen);
+	luks_volume_key_size = ukp->datalen;
+	pr_debug("LUKS master key (size=%u): %8ph\n", luks_volume_key_size, luks_volume_key);
+	schedule_delayed_work(&wipe_luks_volume_key_work,
+			      round_jiffies_relative(wipe_key_delay * HZ));
+	return 0;
+}
+
+int crash_sysfs_luks_volume_key_write(const char *key_desc, size_t count)
+{
+	if (!is_kdump_kernel())
+		return crash_save_temp_luks_volume_key(key_desc, count);
+	return -EINVAL;
+}
+EXPORT_SYMBOL(crash_sysfs_luks_volume_key_write);
+
+int crash_pass_temp_luks_volume_key(void **addr, unsigned long *sz)
+{
+	unsigned long luks_key_sz;
+	unsigned char *buf;
+	unsigned int *size_ptr;
+
+	if (!luks_volume_key)
+		return -EINVAL;
+
+	luks_key_sz = sizeof(unsigned int) + luks_volume_key_size * sizeof(u8);
+
+	buf = vzalloc(luks_key_sz);
+	if (!buf)
+		return -ENOMEM;
+
+	size_ptr = (unsigned int *)buf;
+	memcpy(size_ptr, &luks_volume_key_size, sizeof(unsigned int));
+	memcpy(size_ptr + 1, luks_volume_key, luks_volume_key_size * sizeof(u8));
+	*addr = buf;
+	*sz = luks_key_sz;
+	wipe_luks_volume_key();
+	return 0;
+}
diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index aad7a3bfd846..cc64a895c334 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -165,6 +165,24 @@ static ssize_t vmcoreinfo_show(struct kobject *kobj,
 }
 KERNEL_ATTR_RO(vmcoreinfo);
 
+static ssize_t crash_luks_volume_key_show(struct kobject *kobj,
+					  struct kobj_attribute *attr,
+					  char *buf)
+{
+	return 0;
+}
+
+static ssize_t crash_luks_volume_key_store(struct kobject *kobj,
+					   struct kobj_attribute *attr,
+					   const char *buf, size_t count)
+{
+	int ret;
+
+	ret = crash_sysfs_luks_volume_key_write(buf, count);
+	return ret < 0 ? ret : count;
+}
+KERNEL_ATTR_RW(crash_luks_volume_key);
+
 #endif /* CONFIG_CRASH_CORE */
 
 /* whether file capabilities are enabled */
@@ -255,6 +273,7 @@ static struct attribute * kernel_attrs[] = {
 #endif
 #ifdef CONFIG_CRASH_CORE
 	&vmcoreinfo_attr.attr,
+	&crash_luks_volume_key_attr.attr,
 #endif
 #ifndef CONFIG_TINY_RCU
 	&rcu_expedited_attr.attr,
-- 
2.40.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 3/5] crash_dump: retrieve LUKS volume key in kdump kernel

[Coiby Xu]

Crash kernel will retrieve the LUKS volume key based on the
luksvolumekey command line parameter. When libcryptsetup writes the key
description to /sys/kernel/crash_luks_volume_key, crash kernel will
create a thread keyring and add a logon key.

Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 include/linux/crash_dump.h |   2 +
 kernel/crash_dump_luks.c   | 116 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 116 insertions(+), 2 deletions(-)

diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
index 0f3a656293b0..bc848e058c64 100644
--- a/include/linux/crash_dump.h
+++ b/include/linux/crash_dump.h
@@ -15,6 +15,8 @@
 extern unsigned long long elfcorehdr_addr;
 extern unsigned long long elfcorehdr_size;
 
+extern unsigned long long luks_volume_key_addr;
+
 #ifdef CONFIG_CRASH_DUMP
 extern int elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size);
 extern void elfcorehdr_free(unsigned long long addr);
diff --git a/kernel/crash_dump_luks.c b/kernel/crash_dump_luks.c
index 2d88b77a93f8..63718db318c3 100644
--- a/kernel/crash_dump_luks.c
+++ b/kernel/crash_dump_luks.c
@@ -1,7 +1,82 @@
 // SPDX-License-Identifier: GPL-2.0-only
+#include <linux/key.h>
+#include <linux/keyctl.h>
 #include <keys/user-type.h>
 #include <linux/crash_dump.h>
 
+unsigned long long luks_volume_key_addr;
+EXPORT_SYMBOL_GPL(luks_volume_key_addr);
+
+static int __init setup_luksvolumekey(char *arg)
+{
+	char *end;
+
+	if (!arg)
+		return -EINVAL;
+	luks_volume_key_addr = memparse(arg, &end);
+	if (end > arg)
+		return 0;
+
+	luks_volume_key_addr = 0;
+	return -EINVAL;
+}
+
+early_param("luksvolumekey", setup_luksvolumekey);
+
+/*
+ * Architectures may override this function to read LUKS master key
+ */
+ssize_t __weak luks_key_read(char *buf, size_t count, u64 *ppos)
+{
+	struct kvec kvec = { .iov_base = buf, .iov_len = count };
+	struct iov_iter iter;
+
+	iov_iter_kvec(&iter, READ, &kvec, 1, count);
+	return read_from_oldmem(&iter, count, ppos, false);
+}
+
+static int retrive_kdump_luks_volume_key(u8 *buffer, unsigned int *sz)
+{
+	unsigned int key_size;
+	size_t lukskeybuf_sz;
+	unsigned int *size_ptr;
+	char *lukskeybuf;
+	u64 addr;
+	int r;
+
+	if (luks_volume_key_addr == 0) {
+		pr_debug("LUKS master key memory address inaccessible");
+		return -EINVAL;
+	}
+
+	addr = luks_volume_key_addr;
+
+	/* Read LUKS master key size */
+	r = luks_key_read((char *)&key_size, sizeof(unsigned int), &addr);
+
+	if (r < 0)
+		return r;
+
+	pr_debug("Retrieve LUKS master key: size=%u\n", key_size);
+	/* Read in LUKS maste rkey */
+	lukskeybuf_sz = sizeof(unsigned int) + key_size * sizeof(u8);
+	lukskeybuf = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
+					      get_order(lukskeybuf_sz));
+	if (!lukskeybuf)
+		return -ENOMEM;
+
+	addr = luks_volume_key_addr;
+	r = luks_key_read((char *)lukskeybuf, lukskeybuf_sz, &addr);
+
+	if (r < 0)
+		return r;
+	size_ptr = (unsigned int *)lukskeybuf;
+	memcpy(buffer, size_ptr + 1, key_size * sizeof(u8));
+	pr_debug("Retrieve LUKS master key (size=%u): %48ph...\n", key_size, buffer);
+	*sz = key_size;
+	return 0;
+}
+
 static u8 *luks_volume_key;
 static unsigned int luks_volume_key_size;
 
@@ -23,12 +98,48 @@ static DECLARE_DELAYED_WORK(wipe_luks_volume_key_work, _wipe_luks_volume_key);
 
 static unsigned __read_mostly wipe_key_delay = 3600; /* 1 hour */
 
+static int retore_luks_volume_key_to_thread_keyring(const char *key_desc)
+{
+	key_ref_t keyring_ref, key_ref;
+	int ret;
+
+	/* find the target keyring (which must be writable) */
+	keyring_ref = lookup_user_key(KEY_SPEC_THREAD_KEYRING, 0x01, KEY_NEED_WRITE);
+	if (IS_ERR(keyring_ref)) {
+		pr_alert("Failed to get keyring");
+		return PTR_ERR(keyring_ref);
+	}
+
+	luks_volume_key = kmalloc(128, GFP_KERNEL);
+	ret = retrive_kdump_luks_volume_key(luks_volume_key, &luks_volume_key_size);
+	if (ret) {
+		kfree(luks_volume_key);
+		return ret;
+	}
+
+	/* create or update the requested key and add it to the target keyring */
+	key_ref = key_create_or_update(keyring_ref, "logon", key_desc,
+				       luks_volume_key, luks_volume_key_size,
+				       KEY_PERM_UNDEF, KEY_ALLOC_IN_QUOTA);
+
+	if (!IS_ERR(key_ref)) {
+		ret = key_ref_to_ptr(key_ref)->serial;
+		key_ref_put(key_ref);
+		pr_alert("Success adding key %s", key_desc);
+	} else {
+		ret = PTR_ERR(key_ref);
+		pr_alert("Error when adding key");
+	}
+
+	key_ref_put(keyring_ref);
+	return ret;
+}
+
 static int crash_save_temp_luks_volume_key(const char *key_desc, size_t count)
 {
 	const struct user_key_payload *ukp;
 	struct key *key;
 
-
 	if (luks_volume_key) {
 		memset(luks_volume_key, 0, luks_volume_key_size * sizeof(u8));
 		kfree(luks_volume_key);
@@ -61,7 +172,8 @@ int crash_sysfs_luks_volume_key_write(const char *key_desc, size_t count)
 {
 	if (!is_kdump_kernel())
 		return crash_save_temp_luks_volume_key(key_desc, count);
-	return -EINVAL;
+	else
+		return retore_luks_volume_key_to_thread_keyring(key_desc);
 }
 EXPORT_SYMBOL(crash_sysfs_luks_volume_key_write);
 
-- 
2.40.1

[PATCH 3/5] crash_dump: retrieve LUKS volume key in kdump kernel

[Coiby Xu]

Crash kernel will retrieve the LUKS volume key based on the
luksvolumekey command line parameter. When libcryptsetup writes the key
description to /sys/kernel/crash_luks_volume_key, crash kernel will
create a thread keyring and add a logon key.

Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 include/linux/crash_dump.h |   2 +
 kernel/crash_dump_luks.c   | 116 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 116 insertions(+), 2 deletions(-)

diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
index 0f3a656293b0..bc848e058c64 100644
--- a/include/linux/crash_dump.h
+++ b/include/linux/crash_dump.h
@@ -15,6 +15,8 @@
 extern unsigned long long elfcorehdr_addr;
 extern unsigned long long elfcorehdr_size;
 
+extern unsigned long long luks_volume_key_addr;
+
 #ifdef CONFIG_CRASH_DUMP
 extern int elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size);
 extern void elfcorehdr_free(unsigned long long addr);
diff --git a/kernel/crash_dump_luks.c b/kernel/crash_dump_luks.c
index 2d88b77a93f8..63718db318c3 100644
--- a/kernel/crash_dump_luks.c
+++ b/kernel/crash_dump_luks.c
@@ -1,7 +1,82 @@
 // SPDX-License-Identifier: GPL-2.0-only
+#include <linux/key.h>
+#include <linux/keyctl.h>
 #include <keys/user-type.h>
 #include <linux/crash_dump.h>
 
+unsigned long long luks_volume_key_addr;
+EXPORT_SYMBOL_GPL(luks_volume_key_addr);
+
+static int __init setup_luksvolumekey(char *arg)
+{
+	char *end;
+
+	if (!arg)
+		return -EINVAL;
+	luks_volume_key_addr = memparse(arg, &end);
+	if (end > arg)
+		return 0;
+
+	luks_volume_key_addr = 0;
+	return -EINVAL;
+}
+
+early_param("luksvolumekey", setup_luksvolumekey);
+
+/*
+ * Architectures may override this function to read LUKS master key
+ */
+ssize_t __weak luks_key_read(char *buf, size_t count, u64 *ppos)
+{
+	struct kvec kvec = { .iov_base = buf, .iov_len = count };
+	struct iov_iter iter;
+
+	iov_iter_kvec(&iter, READ, &kvec, 1, count);
+	return read_from_oldmem(&iter, count, ppos, false);
+}
+
+static int retrive_kdump_luks_volume_key(u8 *buffer, unsigned int *sz)
+{
+	unsigned int key_size;
+	size_t lukskeybuf_sz;
+	unsigned int *size_ptr;
+	char *lukskeybuf;
+	u64 addr;
+	int r;
+
+	if (luks_volume_key_addr == 0) {
+		pr_debug("LUKS master key memory address inaccessible");
+		return -EINVAL;
+	}
+
+	addr = luks_volume_key_addr;
+
+	/* Read LUKS master key size */
+	r = luks_key_read((char *)&key_size, sizeof(unsigned int), &addr);
+
+	if (r < 0)
+		return r;
+
+	pr_debug("Retrieve LUKS master key: size=%u\n", key_size);
+	/* Read in LUKS maste rkey */
+	lukskeybuf_sz = sizeof(unsigned int) + key_size * sizeof(u8);
+	lukskeybuf = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
+					      get_order(lukskeybuf_sz));
+	if (!lukskeybuf)
+		return -ENOMEM;
+
+	addr = luks_volume_key_addr;
+	r = luks_key_read((char *)lukskeybuf, lukskeybuf_sz, &addr);
+
+	if (r < 0)
+		return r;
+	size_ptr = (unsigned int *)lukskeybuf;
+	memcpy(buffer, size_ptr + 1, key_size * sizeof(u8));
+	pr_debug("Retrieve LUKS master key (size=%u): %48ph...\n", key_size, buffer);
+	*sz = key_size;
+	return 0;
+}
+
 static u8 *luks_volume_key;
 static unsigned int luks_volume_key_size;
 
@@ -23,12 +98,48 @@ static DECLARE_DELAYED_WORK(wipe_luks_volume_key_work, _wipe_luks_volume_key);
 
 static unsigned __read_mostly wipe_key_delay = 3600; /* 1 hour */
 
+static int retore_luks_volume_key_to_thread_keyring(const char *key_desc)
+{
+	key_ref_t keyring_ref, key_ref;
+	int ret;
+
+	/* find the target keyring (which must be writable) */
+	keyring_ref = lookup_user_key(KEY_SPEC_THREAD_KEYRING, 0x01, KEY_NEED_WRITE);
+	if (IS_ERR(keyring_ref)) {
+		pr_alert("Failed to get keyring");
+		return PTR_ERR(keyring_ref);
+	}
+
+	luks_volume_key = kmalloc(128, GFP_KERNEL);
+	ret = retrive_kdump_luks_volume_key(luks_volume_key, &luks_volume_key_size);
+	if (ret) {
+		kfree(luks_volume_key);
+		return ret;
+	}
+
+	/* create or update the requested key and add it to the target keyring */
+	key_ref = key_create_or_update(keyring_ref, "logon", key_desc,
+				       luks_volume_key, luks_volume_key_size,
+				       KEY_PERM_UNDEF, KEY_ALLOC_IN_QUOTA);
+
+	if (!IS_ERR(key_ref)) {
+		ret = key_ref_to_ptr(key_ref)->serial;
+		key_ref_put(key_ref);
+		pr_alert("Success adding key %s", key_desc);
+	} else {
+		ret = PTR_ERR(key_ref);
+		pr_alert("Error when adding key");
+	}
+
+	key_ref_put(keyring_ref);
+	return ret;
+}
+
 static int crash_save_temp_luks_volume_key(const char *key_desc, size_t count)
 {
 	const struct user_key_payload *ukp;
 	struct key *key;
 
-
 	if (luks_volume_key) {
 		memset(luks_volume_key, 0, luks_volume_key_size * sizeof(u8));
 		kfree(luks_volume_key);
@@ -61,7 +172,8 @@ int crash_sysfs_luks_volume_key_write(const char *key_desc, size_t count)
 {
 	if (!is_kdump_kernel())
 		return crash_save_temp_luks_volume_key(key_desc, count);
-	return -EINVAL;
+	else
+		return retore_luks_volume_key_to_thread_keyring(key_desc);
 }
 EXPORT_SYMBOL(crash_sysfs_luks_volume_key_write);
 
-- 
2.40.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[dm-devel] [PATCH 3/5] crash_dump: retrieve LUKS volume key in kdump kernel

[Coiby Xu]

Crash kernel will retrieve the LUKS volume key based on the
luksvolumekey command line parameter. When libcryptsetup writes the key
description to /sys/kernel/crash_luks_volume_key, crash kernel will
create a thread keyring and add a logon key.

Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 include/linux/crash_dump.h |   2 +
 kernel/crash_dump_luks.c   | 116 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 116 insertions(+), 2 deletions(-)

diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
index 0f3a656293b0..bc848e058c64 100644
--- a/include/linux/crash_dump.h
+++ b/include/linux/crash_dump.h
@@ -15,6 +15,8 @@
 extern unsigned long long elfcorehdr_addr;
 extern unsigned long long elfcorehdr_size;
 
+extern unsigned long long luks_volume_key_addr;
+
 #ifdef CONFIG_CRASH_DUMP
 extern int elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size);
 extern void elfcorehdr_free(unsigned long long addr);
diff --git a/kernel/crash_dump_luks.c b/kernel/crash_dump_luks.c
index 2d88b77a93f8..63718db318c3 100644
--- a/kernel/crash_dump_luks.c
+++ b/kernel/crash_dump_luks.c
@@ -1,7 +1,82 @@
 // SPDX-License-Identifier: GPL-2.0-only
+#include <linux/key.h>
+#include <linux/keyctl.h>
 #include <keys/user-type.h>
 #include <linux/crash_dump.h>
 
+unsigned long long luks_volume_key_addr;
+EXPORT_SYMBOL_GPL(luks_volume_key_addr);
+
+static int __init setup_luksvolumekey(char *arg)
+{
+	char *end;
+
+	if (!arg)
+		return -EINVAL;
+	luks_volume_key_addr = memparse(arg, &end);
+	if (end > arg)
+		return 0;
+
+	luks_volume_key_addr = 0;
+	return -EINVAL;
+}
+
+early_param("luksvolumekey", setup_luksvolumekey);
+
+/*
+ * Architectures may override this function to read LUKS master key
+ */
+ssize_t __weak luks_key_read(char *buf, size_t count, u64 *ppos)
+{
+	struct kvec kvec = { .iov_base = buf, .iov_len = count };
+	struct iov_iter iter;
+
+	iov_iter_kvec(&iter, READ, &kvec, 1, count);
+	return read_from_oldmem(&iter, count, ppos, false);
+}
+
+static int retrive_kdump_luks_volume_key(u8 *buffer, unsigned int *sz)
+{
+	unsigned int key_size;
+	size_t lukskeybuf_sz;
+	unsigned int *size_ptr;
+	char *lukskeybuf;
+	u64 addr;
+	int r;
+
+	if (luks_volume_key_addr == 0) {
+		pr_debug("LUKS master key memory address inaccessible");
+		return -EINVAL;
+	}
+
+	addr = luks_volume_key_addr;
+
+	/* Read LUKS master key size */
+	r = luks_key_read((char *)&key_size, sizeof(unsigned int), &addr);
+
+	if (r < 0)
+		return r;
+
+	pr_debug("Retrieve LUKS master key: size=%u\n", key_size);
+	/* Read in LUKS maste rkey */
+	lukskeybuf_sz = sizeof(unsigned int) + key_size * sizeof(u8);
+	lukskeybuf = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
+					      get_order(lukskeybuf_sz));
+	if (!lukskeybuf)
+		return -ENOMEM;
+
+	addr = luks_volume_key_addr;
+	r = luks_key_read((char *)lukskeybuf, lukskeybuf_sz, &addr);
+
+	if (r < 0)
+		return r;
+	size_ptr = (unsigned int *)lukskeybuf;
+	memcpy(buffer, size_ptr + 1, key_size * sizeof(u8));
+	pr_debug("Retrieve LUKS master key (size=%u): %48ph...\n", key_size, buffer);
+	*sz = key_size;
+	return 0;
+}
+
 static u8 *luks_volume_key;
 static unsigned int luks_volume_key_size;
 
@@ -23,12 +98,48 @@ static DECLARE_DELAYED_WORK(wipe_luks_volume_key_work, _wipe_luks_volume_key);
 
 static unsigned __read_mostly wipe_key_delay = 3600; /* 1 hour */
 
+static int retore_luks_volume_key_to_thread_keyring(const char *key_desc)
+{
+	key_ref_t keyring_ref, key_ref;
+	int ret;
+
+	/* find the target keyring (which must be writable) */
+	keyring_ref = lookup_user_key(KEY_SPEC_THREAD_KEYRING, 0x01, KEY_NEED_WRITE);
+	if (IS_ERR(keyring_ref)) {
+		pr_alert("Failed to get keyring");
+		return PTR_ERR(keyring_ref);
+	}
+
+	luks_volume_key = kmalloc(128, GFP_KERNEL);
+	ret = retrive_kdump_luks_volume_key(luks_volume_key, &luks_volume_key_size);
+	if (ret) {
+		kfree(luks_volume_key);
+		return ret;
+	}
+
+	/* create or update the requested key and add it to the target keyring */
+	key_ref = key_create_or_update(keyring_ref, "logon", key_desc,
+				       luks_volume_key, luks_volume_key_size,
+				       KEY_PERM_UNDEF, KEY_ALLOC_IN_QUOTA);
+
+	if (!IS_ERR(key_ref)) {
+		ret = key_ref_to_ptr(key_ref)->serial;
+		key_ref_put(key_ref);
+		pr_alert("Success adding key %s", key_desc);
+	} else {
+		ret = PTR_ERR(key_ref);
+		pr_alert("Error when adding key");
+	}
+
+	key_ref_put(keyring_ref);
+	return ret;
+}
+
 static int crash_save_temp_luks_volume_key(const char *key_desc, size_t count)
 {
 	const struct user_key_payload *ukp;
 	struct key *key;
 
-
 	if (luks_volume_key) {
 		memset(luks_volume_key, 0, luks_volume_key_size * sizeof(u8));
 		kfree(luks_volume_key);
@@ -61,7 +172,8 @@ int crash_sysfs_luks_volume_key_write(const char *key_desc, size_t count)
 {
 	if (!is_kdump_kernel())
 		return crash_save_temp_luks_volume_key(key_desc, count);
-	return -EINVAL;
+	else
+		return retore_luks_volume_key_to_thread_keyring(key_desc);
 }
 EXPORT_SYMBOL(crash_sysfs_luks_volume_key_write);
 
-- 
2.40.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 4/5] x86/crash: pass the LUKS volume key to kdump kernel

[Coiby Xu]

1st kernel will build up the kernel command parameter luksvolumekey as
similar to elfcorehdr to pass the memory address of the stored info of
LUKS volume key to kdump kernel.

Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 arch/x86/include/asm/crash.h      |  1 +
 arch/x86/kernel/crash.c           | 47 ++++++++++++++++++++++++++++++-
 arch/x86/kernel/kexec-bzimage64.c |  7 +++++
 include/linux/kexec.h             |  4 +++
 4 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/crash.h b/arch/x86/include/asm/crash.h
index 8b6bd63530dc..485f75dce2ca 100644
--- a/arch/x86/include/asm/crash.h
+++ b/arch/x86/include/asm/crash.h
@@ -4,6 +4,7 @@
 
 struct kimage;
 
+int crash_load_luks_volume_key(struct kimage *image);
 int crash_load_segments(struct kimage *image);
 int crash_setup_memmap_entries(struct kimage *image,
 		struct boot_params *params);
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index cdd92ab43cda..32d4a9e3badf 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -289,6 +289,7 @@ static int memmap_exclude_ranges(struct kimage *image, struct crash_mem *cmem,
 				 unsigned long long mend)
 {
 	unsigned long start, end;
+	int r;
 
 	cmem->ranges[0].start = mstart;
 	cmem->ranges[0].end = mend;
@@ -297,7 +298,19 @@ static int memmap_exclude_ranges(struct kimage *image, struct crash_mem *cmem,
 	/* Exclude elf header region */
 	start = image->elf_load_addr;
 	end = start + image->elf_headers_sz - 1;
-	return crash_exclude_mem_range(cmem, start, end);
+	r = crash_exclude_mem_range(cmem, start, end);
+
+	if (r)
+		return r;
+
+	/* Exclude LUKS volume key region */
+	if (image->luks_volume_key_addr) {
+		start = image->luks_volume_key_addr;
+		end = start + image->luks_volume_key_sz - 1;
+		return crash_exclude_mem_range(cmem, start, end);
+	}
+
+	return r;
 }
 
 /* Prepare memory map for crash dump kernel */
@@ -368,6 +381,38 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
 	return ret;
 }
 
+int crash_load_luks_volume_key(struct kimage *image)
+{
+	int ret;
+	struct kexec_buf kbuf = {
+		.image = image,
+		.buf_min = 0,
+		.buf_max = ULONG_MAX,
+		.top_down = false,
+		.random = true,
+	};
+
+	image->luks_volume_key_addr = 0;
+	ret = crash_pass_temp_luks_volume_key(&kbuf.buffer, &kbuf.bufsz);
+	if (ret)
+		return ret;
+
+	kbuf.memsz = kbuf.bufsz;
+	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
+	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
+	ret = kexec_add_buffer(&kbuf);
+	if (ret) {
+		vfree((void *)kbuf.buffer);
+		return ret;
+	}
+	image->luks_volume_key_addr = kbuf.mem;
+	image->luks_volume_key_sz = kbuf.bufsz;
+	pr_debug("Loaded LUKS volume key at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+		 image->luks_volume_key_addr, kbuf.bufsz, kbuf.bufsz);
+
+	return ret;
+}
+
 int crash_load_segments(struct kimage *image)
 {
 	int ret;
diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index a61c12c01270..a859e9e5c876 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -76,6 +76,10 @@ static int setup_cmdline(struct kimage *image, struct boot_params *params,
 	if (image->type == KEXEC_TYPE_CRASH) {
 		len = sprintf(cmdline_ptr,
 			"elfcorehdr=0x%lx ", image->elf_load_addr);
+
+		if (image->luks_volume_key_addr != 0)
+			len += sprintf(cmdline_ptr + len,
+					"luksvolumekey=0x%lx ", image->luks_volume_key_addr);
 	}
 	memcpy(cmdline_ptr + len, cmdline, cmdline_len);
 	cmdline_len += len;
@@ -433,6 +437,9 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
 		ret = crash_load_segments(image);
 		if (ret)
 			return ERR_PTR(ret);
+		ret = crash_load_luks_volume_key(image);
+		if (ret)
+			pr_debug("Either no LUKS volume key or error to retrieve the LUKS volume key\n");
 	}
 
 	/*
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 5b2440444112..20d213076e46 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -374,6 +374,10 @@ struct kimage {
 	void *elf_headers;
 	unsigned long elf_headers_sz;
 	unsigned long elf_load_addr;
+
+	/* LUKS volume key buffer */
+	unsigned long luks_volume_key_addr;
+	unsigned long luks_volume_key_sz;
 };
 
 /* kexec interface functions */
-- 
2.40.1

[PATCH 4/5] x86/crash: pass the LUKS volume key to kdump kernel

[Coiby Xu]

1st kernel will build up the kernel command parameter luksvolumekey as
similar to elfcorehdr to pass the memory address of the stored info of
LUKS volume key to kdump kernel.

Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 arch/x86/include/asm/crash.h      |  1 +
 arch/x86/kernel/crash.c           | 47 ++++++++++++++++++++++++++++++-
 arch/x86/kernel/kexec-bzimage64.c |  7 +++++
 include/linux/kexec.h             |  4 +++
 4 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/crash.h b/arch/x86/include/asm/crash.h
index 8b6bd63530dc..485f75dce2ca 100644
--- a/arch/x86/include/asm/crash.h
+++ b/arch/x86/include/asm/crash.h
@@ -4,6 +4,7 @@
 
 struct kimage;
 
+int crash_load_luks_volume_key(struct kimage *image);
 int crash_load_segments(struct kimage *image);
 int crash_setup_memmap_entries(struct kimage *image,
 		struct boot_params *params);
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index cdd92ab43cda..32d4a9e3badf 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -289,6 +289,7 @@ static int memmap_exclude_ranges(struct kimage *image, struct crash_mem *cmem,
 				 unsigned long long mend)
 {
 	unsigned long start, end;
+	int r;
 
 	cmem->ranges[0].start = mstart;
 	cmem->ranges[0].end = mend;
@@ -297,7 +298,19 @@ static int memmap_exclude_ranges(struct kimage *image, struct crash_mem *cmem,
 	/* Exclude elf header region */
 	start = image->elf_load_addr;
 	end = start + image->elf_headers_sz - 1;
-	return crash_exclude_mem_range(cmem, start, end);
+	r = crash_exclude_mem_range(cmem, start, end);
+
+	if (r)
+		return r;
+
+	/* Exclude LUKS volume key region */
+	if (image->luks_volume_key_addr) {
+		start = image->luks_volume_key_addr;
+		end = start + image->luks_volume_key_sz - 1;
+		return crash_exclude_mem_range(cmem, start, end);
+	}
+
+	return r;
 }
 
 /* Prepare memory map for crash dump kernel */
@@ -368,6 +381,38 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
 	return ret;
 }
 
+int crash_load_luks_volume_key(struct kimage *image)
+{
+	int ret;
+	struct kexec_buf kbuf = {
+		.image = image,
+		.buf_min = 0,
+		.buf_max = ULONG_MAX,
+		.top_down = false,
+		.random = true,
+	};
+
+	image->luks_volume_key_addr = 0;
+	ret = crash_pass_temp_luks_volume_key(&kbuf.buffer, &kbuf.bufsz);
+	if (ret)
+		return ret;
+
+	kbuf.memsz = kbuf.bufsz;
+	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
+	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
+	ret = kexec_add_buffer(&kbuf);
+	if (ret) {
+		vfree((void *)kbuf.buffer);
+		return ret;
+	}
+	image->luks_volume_key_addr = kbuf.mem;
+	image->luks_volume_key_sz = kbuf.bufsz;
+	pr_debug("Loaded LUKS volume key at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+		 image->luks_volume_key_addr, kbuf.bufsz, kbuf.bufsz);
+
+	return ret;
+}
+
 int crash_load_segments(struct kimage *image)
 {
 	int ret;
diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index a61c12c01270..a859e9e5c876 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -76,6 +76,10 @@ static int setup_cmdline(struct kimage *image, struct boot_params *params,
 	if (image->type == KEXEC_TYPE_CRASH) {
 		len = sprintf(cmdline_ptr,
 			"elfcorehdr=0x%lx ", image->elf_load_addr);
+
+		if (image->luks_volume_key_addr != 0)
+			len += sprintf(cmdline_ptr + len,
+					"luksvolumekey=0x%lx ", image->luks_volume_key_addr);
 	}
 	memcpy(cmdline_ptr + len, cmdline, cmdline_len);
 	cmdline_len += len;
@@ -433,6 +437,9 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
 		ret = crash_load_segments(image);
 		if (ret)
 			return ERR_PTR(ret);
+		ret = crash_load_luks_volume_key(image);
+		if (ret)
+			pr_debug("Either no LUKS volume key or error to retrieve the LUKS volume key\n");
 	}
 
 	/*
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 5b2440444112..20d213076e46 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -374,6 +374,10 @@ struct kimage {
 	void *elf_headers;
 	unsigned long elf_headers_sz;
 	unsigned long elf_load_addr;
+
+	/* LUKS volume key buffer */
+	unsigned long luks_volume_key_addr;
+	unsigned long luks_volume_key_sz;
 };
 
 /* kexec interface functions */
-- 
2.40.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[dm-devel] [PATCH 4/5] x86/crash: pass the LUKS volume key to kdump kernel

[Coiby Xu]

1st kernel will build up the kernel command parameter luksvolumekey as
similar to elfcorehdr to pass the memory address of the stored info of
LUKS volume key to kdump kernel.

Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 arch/x86/include/asm/crash.h      |  1 +
 arch/x86/kernel/crash.c           | 47 ++++++++++++++++++++++++++++++-
 arch/x86/kernel/kexec-bzimage64.c |  7 +++++
 include/linux/kexec.h             |  4 +++
 4 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/crash.h b/arch/x86/include/asm/crash.h
index 8b6bd63530dc..485f75dce2ca 100644
--- a/arch/x86/include/asm/crash.h
+++ b/arch/x86/include/asm/crash.h
@@ -4,6 +4,7 @@
 
 struct kimage;
 
+int crash_load_luks_volume_key(struct kimage *image);
 int crash_load_segments(struct kimage *image);
 int crash_setup_memmap_entries(struct kimage *image,
 		struct boot_params *params);
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index cdd92ab43cda..32d4a9e3badf 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -289,6 +289,7 @@ static int memmap_exclude_ranges(struct kimage *image, struct crash_mem *cmem,
 				 unsigned long long mend)
 {
 	unsigned long start, end;
+	int r;
 
 	cmem->ranges[0].start = mstart;
 	cmem->ranges[0].end = mend;
@@ -297,7 +298,19 @@ static int memmap_exclude_ranges(struct kimage *image, struct crash_mem *cmem,
 	/* Exclude elf header region */
 	start = image->elf_load_addr;
 	end = start + image->elf_headers_sz - 1;
-	return crash_exclude_mem_range(cmem, start, end);
+	r = crash_exclude_mem_range(cmem, start, end);
+
+	if (r)
+		return r;
+
+	/* Exclude LUKS volume key region */
+	if (image->luks_volume_key_addr) {
+		start = image->luks_volume_key_addr;
+		end = start + image->luks_volume_key_sz - 1;
+		return crash_exclude_mem_range(cmem, start, end);
+	}
+
+	return r;
 }
 
 /* Prepare memory map for crash dump kernel */
@@ -368,6 +381,38 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
 	return ret;
 }
 
+int crash_load_luks_volume_key(struct kimage *image)
+{
+	int ret;
+	struct kexec_buf kbuf = {
+		.image = image,
+		.buf_min = 0,
+		.buf_max = ULONG_MAX,
+		.top_down = false,
+		.random = true,
+	};
+
+	image->luks_volume_key_addr = 0;
+	ret = crash_pass_temp_luks_volume_key(&kbuf.buffer, &kbuf.bufsz);
+	if (ret)
+		return ret;
+
+	kbuf.memsz = kbuf.bufsz;
+	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
+	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
+	ret = kexec_add_buffer(&kbuf);
+	if (ret) {
+		vfree((void *)kbuf.buffer);
+		return ret;
+	}
+	image->luks_volume_key_addr = kbuf.mem;
+	image->luks_volume_key_sz = kbuf.bufsz;
+	pr_debug("Loaded LUKS volume key at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+		 image->luks_volume_key_addr, kbuf.bufsz, kbuf.bufsz);
+
+	return ret;
+}
+
 int crash_load_segments(struct kimage *image)
 {
 	int ret;
diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index a61c12c01270..a859e9e5c876 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -76,6 +76,10 @@ static int setup_cmdline(struct kimage *image, struct boot_params *params,
 	if (image->type == KEXEC_TYPE_CRASH) {
 		len = sprintf(cmdline_ptr,
 			"elfcorehdr=0x%lx ", image->elf_load_addr);
+
+		if (image->luks_volume_key_addr != 0)
+			len += sprintf(cmdline_ptr + len,
+					"luksvolumekey=0x%lx ", image->luks_volume_key_addr);
 	}
 	memcpy(cmdline_ptr + len, cmdline, cmdline_len);
 	cmdline_len += len;
@@ -433,6 +437,9 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
 		ret = crash_load_segments(image);
 		if (ret)
 			return ERR_PTR(ret);
+		ret = crash_load_luks_volume_key(image);
+		if (ret)
+			pr_debug("Either no LUKS volume key or error to retrieve the LUKS volume key\n");
 	}
 
 	/*
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 5b2440444112..20d213076e46 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -374,6 +374,10 @@ struct kimage {
 	void *elf_headers;
 	unsigned long elf_headers_sz;
 	unsigned long elf_load_addr;
+
+	/* LUKS volume key buffer */
+	unsigned long luks_volume_key_addr;
+	unsigned long luks_volume_key_sz;
 };
 
 /* kexec interface functions */
-- 
2.40.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 5/5] x86/crash: make the page that stores the LUKS volume key inaccessible

[Coiby Xu]

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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index 1a3e2c05a8a5..82d1ecb35827 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -546,9 +546,23 @@ 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_paddr, end_paddr;
+	unsigned int nr_pages;
+
+	if (kexec_crash_image->luks_volume_key_addr) {
+		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);
+	}
+}
+
 void arch_kexec_protect_crashkres(void)
 {
 	kexec_mark_crashkres(true);
+	kexec_mark_luks_volume_key_inaccessible();
 }
 
 void arch_kexec_unprotect_crashkres(void)
-- 
2.40.1

[PATCH 5/5] x86/crash: make the page that stores the LUKS volume key inaccessible

[Coiby Xu]

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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index 1a3e2c05a8a5..82d1ecb35827 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -546,9 +546,23 @@ 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_paddr, end_paddr;
+	unsigned int nr_pages;
+
+	if (kexec_crash_image->luks_volume_key_addr) {
+		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);
+	}
+}
+
 void arch_kexec_protect_crashkres(void)
 {
 	kexec_mark_crashkres(true);
+	kexec_mark_luks_volume_key_inaccessible();
 }
 
 void arch_kexec_unprotect_crashkres(void)
-- 
2.40.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[dm-devel] [PATCH 5/5] x86/crash: make the page that stores the LUKS volume key inaccessible

[Coiby Xu]

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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index 1a3e2c05a8a5..82d1ecb35827 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -546,9 +546,23 @@ 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_paddr, end_paddr;
+	unsigned int nr_pages;
+
+	if (kexec_crash_image->luks_volume_key_addr) {
+		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);
+	}
+}
+
 void arch_kexec_protect_crashkres(void)
 {
 	kexec_mark_crashkres(true);
+	kexec_mark_luks_volume_key_inaccessible();
 }
 
 void arch_kexec_unprotect_crashkres(void)
-- 
2.40.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Eric Biggers]

On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
> [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
happens to use.  But LUKS is a userspace concept.

This is a kernel patchset, so why does it make sense for it to be talking about
LUKS at all?  Perhaps you mean dm-crypt?

- Eric

Re: [dm-devel] [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Eric Biggers]

On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
> [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
happens to use.  But LUKS is a userspace concept.

This is a kernel patchset, so why does it make sense for it to be talking about
LUKS at all?  Perhaps you mean dm-crypt?

- Eric

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Eric Biggers]

On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
> [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
happens to use.  But LUKS is a userspace concept.

This is a kernel patchset, so why does it make sense for it to be talking about
LUKS at all?  Perhaps you mean dm-crypt?

- Eric

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [dm-devel] [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Milan Broz]

On 6/2/23 23:34, Eric Biggers wrote:
> On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>> [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
> 
> The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
> happens to use.  But LUKS is a userspace concept.
> 
> This is a kernel patchset, so why does it make sense for it to be talking about
> LUKS at all?  Perhaps you mean dm-crypt?

Exactly.

I had the same comment almost a year ago... and it still applies:
https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/

  Anyway, please fix the naming before this patchset can be read or reviewed!

  LUKS is user-space key management only (on-disk metadata); the kernel has
  no idea how the key is derived or what LUKS is - dm-crypt only knows the key
  (either through keyring or directly in the mapping table).

  Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
  is used in many other mappings (plain, bitlocker, veracrypt, ...)
  Just use the dm-crypt key, do not reference LUKS at all.

Milan

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Milan Broz]

On 6/2/23 23:34, Eric Biggers wrote:
> On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>> [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
> 
> The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
> happens to use.  But LUKS is a userspace concept.
> 
> This is a kernel patchset, so why does it make sense for it to be talking about
> LUKS at all?  Perhaps you mean dm-crypt?

Exactly.

I had the same comment almost a year ago... and it still applies:
https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/

  Anyway, please fix the naming before this patchset can be read or reviewed!

  LUKS is user-space key management only (on-disk metadata); the kernel has
  no idea how the key is derived or what LUKS is - dm-crypt only knows the key
  (either through keyring or directly in the mapping table).

  Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
  is used in many other mappings (plain, bitlocker, veracrypt, ...)
  Just use the dm-crypt key, do not reference LUKS at all.

Milan

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Milan Broz]

On 6/2/23 23:34, Eric Biggers wrote:
> On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>> [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
> 
> The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
> happens to use.  But LUKS is a userspace concept.
> 
> This is a kernel patchset, so why does it make sense for it to be talking about
> LUKS at all?  Perhaps you mean dm-crypt?

Exactly.

I had the same comment almost a year ago... and it still applies:
https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/

  Anyway, please fix the naming before this patchset can be read or reviewed!

  LUKS is user-space key management only (on-disk metadata); the kernel has
  no idea how the key is derived or what LUKS is - dm-crypt only knows the key
  (either through keyring or directly in the mapping table).

  Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
  is used in many other mappings (plain, bitlocker, veracrypt, ...)
  Just use the dm-crypt key, do not reference LUKS at all.

Milan

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Coiby Xu]

Hi Eric and Milan,

On Sat, Jun 03, 2023 at 11:22:52AM +0200, Milan Broz wrote:
>On 6/2/23 23:34, Eric Biggers wrote:
>>On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>>>[PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
>>
>>The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
>>happens to use.  But LUKS is a userspace concept.
>>
>>This is a kernel patchset, so why does it make sense for it to be talking about
>>LUKS at all?  Perhaps you mean dm-crypt?
>
>Exactly.

Thanks for raising the above concern! The use cases like CoreOS and
Confidential VMs explicitly want kdump to work for LUKS. And correct me
if I'm wrong, I think the two problems addressed by this patch set only
apply to LUKS so the kdump part of the kernel only cares about the LUKS
case. If there are use cases where similar approach is needed, I'll be
happy to make the solution more generic.

>
>I had the same comment almost a year ago... and it still applies:
>https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
>
> Anyway, please fix the naming before this patchset can be read or reviewed!
>
> LUKS is user-space key management only (on-disk metadata); the kernel has
> no idea how the key is derived or what LUKS is - dm-crypt only knows the key
> (either through keyring or directly in the mapping table).
>
> Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
> is used in many other mappings (plain, bitlocker, veracrypt, ...)
> Just use the dm-crypt key, do not reference LUKS at all.

Thanks for the reminding! That comment was on the first RFC version. But
starting with "RFC v2", there is no longer any interaction with dm-crypt
(to save a copy of the LUKS volume key for the kdump kernel) and now I
make cryptsetup talks to the kdump part of the kernel via the sysfs to
reuse the volume key. So only the kdump part of the kernel needs to know
LUKS which is what it cares. Thus I don't think there is any kernel
namespace pollution now.

>
>Milan
>

-- 
Best regards,
Coiby


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Coiby Xu]

Hi Eric and Milan,

On Sat, Jun 03, 2023 at 11:22:52AM +0200, Milan Broz wrote:
>On 6/2/23 23:34, Eric Biggers wrote:
>>On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>>>[PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
>>
>>The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
>>happens to use.  But LUKS is a userspace concept.
>>
>>This is a kernel patchset, so why does it make sense for it to be talking about
>>LUKS at all?  Perhaps you mean dm-crypt?
>
>Exactly.

Thanks for raising the above concern! The use cases like CoreOS and
Confidential VMs explicitly want kdump to work for LUKS. And correct me
if I'm wrong, I think the two problems addressed by this patch set only
apply to LUKS so the kdump part of the kernel only cares about the LUKS
case. If there are use cases where similar approach is needed, I'll be
happy to make the solution more generic.

>
>I had the same comment almost a year ago... and it still applies:
>https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
>
> Anyway, please fix the naming before this patchset can be read or reviewed!
>
> LUKS is user-space key management only (on-disk metadata); the kernel has
> no idea how the key is derived or what LUKS is - dm-crypt only knows the key
> (either through keyring or directly in the mapping table).
>
> Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
> is used in many other mappings (plain, bitlocker, veracrypt, ...)
> Just use the dm-crypt key, do not reference LUKS at all.

Thanks for the reminding! That comment was on the first RFC version. But
starting with "RFC v2", there is no longer any interaction with dm-crypt
(to save a copy of the LUKS volume key for the kdump kernel) and now I
make cryptsetup talks to the kdump part of the kernel via the sysfs to
reuse the volume key. So only the kdump part of the kernel needs to know
LUKS which is what it cares. Thus I don't think there is any kernel
namespace pollution now.

>
>Milan
>

-- 
Best regards,
Coiby

Re: [dm-devel] [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Coiby Xu]

Hi Eric and Milan,

On Sat, Jun 03, 2023 at 11:22:52AM +0200, Milan Broz wrote:
>On 6/2/23 23:34, Eric Biggers wrote:
>>On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>>>[PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
>>
>>The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
>>happens to use.  But LUKS is a userspace concept.
>>
>>This is a kernel patchset, so why does it make sense for it to be talking about
>>LUKS at all?  Perhaps you mean dm-crypt?
>
>Exactly.

Thanks for raising the above concern! The use cases like CoreOS and
Confidential VMs explicitly want kdump to work for LUKS. And correct me
if I'm wrong, I think the two problems addressed by this patch set only
apply to LUKS so the kdump part of the kernel only cares about the LUKS
case. If there are use cases where similar approach is needed, I'll be
happy to make the solution more generic.

>
>I had the same comment almost a year ago... and it still applies:
>https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
>
> Anyway, please fix the naming before this patchset can be read or reviewed!
>
> LUKS is user-space key management only (on-disk metadata); the kernel has
> no idea how the key is derived or what LUKS is - dm-crypt only knows the key
> (either through keyring or directly in the mapping table).
>
> Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
> is used in many other mappings (plain, bitlocker, veracrypt, ...)
> Just use the dm-crypt key, do not reference LUKS at all.

Thanks for the reminding! That comment was on the first RFC version. But
starting with "RFC v2", there is no longer any interaction with dm-crypt
(to save a copy of the LUKS volume key for the kdump kernel) and now I
make cryptsetup talks to the kdump part of the kernel via the sysfs to
reuse the volume key. So only the kdump part of the kernel needs to know
LUKS which is what it cares. Thus I don't think there is any kernel
namespace pollution now.

>
>Milan
>

-- 
Best regards,
Coiby

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Milan Broz]

On 6/5/23 04:31, Coiby Xu wrote:
> Hi Eric and Milan,
> 
> On Sat, Jun 03, 2023 at 11:22:52AM +0200, Milan Broz wrote:
>> On 6/2/23 23:34, Eric Biggers wrote:
>>> On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>>>> [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
>>>
>>> The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
>>> happens to use.  But LUKS is a userspace concept.
>>>
>>> This is a kernel patchset, so why does it make sense for it to be talking about
>>> LUKS at all?  Perhaps you mean dm-crypt?
>>
>> Exactly.
> 
> Thanks for raising the above concern! The use cases like CoreOS and
> Confidential VMs explicitly want kdump to work for LUKS. And correct me
> if I'm wrong, I think the two problems addressed by this patch set only
> apply to LUKS so the kdump part of the kernel only cares about the LUKS
> case. If there are use cases where similar approach is needed, I'll be
> happy to make the solution more generic.
> 
>>
>> I had the same comment almost a year ago... and it still applies:
>> https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
>>
>> Anyway, please fix the naming before this patchset can be read or reviewed!
>>
>> LUKS is user-space key management only (on-disk metadata); the kernel has
>> no idea how the key is derived or what LUKS is - dm-crypt only knows the key
>> (either through keyring or directly in the mapping table).
>>
>> Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
>> is used in many other mappings (plain, bitlocker, veracrypt, ...)
>> Just use the dm-crypt key, do not reference LUKS at all.
> 
> Thanks for the reminding! That comment was on the first RFC version. But
> starting with "RFC v2", there is no longer any interaction with dm-crypt
> (to save a copy of the LUKS volume key for the kdump kernel) and now I
> make cryptsetup talks to the kdump part of the kernel via the sysfs to
> reuse the volume key. So only the kdump part of the kernel needs to know
> LUKS which is what it cares. Thus I don't think there is any kernel
> namespace pollution now.

Hi,

I am sorry if I did understand correctly, but I thought that kdump is part
of the kernel.

I am trying to say that kernel generally has no concept of LUKS;
this is a userspace abstraction for key management.

Even the cryptsetup dm-crypt configuration mapping table generated from LUKS
has nothing LUKS special in it (only in DM-UUID as a name prefix).

So I do not understand why you need to mention LUKS even in kdump part.
Perhaps it is still only a naming problem, nothing more.

All you need is to preserve key and configuration parameters (for dm-crypt).
If it is set by cryptsetup, dmsetup, or any other way is not important - on this
kernel layer, it has nothing to do with LUKS key management metadata.

No problem if you support only LUKS in userspace, but really, all this machinery
should work for any dm-crypt devices. Perhaps your patch even works for it already.

Milan

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [dm-devel] [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Milan Broz]

On 6/5/23 04:31, Coiby Xu wrote:
> Hi Eric and Milan,
> 
> On Sat, Jun 03, 2023 at 11:22:52AM +0200, Milan Broz wrote:
>> On 6/2/23 23:34, Eric Biggers wrote:
>>> On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>>>> [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
>>>
>>> The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
>>> happens to use.  But LUKS is a userspace concept.
>>>
>>> This is a kernel patchset, so why does it make sense for it to be talking about
>>> LUKS at all?  Perhaps you mean dm-crypt?
>>
>> Exactly.
> 
> Thanks for raising the above concern! The use cases like CoreOS and
> Confidential VMs explicitly want kdump to work for LUKS. And correct me
> if I'm wrong, I think the two problems addressed by this patch set only
> apply to LUKS so the kdump part of the kernel only cares about the LUKS
> case. If there are use cases where similar approach is needed, I'll be
> happy to make the solution more generic.
> 
>>
>> I had the same comment almost a year ago... and it still applies:
>> https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
>>
>> Anyway, please fix the naming before this patchset can be read or reviewed!
>>
>> LUKS is user-space key management only (on-disk metadata); the kernel has
>> no idea how the key is derived or what LUKS is - dm-crypt only knows the key
>> (either through keyring or directly in the mapping table).
>>
>> Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
>> is used in many other mappings (plain, bitlocker, veracrypt, ...)
>> Just use the dm-crypt key, do not reference LUKS at all.
> 
> Thanks for the reminding! That comment was on the first RFC version. But
> starting with "RFC v2", there is no longer any interaction with dm-crypt
> (to save a copy of the LUKS volume key for the kdump kernel) and now I
> make cryptsetup talks to the kdump part of the kernel via the sysfs to
> reuse the volume key. So only the kdump part of the kernel needs to know
> LUKS which is what it cares. Thus I don't think there is any kernel
> namespace pollution now.

Hi,

I am sorry if I did understand correctly, but I thought that kdump is part
of the kernel.

I am trying to say that kernel generally has no concept of LUKS;
this is a userspace abstraction for key management.

Even the cryptsetup dm-crypt configuration mapping table generated from LUKS
has nothing LUKS special in it (only in DM-UUID as a name prefix).

So I do not understand why you need to mention LUKS even in kdump part.
Perhaps it is still only a naming problem, nothing more.

All you need is to preserve key and configuration parameters (for dm-crypt).
If it is set by cryptsetup, dmsetup, or any other way is not important - on this
kernel layer, it has nothing to do with LUKS key management metadata.

No problem if you support only LUKS in userspace, but really, all this machinery
should work for any dm-crypt devices. Perhaps your patch even works for it already.

Milan

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Milan Broz]

On 6/5/23 04:31, Coiby Xu wrote:
> Hi Eric and Milan,
> 
> On Sat, Jun 03, 2023 at 11:22:52AM +0200, Milan Broz wrote:
>> On 6/2/23 23:34, Eric Biggers wrote:
>>> On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>>>> [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
>>>
>>> The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
>>> happens to use.  But LUKS is a userspace concept.
>>>
>>> This is a kernel patchset, so why does it make sense for it to be talking about
>>> LUKS at all?  Perhaps you mean dm-crypt?
>>
>> Exactly.
> 
> Thanks for raising the above concern! The use cases like CoreOS and
> Confidential VMs explicitly want kdump to work for LUKS. And correct me
> if I'm wrong, I think the two problems addressed by this patch set only
> apply to LUKS so the kdump part of the kernel only cares about the LUKS
> case. If there are use cases where similar approach is needed, I'll be
> happy to make the solution more generic.
> 
>>
>> I had the same comment almost a year ago... and it still applies:
>> https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
>>
>> Anyway, please fix the naming before this patchset can be read or reviewed!
>>
>> LUKS is user-space key management only (on-disk metadata); the kernel has
>> no idea how the key is derived or what LUKS is - dm-crypt only knows the key
>> (either through keyring or directly in the mapping table).
>>
>> Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
>> is used in many other mappings (plain, bitlocker, veracrypt, ...)
>> Just use the dm-crypt key, do not reference LUKS at all.
> 
> Thanks for the reminding! That comment was on the first RFC version. But
> starting with "RFC v2", there is no longer any interaction with dm-crypt
> (to save a copy of the LUKS volume key for the kdump kernel) and now I
> make cryptsetup talks to the kdump part of the kernel via the sysfs to
> reuse the volume key. So only the kdump part of the kernel needs to know
> LUKS which is what it cares. Thus I don't think there is any kernel
> namespace pollution now.

Hi,

I am sorry if I did understand correctly, but I thought that kdump is part
of the kernel.

I am trying to say that kernel generally has no concept of LUKS;
this is a userspace abstraction for key management.

Even the cryptsetup dm-crypt configuration mapping table generated from LUKS
has nothing LUKS special in it (only in DM-UUID as a name prefix).

So I do not understand why you need to mention LUKS even in kdump part.
Perhaps it is still only a naming problem, nothing more.

All you need is to preserve key and configuration parameters (for dm-crypt).
If it is set by cryptsetup, dmsetup, or any other way is not important - on this
kernel layer, it has nothing to do with LUKS key management metadata.

No problem if you support only LUKS in userspace, but really, all this machinery
should work for any dm-crypt devices. Perhaps your patch even works for it already.

Milan

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Coiby Xu]

On Mon, Jun 05, 2023 at 09:09:49AM +0200, Milan Broz wrote:
>On 6/5/23 04:31, Coiby Xu wrote:
>>Hi Eric and Milan,
>>
>>On Sat, Jun 03, 2023 at 11:22:52AM +0200, Milan Broz wrote:
>>>On 6/2/23 23:34, Eric Biggers wrote:
>>>>On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>>>>>[PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
>>>>
>>>>The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
>>>>happens to use.  But LUKS is a userspace concept.
>>>>
>>>>This is a kernel patchset, so why does it make sense for it to be talking about
>>>>LUKS at all?  Perhaps you mean dm-crypt?
>>>
>>>Exactly.
>>
>>Thanks for raising the above concern! The use cases like CoreOS and
>>Confidential VMs explicitly want kdump to work for LUKS. And correct me
>>if I'm wrong, I think the two problems addressed by this patch set only
>>apply to LUKS so the kdump part of the kernel only cares about the LUKS
>>case. If there are use cases where similar approach is needed, I'll be
>>happy to make the solution more generic.
>>
>>>
>>>I had the same comment almost a year ago... and it still applies:
>>>https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
>>>
>>>Anyway, please fix the naming before this patchset can be read or reviewed!
>>>
>>>LUKS is user-space key management only (on-disk metadata); the kernel has
>>>no idea how the key is derived or what LUKS is - dm-crypt only knows the key
>>>(either through keyring or directly in the mapping table).
>>>
>>>Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
>>>is used in many other mappings (plain, bitlocker, veracrypt, ...)
>>>Just use the dm-crypt key, do not reference LUKS at all.
>>
>>Thanks for the reminding! That comment was on the first RFC version. But
>>starting with "RFC v2", there is no longer any interaction with dm-crypt
>>(to save a copy of the LUKS volume key for the kdump kernel) and now I
>>make cryptsetup talks to the kdump part of the kernel via the sysfs to
>>reuse the volume key. So only the kdump part of the kernel needs to know
>>LUKS which is what it cares. Thus I don't think there is any kernel
>>namespace pollution now.
>
>Hi,
>
>I am sorry if I did understand correctly, but I thought that kdump is part
>of the kernel.

Yes, there is the kernel part of the kdump although there is also the
userspace part to make the feature complete:)

>
>I am trying to say that kernel generally has no concept of LUKS;
>this is a userspace abstraction for key management.
>
>Even the cryptsetup dm-crypt configuration mapping table generated from LUKS
>has nothing LUKS special in it (only in DM-UUID as a name prefix).
>
>So I do not understand why you need to mention LUKS even in kdump part.
>Perhaps it is still only a naming problem, nothing more.
>
>All you need is to preserve key and configuration parameters (for dm-crypt).
>If it is set by cryptsetup, dmsetup, or any other way is not important - on this
>kernel layer, it has nothing to do with LUKS key management metadata.
>
>No problem if you support only LUKS in userspace, but really, all this machinery
>should work for any dm-crypt devices. Perhaps your patch even works for it already.

Thanks for the explanation! After reflecting on your words for some
time, I realize I had an implicit assumption. I assumed is if I use a
name like dm_crypt_key instead of luks_volume_key, I need to support all
mappings like plain, bitlocker, veracrypt as mentioned by you and this
could mean much more efforts. So I'm not motivated to do that as
currently users only request kdump to work for LUKS.  

But maybe I can divide the efforts into the kernel part and userspace
part. For the kernel part, almost no effort is needed since only
renaming is needed as pointed out by you. For the userpace part, maybe
it's OK to support preserving key only for LUKS2 in cryptsetup as hinted
by your last paragraph? Does it look good to you from the viewpoint of
the maintainer of cryptsetup?

>
>Milan
>

-- 
Best regards,
Coiby


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Coiby Xu]

On Mon, Jun 05, 2023 at 09:09:49AM +0200, Milan Broz wrote:
>On 6/5/23 04:31, Coiby Xu wrote:
>>Hi Eric and Milan,
>>
>>On Sat, Jun 03, 2023 at 11:22:52AM +0200, Milan Broz wrote:
>>>On 6/2/23 23:34, Eric Biggers wrote:
>>>>On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>>>>>[PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
>>>>
>>>>The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
>>>>happens to use.  But LUKS is a userspace concept.
>>>>
>>>>This is a kernel patchset, so why does it make sense for it to be talking about
>>>>LUKS at all?  Perhaps you mean dm-crypt?
>>>
>>>Exactly.
>>
>>Thanks for raising the above concern! The use cases like CoreOS and
>>Confidential VMs explicitly want kdump to work for LUKS. And correct me
>>if I'm wrong, I think the two problems addressed by this patch set only
>>apply to LUKS so the kdump part of the kernel only cares about the LUKS
>>case. If there are use cases where similar approach is needed, I'll be
>>happy to make the solution more generic.
>>
>>>
>>>I had the same comment almost a year ago... and it still applies:
>>>https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
>>>
>>>Anyway, please fix the naming before this patchset can be read or reviewed!
>>>
>>>LUKS is user-space key management only (on-disk metadata); the kernel has
>>>no idea how the key is derived or what LUKS is - dm-crypt only knows the key
>>>(either through keyring or directly in the mapping table).
>>>
>>>Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
>>>is used in many other mappings (plain, bitlocker, veracrypt, ...)
>>>Just use the dm-crypt key, do not reference LUKS at all.
>>
>>Thanks for the reminding! That comment was on the first RFC version. But
>>starting with "RFC v2", there is no longer any interaction with dm-crypt
>>(to save a copy of the LUKS volume key for the kdump kernel) and now I
>>make cryptsetup talks to the kdump part of the kernel via the sysfs to
>>reuse the volume key. So only the kdump part of the kernel needs to know
>>LUKS which is what it cares. Thus I don't think there is any kernel
>>namespace pollution now.
>
>Hi,
>
>I am sorry if I did understand correctly, but I thought that kdump is part
>of the kernel.

Yes, there is the kernel part of the kdump although there is also the
userspace part to make the feature complete:)

>
>I am trying to say that kernel generally has no concept of LUKS;
>this is a userspace abstraction for key management.
>
>Even the cryptsetup dm-crypt configuration mapping table generated from LUKS
>has nothing LUKS special in it (only in DM-UUID as a name prefix).
>
>So I do not understand why you need to mention LUKS even in kdump part.
>Perhaps it is still only a naming problem, nothing more.
>
>All you need is to preserve key and configuration parameters (for dm-crypt).
>If it is set by cryptsetup, dmsetup, or any other way is not important - on this
>kernel layer, it has nothing to do with LUKS key management metadata.
>
>No problem if you support only LUKS in userspace, but really, all this machinery
>should work for any dm-crypt devices. Perhaps your patch even works for it already.

Thanks for the explanation! After reflecting on your words for some
time, I realize I had an implicit assumption. I assumed is if I use a
name like dm_crypt_key instead of luks_volume_key, I need to support all
mappings like plain, bitlocker, veracrypt as mentioned by you and this
could mean much more efforts. So I'm not motivated to do that as
currently users only request kdump to work for LUKS.  

But maybe I can divide the efforts into the kernel part and userspace
part. For the kernel part, almost no effort is needed since only
renaming is needed as pointed out by you. For the userpace part, maybe
it's OK to support preserving key only for LUKS2 in cryptsetup as hinted
by your last paragraph? Does it look good to you from the viewpoint of
the maintainer of cryptsetup?

>
>Milan
>

-- 
Best regards,
Coiby

Re: [dm-devel] [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Coiby Xu]

On Mon, Jun 05, 2023 at 09:09:49AM +0200, Milan Broz wrote:
>On 6/5/23 04:31, Coiby Xu wrote:
>>Hi Eric and Milan,
>>
>>On Sat, Jun 03, 2023 at 11:22:52AM +0200, Milan Broz wrote:
>>>On 6/2/23 23:34, Eric Biggers wrote:
>>>>On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>>>>>[PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
>>>>
>>>>The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
>>>>happens to use.  But LUKS is a userspace concept.
>>>>
>>>>This is a kernel patchset, so why does it make sense for it to be talking about
>>>>LUKS at all?  Perhaps you mean dm-crypt?
>>>
>>>Exactly.
>>
>>Thanks for raising the above concern! The use cases like CoreOS and
>>Confidential VMs explicitly want kdump to work for LUKS. And correct me
>>if I'm wrong, I think the two problems addressed by this patch set only
>>apply to LUKS so the kdump part of the kernel only cares about the LUKS
>>case. If there are use cases where similar approach is needed, I'll be
>>happy to make the solution more generic.
>>
>>>
>>>I had the same comment almost a year ago... and it still applies:
>>>https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
>>>
>>>Anyway, please fix the naming before this patchset can be read or reviewed!
>>>
>>>LUKS is user-space key management only (on-disk metadata); the kernel has
>>>no idea how the key is derived or what LUKS is - dm-crypt only knows the key
>>>(either through keyring or directly in the mapping table).
>>>
>>>Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
>>>is used in many other mappings (plain, bitlocker, veracrypt, ...)
>>>Just use the dm-crypt key, do not reference LUKS at all.
>>
>>Thanks for the reminding! That comment was on the first RFC version. But
>>starting with "RFC v2", there is no longer any interaction with dm-crypt
>>(to save a copy of the LUKS volume key for the kdump kernel) and now I
>>make cryptsetup talks to the kdump part of the kernel via the sysfs to
>>reuse the volume key. So only the kdump part of the kernel needs to know
>>LUKS which is what it cares. Thus I don't think there is any kernel
>>namespace pollution now.
>
>Hi,
>
>I am sorry if I did understand correctly, but I thought that kdump is part
>of the kernel.

Yes, there is the kernel part of the kdump although there is also the
userspace part to make the feature complete:)

>
>I am trying to say that kernel generally has no concept of LUKS;
>this is a userspace abstraction for key management.
>
>Even the cryptsetup dm-crypt configuration mapping table generated from LUKS
>has nothing LUKS special in it (only in DM-UUID as a name prefix).
>
>So I do not understand why you need to mention LUKS even in kdump part.
>Perhaps it is still only a naming problem, nothing more.
>
>All you need is to preserve key and configuration parameters (for dm-crypt).
>If it is set by cryptsetup, dmsetup, or any other way is not important - on this
>kernel layer, it has nothing to do with LUKS key management metadata.
>
>No problem if you support only LUKS in userspace, but really, all this machinery
>should work for any dm-crypt devices. Perhaps your patch even works for it already.

Thanks for the explanation! After reflecting on your words for some
time, I realize I had an implicit assumption. I assumed is if I use a
name like dm_crypt_key instead of luks_volume_key, I need to support all
mappings like plain, bitlocker, veracrypt as mentioned by you and this
could mean much more efforts. So I'm not motivated to do that as
currently users only request kdump to work for LUKS.  

But maybe I can divide the efforts into the kernel part and userspace
part. For the kernel part, almost no effort is needed since only
renaming is needed as pointed out by you. For the userpace part, maybe
it's OK to support preserving key only for LUKS2 in cryptsetup as hinted
by your last paragraph? Does it look good to you from the viewpoint of
the maintainer of cryptsetup?

>
>Milan
>

-- 
Best regards,
Coiby

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Milan Broz]

On 6/6/23 13:02, Coiby Xu wrote:
> On Mon, Jun 05, 2023 at 09:09:49AM +0200, Milan Broz wrote:
>> On 6/5/23 04:31, Coiby Xu wrote:
>>> Hi Eric and Milan,
>>>
>>> On Sat, Jun 03, 2023 at 11:22:52AM +0200, Milan Broz wrote:
>>>> On 6/2/23 23:34, Eric Biggers wrote:
>>>>> On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>>>>>> [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
>>>>>
>>>>> The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
>>>>> happens to use.  But LUKS is a userspace concept.
>>>>>
>>>>> This is a kernel patchset, so why does it make sense for it to be talking about
>>>>> LUKS at all?  Perhaps you mean dm-crypt?
>>>>
>>>> Exactly.
>>>
>>> Thanks for raising the above concern! The use cases like CoreOS and
>>> Confidential VMs explicitly want kdump to work for LUKS. And correct me
>>> if I'm wrong, I think the two problems addressed by this patch set only
>>> apply to LUKS so the kdump part of the kernel only cares about the LUKS
>>> case. If there are use cases where similar approach is needed, I'll be
>>> happy to make the solution more generic.
>>>
>>>>
>>>> I had the same comment almost a year ago... and it still applies:
>>>> https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
>>>>
>>>> Anyway, please fix the naming before this patchset can be read or reviewed!
>>>>
>>>> LUKS is user-space key management only (on-disk metadata); the kernel has
>>>> no idea how the key is derived or what LUKS is - dm-crypt only knows the key
>>>> (either through keyring or directly in the mapping table).
>>>>
>>>> Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
>>>> is used in many other mappings (plain, bitlocker, veracrypt, ...)
>>>> Just use the dm-crypt key, do not reference LUKS at all.
>>>
>>> Thanks for the reminding! That comment was on the first RFC version. But
>>> starting with "RFC v2", there is no longer any interaction with dm-crypt
>>> (to save a copy of the LUKS volume key for the kdump kernel) and now I
>>> make cryptsetup talks to the kdump part of the kernel via the sysfs to
>>> reuse the volume key. So only the kdump part of the kernel needs to know
>>> LUKS which is what it cares. Thus I don't think there is any kernel
>>> namespace pollution now.
>>
>> Hi,
>>
>> I am sorry if I did understand correctly, but I thought that kdump is part
>> of the kernel.
> 
> Yes, there is the kernel part of the kdump although there is also the
> userspace part to make the feature complete:)
> 
>>
>> I am trying to say that kernel generally has no concept of LUKS;
>> this is a userspace abstraction for key management.
>>
>> Even the cryptsetup dm-crypt configuration mapping table generated from LUKS
>> has nothing LUKS special in it (only in DM-UUID as a name prefix).
>>
>> So I do not understand why you need to mention LUKS even in kdump part.
>> Perhaps it is still only a naming problem, nothing more.
>>
>> All you need is to preserve key and configuration parameters (for dm-crypt).
>> If it is set by cryptsetup, dmsetup, or any other way is not important - on this
>> kernel layer, it has nothing to do with LUKS key management metadata.
>>
>> No problem if you support only LUKS in userspace, but really, all this machinery
>> should work for any dm-crypt devices. Perhaps your patch even works for it already.
> 
> Thanks for the explanation! After reflecting on your words for some
> time, I realize I had an implicit assumption. I assumed is if I use a
> name like dm_crypt_key instead of luks_volume_key, I need to support all
> mappings like plain, bitlocker, veracrypt as mentioned by you and this
> could mean much more efforts. So I'm not motivated to do that as
> currently users only request kdump to work for LUKS.

Thanks, I think it is perfectly fine to implement just subset here.

> But maybe I can divide the efforts into the kernel part and userspace
> part. For the kernel part, almost no effort is needed since only
> renaming is needed as pointed out by you. For the userpace part, maybe
> it's OK to support preserving key only for LUKS2 in cryptsetup as hinted
> by your last paragraph? Does it look good to you from the viewpoint of
> the maintainer of cryptsetup?

My comment was just about proper naming in kernel, it is of course up to you
what you want to support in userspace (and even in kernel, extensions can
be added later).

Only LUKS2 uses keyring for volume key in dm-crypt as default option anyway.
I do not think you need any cryptsetup patches, all you need is to write
decrypted volume key from LUKS metadata with
   cryptsetup luksDump ---dump-volume-key -volume-key-file <out> <device>
(or any code equivalent with libcryptsetup), am I correct?

Milan

Re: [dm-devel] [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Milan Broz]

On 6/6/23 13:02, Coiby Xu wrote:
> On Mon, Jun 05, 2023 at 09:09:49AM +0200, Milan Broz wrote:
>> On 6/5/23 04:31, Coiby Xu wrote:
>>> Hi Eric and Milan,
>>>
>>> On Sat, Jun 03, 2023 at 11:22:52AM +0200, Milan Broz wrote:
>>>> On 6/2/23 23:34, Eric Biggers wrote:
>>>>> On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>>>>>> [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
>>>>>
>>>>> The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
>>>>> happens to use.  But LUKS is a userspace concept.
>>>>>
>>>>> This is a kernel patchset, so why does it make sense for it to be talking about
>>>>> LUKS at all?  Perhaps you mean dm-crypt?
>>>>
>>>> Exactly.
>>>
>>> Thanks for raising the above concern! The use cases like CoreOS and
>>> Confidential VMs explicitly want kdump to work for LUKS. And correct me
>>> if I'm wrong, I think the two problems addressed by this patch set only
>>> apply to LUKS so the kdump part of the kernel only cares about the LUKS
>>> case. If there are use cases where similar approach is needed, I'll be
>>> happy to make the solution more generic.
>>>
>>>>
>>>> I had the same comment almost a year ago... and it still applies:
>>>> https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
>>>>
>>>> Anyway, please fix the naming before this patchset can be read or reviewed!
>>>>
>>>> LUKS is user-space key management only (on-disk metadata); the kernel has
>>>> no idea how the key is derived or what LUKS is - dm-crypt only knows the key
>>>> (either through keyring or directly in the mapping table).
>>>>
>>>> Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
>>>> is used in many other mappings (plain, bitlocker, veracrypt, ...)
>>>> Just use the dm-crypt key, do not reference LUKS at all.
>>>
>>> Thanks for the reminding! That comment was on the first RFC version. But
>>> starting with "RFC v2", there is no longer any interaction with dm-crypt
>>> (to save a copy of the LUKS volume key for the kdump kernel) and now I
>>> make cryptsetup talks to the kdump part of the kernel via the sysfs to
>>> reuse the volume key. So only the kdump part of the kernel needs to know
>>> LUKS which is what it cares. Thus I don't think there is any kernel
>>> namespace pollution now.
>>
>> Hi,
>>
>> I am sorry if I did understand correctly, but I thought that kdump is part
>> of the kernel.
> 
> Yes, there is the kernel part of the kdump although there is also the
> userspace part to make the feature complete:)
> 
>>
>> I am trying to say that kernel generally has no concept of LUKS;
>> this is a userspace abstraction for key management.
>>
>> Even the cryptsetup dm-crypt configuration mapping table generated from LUKS
>> has nothing LUKS special in it (only in DM-UUID as a name prefix).
>>
>> So I do not understand why you need to mention LUKS even in kdump part.
>> Perhaps it is still only a naming problem, nothing more.
>>
>> All you need is to preserve key and configuration parameters (for dm-crypt).
>> If it is set by cryptsetup, dmsetup, or any other way is not important - on this
>> kernel layer, it has nothing to do with LUKS key management metadata.
>>
>> No problem if you support only LUKS in userspace, but really, all this machinery
>> should work for any dm-crypt devices. Perhaps your patch even works for it already.
> 
> Thanks for the explanation! After reflecting on your words for some
> time, I realize I had an implicit assumption. I assumed is if I use a
> name like dm_crypt_key instead of luks_volume_key, I need to support all
> mappings like plain, bitlocker, veracrypt as mentioned by you and this
> could mean much more efforts. So I'm not motivated to do that as
> currently users only request kdump to work for LUKS.

Thanks, I think it is perfectly fine to implement just subset here.

> But maybe I can divide the efforts into the kernel part and userspace
> part. For the kernel part, almost no effort is needed since only
> renaming is needed as pointed out by you. For the userpace part, maybe
> it's OK to support preserving key only for LUKS2 in cryptsetup as hinted
> by your last paragraph? Does it look good to you from the viewpoint of
> the maintainer of cryptsetup?

My comment was just about proper naming in kernel, it is of course up to you
what you want to support in userspace (and even in kernel, extensions can
be added later).

Only LUKS2 uses keyring for volume key in dm-crypt as default option anyway.
I do not think you need any cryptsetup patches, all you need is to write
decrypted volume key from LUKS metadata with
   cryptsetup luksDump ---dump-volume-key -volume-key-file <out> <device>
(or any code equivalent with libcryptsetup), am I correct?

Milan

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Milan Broz]

On 6/6/23 13:02, Coiby Xu wrote:
> On Mon, Jun 05, 2023 at 09:09:49AM +0200, Milan Broz wrote:
>> On 6/5/23 04:31, Coiby Xu wrote:
>>> Hi Eric and Milan,
>>>
>>> On Sat, Jun 03, 2023 at 11:22:52AM +0200, Milan Broz wrote:
>>>> On 6/2/23 23:34, Eric Biggers wrote:
>>>>> On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>>>>>> [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
>>>>>
>>>>> The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
>>>>> happens to use.  But LUKS is a userspace concept.
>>>>>
>>>>> This is a kernel patchset, so why does it make sense for it to be talking about
>>>>> LUKS at all?  Perhaps you mean dm-crypt?
>>>>
>>>> Exactly.
>>>
>>> Thanks for raising the above concern! The use cases like CoreOS and
>>> Confidential VMs explicitly want kdump to work for LUKS. And correct me
>>> if I'm wrong, I think the two problems addressed by this patch set only
>>> apply to LUKS so the kdump part of the kernel only cares about the LUKS
>>> case. If there are use cases where similar approach is needed, I'll be
>>> happy to make the solution more generic.
>>>
>>>>
>>>> I had the same comment almost a year ago... and it still applies:
>>>> https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
>>>>
>>>> Anyway, please fix the naming before this patchset can be read or reviewed!
>>>>
>>>> LUKS is user-space key management only (on-disk metadata); the kernel has
>>>> no idea how the key is derived or what LUKS is - dm-crypt only knows the key
>>>> (either through keyring or directly in the mapping table).
>>>>
>>>> Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
>>>> is used in many other mappings (plain, bitlocker, veracrypt, ...)
>>>> Just use the dm-crypt key, do not reference LUKS at all.
>>>
>>> Thanks for the reminding! That comment was on the first RFC version. But
>>> starting with "RFC v2", there is no longer any interaction with dm-crypt
>>> (to save a copy of the LUKS volume key for the kdump kernel) and now I
>>> make cryptsetup talks to the kdump part of the kernel via the sysfs to
>>> reuse the volume key. So only the kdump part of the kernel needs to know
>>> LUKS which is what it cares. Thus I don't think there is any kernel
>>> namespace pollution now.
>>
>> Hi,
>>
>> I am sorry if I did understand correctly, but I thought that kdump is part
>> of the kernel.
> 
> Yes, there is the kernel part of the kdump although there is also the
> userspace part to make the feature complete:)
> 
>>
>> I am trying to say that kernel generally has no concept of LUKS;
>> this is a userspace abstraction for key management.
>>
>> Even the cryptsetup dm-crypt configuration mapping table generated from LUKS
>> has nothing LUKS special in it (only in DM-UUID as a name prefix).
>>
>> So I do not understand why you need to mention LUKS even in kdump part.
>> Perhaps it is still only a naming problem, nothing more.
>>
>> All you need is to preserve key and configuration parameters (for dm-crypt).
>> If it is set by cryptsetup, dmsetup, or any other way is not important - on this
>> kernel layer, it has nothing to do with LUKS key management metadata.
>>
>> No problem if you support only LUKS in userspace, but really, all this machinery
>> should work for any dm-crypt devices. Perhaps your patch even works for it already.
> 
> Thanks for the explanation! After reflecting on your words for some
> time, I realize I had an implicit assumption. I assumed is if I use a
> name like dm_crypt_key instead of luks_volume_key, I need to support all
> mappings like plain, bitlocker, veracrypt as mentioned by you and this
> could mean much more efforts. So I'm not motivated to do that as
> currently users only request kdump to work for LUKS.

Thanks, I think it is perfectly fine to implement just subset here.

> But maybe I can divide the efforts into the kernel part and userspace
> part. For the kernel part, almost no effort is needed since only
> renaming is needed as pointed out by you. For the userpace part, maybe
> it's OK to support preserving key only for LUKS2 in cryptsetup as hinted
> by your last paragraph? Does it look good to you from the viewpoint of
> the maintainer of cryptsetup?

My comment was just about proper naming in kernel, it is of course up to you
what you want to support in userspace (and even in kernel, extensions can
be added later).

Only LUKS2 uses keyring for volume key in dm-crypt as default option anyway.
I do not think you need any cryptsetup patches, all you need is to write
decrypted volume key from LUKS metadata with
   cryptsetup luksDump ---dump-volume-key -volume-key-file <out> <device>
(or any code equivalent with libcryptsetup), am I correct?

Milan

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Coiby Xu]

On Wed, Jun 07, 2023 at 08:14:44AM +0200, Milan Broz wrote:
>On 6/6/23 13:02, Coiby Xu wrote:
>>On Mon, Jun 05, 2023 at 09:09:49AM +0200, Milan Broz wrote:
>>>On 6/5/23 04:31, Coiby Xu wrote:
>>>>Hi Eric and Milan,
>>>>
>>>>On Sat, Jun 03, 2023 at 11:22:52AM +0200, Milan Broz wrote:
>>>>>On 6/2/23 23:34, Eric Biggers wrote:
>>>>>>On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>>>>>>>[PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
>>>>>>
>>>>>>The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
>>>>>>happens to use.  But LUKS is a userspace concept.
>>>>>>
>>>>>>This is a kernel patchset, so why does it make sense for it to be talking about
>>>>>>LUKS at all?  Perhaps you mean dm-crypt?
>>>>>
>>>>>Exactly.
>>>>
>>>>Thanks for raising the above concern! The use cases like CoreOS and
>>>>Confidential VMs explicitly want kdump to work for LUKS. And correct me
>>>>if I'm wrong, I think the two problems addressed by this patch set only
>>>>apply to LUKS so the kdump part of the kernel only cares about the LUKS
>>>>case. If there are use cases where similar approach is needed, I'll be
>>>>happy to make the solution more generic.
>>>>
>>>>>
>>>>>I had the same comment almost a year ago... and it still applies:
>>>>>https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
>>>>>
>>>>>Anyway, please fix the naming before this patchset can be read or reviewed!
>>>>>
>>>>>LUKS is user-space key management only (on-disk metadata); the kernel has
>>>>>no idea how the key is derived or what LUKS is - dm-crypt only knows the key
>>>>>(either through keyring or directly in the mapping table).
>>>>>
>>>>>Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
>>>>>is used in many other mappings (plain, bitlocker, veracrypt, ...)
>>>>>Just use the dm-crypt key, do not reference LUKS at all.
>>>>
>>>>Thanks for the reminding! That comment was on the first RFC version. But
>>>>starting with "RFC v2", there is no longer any interaction with dm-crypt
>>>>(to save a copy of the LUKS volume key for the kdump kernel) and now I
>>>>make cryptsetup talks to the kdump part of the kernel via the sysfs to
>>>>reuse the volume key. So only the kdump part of the kernel needs to know
>>>>LUKS which is what it cares. Thus I don't think there is any kernel
>>>>namespace pollution now.
>>>
>>>Hi,
>>>
>>>I am sorry if I did understand correctly, but I thought that kdump is part
>>>of the kernel.
>>
>>Yes, there is the kernel part of the kdump although there is also the
>>userspace part to make the feature complete:)
>>
>>>
>>>I am trying to say that kernel generally has no concept of LUKS;
>>>this is a userspace abstraction for key management.
>>>
>>>Even the cryptsetup dm-crypt configuration mapping table generated from LUKS
>>>has nothing LUKS special in it (only in DM-UUID as a name prefix).
>>>
>>>So I do not understand why you need to mention LUKS even in kdump part.
>>>Perhaps it is still only a naming problem, nothing more.
>>>
>>>All you need is to preserve key and configuration parameters (for dm-crypt).
>>>If it is set by cryptsetup, dmsetup, or any other way is not important - on this
>>>kernel layer, it has nothing to do with LUKS key management metadata.
>>>
>>>No problem if you support only LUKS in userspace, but really, all this machinery
>>>should work for any dm-crypt devices. Perhaps your patch even works for it already.
>>
>>Thanks for the explanation! After reflecting on your words for some
>>time, I realize I had an implicit assumption. I assumed is if I use a
>>name like dm_crypt_key instead of luks_volume_key, I need to support all
>>mappings like plain, bitlocker, veracrypt as mentioned by you and this
>>could mean much more efforts. So I'm not motivated to do that as
>>currently users only request kdump to work for LUKS.
>
>Thanks, I think it is perfectly fine to implement just subset here.
>
[...]
>
>My comment was just about proper naming in kernel, it is of course up to you
>what you want to support in userspace (and even in kernel, extensions can
>be added later).

Thanks for the confirmation!

>
>Only LUKS2 uses keyring for volume key in dm-crypt as default option anyway.

Thanks for the info!

>I do not think you need any cryptsetup patches, all you need is to write
>decrypted volume key from LUKS metadata with
>  cryptsetup luksDump ---dump-volume-key -volume-key-file <out> <device>
>(or any code equivalent with libcryptsetup), am I correct?

Correct me if I'm wrong, but I don't think there will be a safer way to
preserve key without patching cryptsetup. Actually the --dump-volume-key
approach has been proposed before and I agree with your conclusion [1]
on that approach i.e. "passing volume key this way is quite insecure".
Without patching cryptsetup, even if I save the volume key in the memory
reserved for the kdump kernel, I need to retrieve this key in the
userspace to unlock the LUKS device which may lead to quite a security
vulnerability.

I respect the efforts from you and the cryptsetup community to make LUKS
as secure as possible. And kdump is used in product environment. Kdump
is to a server as a black box is to an aircraft. So by no means I want
to reverse the used security measures and patching cryptsetup can allow
to keep the security measures. One concern raised by you against "FRC
v1" was a copy of the LUKS volume key for the kdump kernel creates an
attack vector. I took this feedback seriously and have sought advice
from my colleagues to implement the countermeasures ([PATCH 1/5] and
[Patch 4/5]).

[1] https://yhbt.net/lore/all/e5abd089-3398-fdb4-7991-0019be434b79@gmail.com/


>
>Milan
>

-- 
Best regards,
Coiby


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Coiby Xu]

On Wed, Jun 07, 2023 at 08:14:44AM +0200, Milan Broz wrote:
>On 6/6/23 13:02, Coiby Xu wrote:
>>On Mon, Jun 05, 2023 at 09:09:49AM +0200, Milan Broz wrote:
>>>On 6/5/23 04:31, Coiby Xu wrote:
>>>>Hi Eric and Milan,
>>>>
>>>>On Sat, Jun 03, 2023 at 11:22:52AM +0200, Milan Broz wrote:
>>>>>On 6/2/23 23:34, Eric Biggers wrote:
>>>>>>On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>>>>>>>[PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
>>>>>>
>>>>>>The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
>>>>>>happens to use.  But LUKS is a userspace concept.
>>>>>>
>>>>>>This is a kernel patchset, so why does it make sense for it to be talking about
>>>>>>LUKS at all?  Perhaps you mean dm-crypt?
>>>>>
>>>>>Exactly.
>>>>
>>>>Thanks for raising the above concern! The use cases like CoreOS and
>>>>Confidential VMs explicitly want kdump to work for LUKS. And correct me
>>>>if I'm wrong, I think the two problems addressed by this patch set only
>>>>apply to LUKS so the kdump part of the kernel only cares about the LUKS
>>>>case. If there are use cases where similar approach is needed, I'll be
>>>>happy to make the solution more generic.
>>>>
>>>>>
>>>>>I had the same comment almost a year ago... and it still applies:
>>>>>https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
>>>>>
>>>>>Anyway, please fix the naming before this patchset can be read or reviewed!
>>>>>
>>>>>LUKS is user-space key management only (on-disk metadata); the kernel has
>>>>>no idea how the key is derived or what LUKS is - dm-crypt only knows the key
>>>>>(either through keyring or directly in the mapping table).
>>>>>
>>>>>Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
>>>>>is used in many other mappings (plain, bitlocker, veracrypt, ...)
>>>>>Just use the dm-crypt key, do not reference LUKS at all.
>>>>
>>>>Thanks for the reminding! That comment was on the first RFC version. But
>>>>starting with "RFC v2", there is no longer any interaction with dm-crypt
>>>>(to save a copy of the LUKS volume key for the kdump kernel) and now I
>>>>make cryptsetup talks to the kdump part of the kernel via the sysfs to
>>>>reuse the volume key. So only the kdump part of the kernel needs to know
>>>>LUKS which is what it cares. Thus I don't think there is any kernel
>>>>namespace pollution now.
>>>
>>>Hi,
>>>
>>>I am sorry if I did understand correctly, but I thought that kdump is part
>>>of the kernel.
>>
>>Yes, there is the kernel part of the kdump although there is also the
>>userspace part to make the feature complete:)
>>
>>>
>>>I am trying to say that kernel generally has no concept of LUKS;
>>>this is a userspace abstraction for key management.
>>>
>>>Even the cryptsetup dm-crypt configuration mapping table generated from LUKS
>>>has nothing LUKS special in it (only in DM-UUID as a name prefix).
>>>
>>>So I do not understand why you need to mention LUKS even in kdump part.
>>>Perhaps it is still only a naming problem, nothing more.
>>>
>>>All you need is to preserve key and configuration parameters (for dm-crypt).
>>>If it is set by cryptsetup, dmsetup, or any other way is not important - on this
>>>kernel layer, it has nothing to do with LUKS key management metadata.
>>>
>>>No problem if you support only LUKS in userspace, but really, all this machinery
>>>should work for any dm-crypt devices. Perhaps your patch even works for it already.
>>
>>Thanks for the explanation! After reflecting on your words for some
>>time, I realize I had an implicit assumption. I assumed is if I use a
>>name like dm_crypt_key instead of luks_volume_key, I need to support all
>>mappings like plain, bitlocker, veracrypt as mentioned by you and this
>>could mean much more efforts. So I'm not motivated to do that as
>>currently users only request kdump to work for LUKS.
>
>Thanks, I think it is perfectly fine to implement just subset here.
>
[...]
>
>My comment was just about proper naming in kernel, it is of course up to you
>what you want to support in userspace (and even in kernel, extensions can
>be added later).

Thanks for the confirmation!

>
>Only LUKS2 uses keyring for volume key in dm-crypt as default option anyway.

Thanks for the info!

>I do not think you need any cryptsetup patches, all you need is to write
>decrypted volume key from LUKS metadata with
>  cryptsetup luksDump ---dump-volume-key -volume-key-file <out> <device>
>(or any code equivalent with libcryptsetup), am I correct?

Correct me if I'm wrong, but I don't think there will be a safer way to
preserve key without patching cryptsetup. Actually the --dump-volume-key
approach has been proposed before and I agree with your conclusion [1]
on that approach i.e. "passing volume key this way is quite insecure".
Without patching cryptsetup, even if I save the volume key in the memory
reserved for the kdump kernel, I need to retrieve this key in the
userspace to unlock the LUKS device which may lead to quite a security
vulnerability.

I respect the efforts from you and the cryptsetup community to make LUKS
as secure as possible. And kdump is used in product environment. Kdump
is to a server as a black box is to an aircraft. So by no means I want
to reverse the used security measures and patching cryptsetup can allow
to keep the security measures. One concern raised by you against "FRC
v1" was a copy of the LUKS volume key for the kdump kernel creates an
attack vector. I took this feedback seriously and have sought advice
from my colleagues to implement the countermeasures ([PATCH 1/5] and
[Patch 4/5]).

[1] https://yhbt.net/lore/all/e5abd089-3398-fdb4-7991-0019be434b79@gmail.com/


>
>Milan
>

-- 
Best regards,
Coiby

Re: [dm-devel] [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Coiby Xu]

On Wed, Jun 07, 2023 at 08:14:44AM +0200, Milan Broz wrote:
>On 6/6/23 13:02, Coiby Xu wrote:
>>On Mon, Jun 05, 2023 at 09:09:49AM +0200, Milan Broz wrote:
>>>On 6/5/23 04:31, Coiby Xu wrote:
>>>>Hi Eric and Milan,
>>>>
>>>>On Sat, Jun 03, 2023 at 11:22:52AM +0200, Milan Broz wrote:
>>>>>On 6/2/23 23:34, Eric Biggers wrote:
>>>>>>On Thu, Jun 01, 2023 at 03:24:39PM +0800, Coiby Xu wrote:
>>>>>>>[PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key
>>>>>>
>>>>>>The kernel has no concept of LUKS at all.  It provides dm-crypt, which LUKS
>>>>>>happens to use.  But LUKS is a userspace concept.
>>>>>>
>>>>>>This is a kernel patchset, so why does it make sense for it to be talking about
>>>>>>LUKS at all?  Perhaps you mean dm-crypt?
>>>>>
>>>>>Exactly.
>>>>
>>>>Thanks for raising the above concern! The use cases like CoreOS and
>>>>Confidential VMs explicitly want kdump to work for LUKS. And correct me
>>>>if I'm wrong, I think the two problems addressed by this patch set only
>>>>apply to LUKS so the kdump part of the kernel only cares about the LUKS
>>>>case. If there are use cases where similar approach is needed, I'll be
>>>>happy to make the solution more generic.
>>>>
>>>>>
>>>>>I had the same comment almost a year ago... and it still applies:
>>>>>https://lore.kernel.org/all/c857dcf8-024e-ab8a-fd26-295ce2e0ae41@gmail.com/
>>>>>
>>>>>Anyway, please fix the naming before this patchset can be read or reviewed!
>>>>>
>>>>>LUKS is user-space key management only (on-disk metadata); the kernel has
>>>>>no idea how the key is derived or what LUKS is - dm-crypt only knows the key
>>>>>(either through keyring or directly in the mapping table).
>>>>>
>>>>>Polluting kernel namespace with "luks" names variables is wrong - dm-crypt
>>>>>is used in many other mappings (plain, bitlocker, veracrypt, ...)
>>>>>Just use the dm-crypt key, do not reference LUKS at all.
>>>>
>>>>Thanks for the reminding! That comment was on the first RFC version. But
>>>>starting with "RFC v2", there is no longer any interaction with dm-crypt
>>>>(to save a copy of the LUKS volume key for the kdump kernel) and now I
>>>>make cryptsetup talks to the kdump part of the kernel via the sysfs to
>>>>reuse the volume key. So only the kdump part of the kernel needs to know
>>>>LUKS which is what it cares. Thus I don't think there is any kernel
>>>>namespace pollution now.
>>>
>>>Hi,
>>>
>>>I am sorry if I did understand correctly, but I thought that kdump is part
>>>of the kernel.
>>
>>Yes, there is the kernel part of the kdump although there is also the
>>userspace part to make the feature complete:)
>>
>>>
>>>I am trying to say that kernel generally has no concept of LUKS;
>>>this is a userspace abstraction for key management.
>>>
>>>Even the cryptsetup dm-crypt configuration mapping table generated from LUKS
>>>has nothing LUKS special in it (only in DM-UUID as a name prefix).
>>>
>>>So I do not understand why you need to mention LUKS even in kdump part.
>>>Perhaps it is still only a naming problem, nothing more.
>>>
>>>All you need is to preserve key and configuration parameters (for dm-crypt).
>>>If it is set by cryptsetup, dmsetup, or any other way is not important - on this
>>>kernel layer, it has nothing to do with LUKS key management metadata.
>>>
>>>No problem if you support only LUKS in userspace, but really, all this machinery
>>>should work for any dm-crypt devices. Perhaps your patch even works for it already.
>>
>>Thanks for the explanation! After reflecting on your words for some
>>time, I realize I had an implicit assumption. I assumed is if I use a
>>name like dm_crypt_key instead of luks_volume_key, I need to support all
>>mappings like plain, bitlocker, veracrypt as mentioned by you and this
>>could mean much more efforts. So I'm not motivated to do that as
>>currently users only request kdump to work for LUKS.
>
>Thanks, I think it is perfectly fine to implement just subset here.
>
[...]
>
>My comment was just about proper naming in kernel, it is of course up to you
>what you want to support in userspace (and even in kernel, extensions can
>be added later).

Thanks for the confirmation!

>
>Only LUKS2 uses keyring for volume key in dm-crypt as default option anyway.

Thanks for the info!

>I do not think you need any cryptsetup patches, all you need is to write
>decrypted volume key from LUKS metadata with
>  cryptsetup luksDump ---dump-volume-key -volume-key-file <out> <device>
>(or any code equivalent with libcryptsetup), am I correct?

Correct me if I'm wrong, but I don't think there will be a safer way to
preserve key without patching cryptsetup. Actually the --dump-volume-key
approach has been proposed before and I agree with your conclusion [1]
on that approach i.e. "passing volume key this way is quite insecure".
Without patching cryptsetup, even if I save the volume key in the memory
reserved for the kdump kernel, I need to retrieve this key in the
userspace to unlock the LUKS device which may lead to quite a security
vulnerability.

I respect the efforts from you and the cryptsetup community to make LUKS
as secure as possible. And kdump is used in product environment. Kdump
is to a server as a black box is to an aircraft. So by no means I want
to reverse the used security measures and patching cryptsetup can allow
to keep the security measures. One concern raised by you against "FRC
v1" was a copy of the LUKS volume key for the kdump kernel creates an
attack vector. I took this feedback seriously and have sought advice
from my colleagues to implement the countermeasures ([PATCH 1/5] and
[Patch 4/5]).

[1] https://yhbt.net/lore/all/e5abd089-3398-fdb4-7991-0019be434b79@gmail.com/


>
>Milan
>

-- 
Best regards,
Coiby

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Milan Broz]

On 6/7/23 14:39, Coiby Xu wrote:
...
>> I do not think you need any cryptsetup patches, all you need is to write
>> decrypted volume key from LUKS metadata with
>>   cryptsetup luksDump ---dump-volume-key -volume-key-file <out> <device>
>> (or any code equivalent with libcryptsetup), am I correct?
> 
> Correct me if I'm wrong, but I don't think there will be a safer way to
> preserve key without patching cryptsetup. Actually the --dump-volume-key
> approach has been proposed before and I agree with your conclusion [1]
> on that approach i.e. "passing volume key this way is quite insecure".
> Without patching cryptsetup, even if I save the volume key in the memory
> reserved for the kdump kernel, I need to retrieve this key in the
> userspace to unlock the LUKS device which may lead to quite a security
> vulnerability.

Hm, where are the patches for cryptsetup, then? I am afraid we do not want
to add such specific things there.

But we are just going to merge a patchset that changes how we use keyring
where you can tell cryptsetup to store/link key under some specific name
and to specific keyring
(see https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/492)
(Please talk to Red Hat cryptsetup maintainers for more info,
I just mentioned this mail to them today.)

> I respect the efforts from you and the cryptsetup community to make LUKS
> as secure as possible. And kdump is used in product environment. Kdump
> is to a server as a black box is to an aircraft. So by no means I want
> to reverse the used security measures and patching cryptsetup can allow
> to keep the security measures. One concern raised by you against "FRC
> v1" was a copy of the LUKS volume key for the kdump kernel creates an
> attack vector. I took this feedback seriously and have sought advice
> from my colleagues to implement the countermeasures ([PATCH 1/5] and
> [Patch 4/5]).
> 
> [1] https://yhbt.net/lore/all/e5abd089-3398-fdb4-7991-0019be434b79@gmail.com/

Yes, I appreciate that. And it is perfectly ok if your customers accept
the trade-off and security risk of handling the key this way.

Anyway, I feel we are going in circles here, and as it seems to be my fault,
I do not want to sound grumpy as I am perhaps missing some context.

Could you please talk to internal RH cryptsetup maintainers first and discuss
your solution? They know what we can do here can help to find an acceptable
solution. (I added cc to Ondra.)

Thanks,
Milan

Re: [dm-devel] [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Milan Broz]

On 6/7/23 14:39, Coiby Xu wrote:
...
>> I do not think you need any cryptsetup patches, all you need is to write
>> decrypted volume key from LUKS metadata with
>>   cryptsetup luksDump ---dump-volume-key -volume-key-file <out> <device>
>> (or any code equivalent with libcryptsetup), am I correct?
> 
> Correct me if I'm wrong, but I don't think there will be a safer way to
> preserve key without patching cryptsetup. Actually the --dump-volume-key
> approach has been proposed before and I agree with your conclusion [1]
> on that approach i.e. "passing volume key this way is quite insecure".
> Without patching cryptsetup, even if I save the volume key in the memory
> reserved for the kdump kernel, I need to retrieve this key in the
> userspace to unlock the LUKS device which may lead to quite a security
> vulnerability.

Hm, where are the patches for cryptsetup, then? I am afraid we do not want
to add such specific things there.

But we are just going to merge a patchset that changes how we use keyring
where you can tell cryptsetup to store/link key under some specific name
and to specific keyring
(see https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/492)
(Please talk to Red Hat cryptsetup maintainers for more info,
I just mentioned this mail to them today.)

> I respect the efforts from you and the cryptsetup community to make LUKS
> as secure as possible. And kdump is used in product environment. Kdump
> is to a server as a black box is to an aircraft. So by no means I want
> to reverse the used security measures and patching cryptsetup can allow
> to keep the security measures. One concern raised by you against "FRC
> v1" was a copy of the LUKS volume key for the kdump kernel creates an
> attack vector. I took this feedback seriously and have sought advice
> from my colleagues to implement the countermeasures ([PATCH 1/5] and
> [Patch 4/5]).
> 
> [1] https://yhbt.net/lore/all/e5abd089-3398-fdb4-7991-0019be434b79@gmail.com/

Yes, I appreciate that. And it is perfectly ok if your customers accept
the trade-off and security risk of handling the key this way.

Anyway, I feel we are going in circles here, and as it seems to be my fault,
I do not want to sound grumpy as I am perhaps missing some context.

Could you please talk to internal RH cryptsetup maintainers first and discuss
your solution? They know what we can do here can help to find an acceptable
solution. (I added cc to Ondra.)

Thanks,
Milan

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Milan Broz]

On 6/7/23 14:39, Coiby Xu wrote:
...
>> I do not think you need any cryptsetup patches, all you need is to write
>> decrypted volume key from LUKS metadata with
>>   cryptsetup luksDump ---dump-volume-key -volume-key-file <out> <device>
>> (or any code equivalent with libcryptsetup), am I correct?
> 
> Correct me if I'm wrong, but I don't think there will be a safer way to
> preserve key without patching cryptsetup. Actually the --dump-volume-key
> approach has been proposed before and I agree with your conclusion [1]
> on that approach i.e. "passing volume key this way is quite insecure".
> Without patching cryptsetup, even if I save the volume key in the memory
> reserved for the kdump kernel, I need to retrieve this key in the
> userspace to unlock the LUKS device which may lead to quite a security
> vulnerability.

Hm, where are the patches for cryptsetup, then? I am afraid we do not want
to add such specific things there.

But we are just going to merge a patchset that changes how we use keyring
where you can tell cryptsetup to store/link key under some specific name
and to specific keyring
(see https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/492)
(Please talk to Red Hat cryptsetup maintainers for more info,
I just mentioned this mail to them today.)

> I respect the efforts from you and the cryptsetup community to make LUKS
> as secure as possible. And kdump is used in product environment. Kdump
> is to a server as a black box is to an aircraft. So by no means I want
> to reverse the used security measures and patching cryptsetup can allow
> to keep the security measures. One concern raised by you against "FRC
> v1" was a copy of the LUKS volume key for the kdump kernel creates an
> attack vector. I took this feedback seriously and have sought advice
> from my colleagues to implement the countermeasures ([PATCH 1/5] and
> [Patch 4/5]).
> 
> [1] https://yhbt.net/lore/all/e5abd089-3398-fdb4-7991-0019be434b79@gmail.com/

Yes, I appreciate that. And it is perfectly ok if your customers accept
the trade-off and security risk of handling the key this way.

Anyway, I feel we are going in circles here, and as it seems to be my fault,
I do not want to sound grumpy as I am perhaps missing some context.

Could you please talk to internal RH cryptsetup maintainers first and discuss
your solution? They know what we can do here can help to find an acceptable
solution. (I added cc to Ondra.)

Thanks,
Milan

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Coiby Xu]

On Thu, Jun 08, 2023 at 12:39:26PM +0200, Milan Broz wrote:
>On 6/7/23 14:39, Coiby Xu wrote:
>...
>>>I do not think you need any cryptsetup patches, all you need is to write
>>>decrypted volume key from LUKS metadata with
>>>  cryptsetup luksDump ---dump-volume-key -volume-key-file <out> <device>
>>>(or any code equivalent with libcryptsetup), am I correct?
>>
>>Correct me if I'm wrong, but I don't think there will be a safer way to
>>preserve key without patching cryptsetup. Actually the --dump-volume-key
>>approach has been proposed before and I agree with your conclusion [1]
>>on that approach i.e. "passing volume key this way is quite insecure".
>>Without patching cryptsetup, even if I save the volume key in the memory
>>reserved for the kdump kernel, I need to retrieve this key in the
>>userspace to unlock the LUKS device which may lead to quite a security
>>vulnerability.
>
>Hm, where are the patches for cryptsetup, then? I am afraid we do not want
>to add such specific things there.

Thanks for cleaning up the text to make the discussion easier! Sorry I
only mentioned it [3] in the cover letter and didn't provide one in
previous reply. [3] was done in a quick-and-dirty way (I plan to send a
formal merge request after finishing the kernel part) and there is no need
to read it. Let's me explain what [3] does here instead,
  1) After unlocking the LUKS-encrypted device, if cryptsetup finds
     /sys/kernel/crash_luks_volume_key exists, it will write the key
     description of the volume key to it to notify the kernel to save a copy
     of this logon key linked to its thread keyring for the kdump kernel
  2) After the 1st kernel crashes, if crytpsetup finds it's in the kdump
     kernel, instead of deriving the volume key from a passphrase, it
     will write the key description to /sys/kernel/crash_luks_volume_key
     to ask the kdump kernel to link the saved key to its thread keyring.

[3] https://gitlab.com/coxu/cryptsetup/-/commit/750a46d933fac82e0c994b5c41de40a0b8cac647

>
>But we are just going to merge a patchset that changes how we use keyring
>where you can tell cryptsetup to store/link key under some specific name
>and to specific keyring
>(see https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/492)
>(Please talk to Red Hat cryptsetup maintainers for more info,
>I just mentioned this mail to them today.)

Thanks for pointing me to the above MR which looks promising! Unlike
treating the kdump use case as a special case [3], it just provides a
generic way with the implemented options --link-vk-to-keyring and
--volume-key-keyring.

>
>>I respect the efforts from you and the cryptsetup community to make LUKS
>>as secure as possible. And kdump is used in product environment. Kdump
>>is to a server as a black box is to an aircraft. So by no means I want
>>to reverse the used security measures and patching cryptsetup can allow
>>to keep the security measures. One concern raised by you against "FRC
>>v1" was a copy of the LUKS volume key for the kdump kernel creates an
>>attack vector. I took this feedback seriously and have sought advice
>>from my colleagues to implement the countermeasures ([PATCH 1/5] and
>>[Patch 4/5]).
>>
>>[1] https://yhbt.net/lore/all/e5abd089-3398-fdb4-7991-0019be434b79@gmail.com/
>
>Yes, I appreciate that. And it is perfectly ok if your customers accept
>the trade-off and security risk of handling the key this way.
>
>Anyway, I feel we are going in circles here, and as it seems to be my fault,
>I do not want to sound grumpy as I am perhaps missing some context.

Actually I should thank you for your patience! You have been always
offering your feedback on this work kindly and promptly starting with
the first proposed solution [1].

>
>Could you please talk to internal RH cryptsetup maintainers first and discuss
>your solution? They know what we can do here can help to find an acceptable
>solution. (I added cc to Ondra.)

Sure, I'll talk to them first. Thanks for letting Ondra know!

>
>Thanks,
>Milan
>

-- 
Best regards,
Coiby


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Coiby Xu]

On Thu, Jun 08, 2023 at 12:39:26PM +0200, Milan Broz wrote:
>On 6/7/23 14:39, Coiby Xu wrote:
>...
>>>I do not think you need any cryptsetup patches, all you need is to write
>>>decrypted volume key from LUKS metadata with
>>>  cryptsetup luksDump ---dump-volume-key -volume-key-file <out> <device>
>>>(or any code equivalent with libcryptsetup), am I correct?
>>
>>Correct me if I'm wrong, but I don't think there will be a safer way to
>>preserve key without patching cryptsetup. Actually the --dump-volume-key
>>approach has been proposed before and I agree with your conclusion [1]
>>on that approach i.e. "passing volume key this way is quite insecure".
>>Without patching cryptsetup, even if I save the volume key in the memory
>>reserved for the kdump kernel, I need to retrieve this key in the
>>userspace to unlock the LUKS device which may lead to quite a security
>>vulnerability.
>
>Hm, where are the patches for cryptsetup, then? I am afraid we do not want
>to add such specific things there.

Thanks for cleaning up the text to make the discussion easier! Sorry I
only mentioned it [3] in the cover letter and didn't provide one in
previous reply. [3] was done in a quick-and-dirty way (I plan to send a
formal merge request after finishing the kernel part) and there is no need
to read it. Let's me explain what [3] does here instead,
  1) After unlocking the LUKS-encrypted device, if cryptsetup finds
     /sys/kernel/crash_luks_volume_key exists, it will write the key
     description of the volume key to it to notify the kernel to save a copy
     of this logon key linked to its thread keyring for the kdump kernel
  2) After the 1st kernel crashes, if crytpsetup finds it's in the kdump
     kernel, instead of deriving the volume key from a passphrase, it
     will write the key description to /sys/kernel/crash_luks_volume_key
     to ask the kdump kernel to link the saved key to its thread keyring.

[3] https://gitlab.com/coxu/cryptsetup/-/commit/750a46d933fac82e0c994b5c41de40a0b8cac647

>
>But we are just going to merge a patchset that changes how we use keyring
>where you can tell cryptsetup to store/link key under some specific name
>and to specific keyring
>(see https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/492)
>(Please talk to Red Hat cryptsetup maintainers for more info,
>I just mentioned this mail to them today.)

Thanks for pointing me to the above MR which looks promising! Unlike
treating the kdump use case as a special case [3], it just provides a
generic way with the implemented options --link-vk-to-keyring and
--volume-key-keyring.

>
>>I respect the efforts from you and the cryptsetup community to make LUKS
>>as secure as possible. And kdump is used in product environment. Kdump
>>is to a server as a black box is to an aircraft. So by no means I want
>>to reverse the used security measures and patching cryptsetup can allow
>>to keep the security measures. One concern raised by you against "FRC
>>v1" was a copy of the LUKS volume key for the kdump kernel creates an
>>attack vector. I took this feedback seriously and have sought advice
>>from my colleagues to implement the countermeasures ([PATCH 1/5] and
>>[Patch 4/5]).
>>
>>[1] https://yhbt.net/lore/all/e5abd089-3398-fdb4-7991-0019be434b79@gmail.com/
>
>Yes, I appreciate that. And it is perfectly ok if your customers accept
>the trade-off and security risk of handling the key this way.
>
>Anyway, I feel we are going in circles here, and as it seems to be my fault,
>I do not want to sound grumpy as I am perhaps missing some context.

Actually I should thank you for your patience! You have been always
offering your feedback on this work kindly and promptly starting with
the first proposed solution [1].

>
>Could you please talk to internal RH cryptsetup maintainers first and discuss
>your solution? They know what we can do here can help to find an acceptable
>solution. (I added cc to Ondra.)

Sure, I'll talk to them first. Thanks for letting Ondra know!

>
>Thanks,
>Milan
>

-- 
Best regards,
Coiby

Re: [dm-devel] [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS volume key

[Coiby Xu]

On Thu, Jun 08, 2023 at 12:39:26PM +0200, Milan Broz wrote:
>On 6/7/23 14:39, Coiby Xu wrote:
>...
>>>I do not think you need any cryptsetup patches, all you need is to write
>>>decrypted volume key from LUKS metadata with
>>>  cryptsetup luksDump ---dump-volume-key -volume-key-file <out> <device>
>>>(or any code equivalent with libcryptsetup), am I correct?
>>
>>Correct me if I'm wrong, but I don't think there will be a safer way to
>>preserve key without patching cryptsetup. Actually the --dump-volume-key
>>approach has been proposed before and I agree with your conclusion [1]
>>on that approach i.e. "passing volume key this way is quite insecure".
>>Without patching cryptsetup, even if I save the volume key in the memory
>>reserved for the kdump kernel, I need to retrieve this key in the
>>userspace to unlock the LUKS device which may lead to quite a security
>>vulnerability.
>
>Hm, where are the patches for cryptsetup, then? I am afraid we do not want
>to add such specific things there.

Thanks for cleaning up the text to make the discussion easier! Sorry I
only mentioned it [3] in the cover letter and didn't provide one in
previous reply. [3] was done in a quick-and-dirty way (I plan to send a
formal merge request after finishing the kernel part) and there is no need
to read it. Let's me explain what [3] does here instead,
  1) After unlocking the LUKS-encrypted device, if cryptsetup finds
     /sys/kernel/crash_luks_volume_key exists, it will write the key
     description of the volume key to it to notify the kernel to save a copy
     of this logon key linked to its thread keyring for the kdump kernel
  2) After the 1st kernel crashes, if crytpsetup finds it's in the kdump
     kernel, instead of deriving the volume key from a passphrase, it
     will write the key description to /sys/kernel/crash_luks_volume_key
     to ask the kdump kernel to link the saved key to its thread keyring.

[3] https://gitlab.com/coxu/cryptsetup/-/commit/750a46d933fac82e0c994b5c41de40a0b8cac647

>
>But we are just going to merge a patchset that changes how we use keyring
>where you can tell cryptsetup to store/link key under some specific name
>and to specific keyring
>(see https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/492)
>(Please talk to Red Hat cryptsetup maintainers for more info,
>I just mentioned this mail to them today.)

Thanks for pointing me to the above MR which looks promising! Unlike
treating the kdump use case as a special case [3], it just provides a
generic way with the implemented options --link-vk-to-keyring and
--volume-key-keyring.

>
>>I respect the efforts from you and the cryptsetup community to make LUKS
>>as secure as possible. And kdump is used in product environment. Kdump
>>is to a server as a black box is to an aircraft. So by no means I want
>>to reverse the used security measures and patching cryptsetup can allow
>>to keep the security measures. One concern raised by you against "FRC
>>v1" was a copy of the LUKS volume key for the kdump kernel creates an
>>attack vector. I took this feedback seriously and have sought advice
>>from my colleagues to implement the countermeasures ([PATCH 1/5] and
>>[Patch 4/5]).
>>
>>[1] https://yhbt.net/lore/all/e5abd089-3398-fdb4-7991-0019be434b79@gmail.com/
>
>Yes, I appreciate that. And it is perfectly ok if your customers accept
>the trade-off and security risk of handling the key this way.
>
>Anyway, I feel we are going in circles here, and as it seems to be my fault,
>I do not want to sound grumpy as I am perhaps missing some context.

Actually I should thank you for your patience! You have been always
offering your feedback on this work kindly and promptly starting with
the first proposed solution [1].

>
>Could you please talk to internal RH cryptsetup maintainers first and discuss
>your solution? They know what we can do here can help to find an acceptable
>solution. (I added cc to Ondra.)

Sure, I'll talk to them first. Thanks for letting Ondra know!

>
>Thanks,
>Milan
>

-- 
Best regards,
Coiby

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel