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@xxxxxxxxx/
> > > > >
> > > > > 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@xxxxxxxxx/


> Milan

--
Best regards,
Coiby