Date:   Wed, 7 Jun 2023 20:39:56 +0800
From:   Coiby Xu <coxu@...hat.com>
To:     Milan Broz <gmazyland@...il.com>
Cc:     Eric Biggers <ebiggers@...nel.org>, kexec@...ts.infradead.org,
        Baoquan He <bhe@...hat.com>, x86@...nel.org,
        dm-devel@...hat.com, Pingfan Liu <kernelfans@...il.com>,
        linux-kernel@...r.kernel.org, Dave Hansen <dave.hansen@...el.com>,
        Kairui Song <ryncsn@...il.com>,
        Jan Pazdziora <jpazdziora@...hat.com>,
        Thomas Staudt <tstaudt@...ibm.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Dave Young <dyoung@...hat.com>
Subject: Re: [PATCH 0/5] Support kdump with LUKS encryption by reusing LUKS
 volume key

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

Powered by blists - more mailing lists