Re: [PATCH v6 4/6] x86/e820: Tag e820_entry with crypto capabilities

From: Martin Fernandez <martin.fernandez@eclypsium.com>
To: Kees Cook <keescook@chromium.org>
Cc: linux-kernel@vger.kernel.org, linux-efi@vger.kernel.org,
	 platform-driver-x86@vger.kernel.org, linux-mm@kvack.org,
	tglx@linutronix.de,  mingo@redhat.com, bp@alien8.de,
	dave.hansen@linux.intel.com, x86@kernel.org,  hpa@zytor.com,
	ardb@kernel.org, dvhart@infradead.org, andy@infradead.org,
	 gregkh@linuxfoundation.org, rafael@kernel.org, rppt@kernel.org,
	 akpm@linux-foundation.org, daniel.gutson@eclypsium.com,
	hughsient@gmail.com,  alex.bazhaniuk@eclypsium.com,
	alison.schofield@intel.com
Subject: Re: [PATCH v6 4/6] x86/e820: Tag e820_entry with crypto capabilities
Date: Tue, 8 Feb 2022 11:46:35 -0300	[thread overview]
Message-ID: <CAKgze5YORW6b4ePx=0v22gKcEEh2iF6eh+W5pYv+OprmfsucqQ@mail.gmail.com> (raw)
In-Reply-To: <202202071351.AEEEA92@keescook>

On 2/7/22, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Feb 03, 2022 at 01:43:26PM -0300, Martin Fernandez wrote:
>> Add a new enum for crypto capabilities.
>>
>> Add a new member in e820_entry to hold whether an entry is able to do
>> hardware memory encryption or not.
>>
>> Add a new function e820__range_set_crypto_capable to mark all the
>> entries in a range of addresses as encryptable. This will be called
>> when initializing EFI.
>>
>> Change e820__update_table to handle merging and overlap problems
>> taking into account crypto_capable.
>>
>> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
>> ---
>>  arch/x86/include/asm/e820/api.h   |   1 +
>>  arch/x86/include/asm/e820/types.h |  12 +++-
>>  arch/x86/kernel/e820.c            | 114 ++++++++++++++++++++++++++++--
>>  3 files changed, 119 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/e820/api.h
>> b/arch/x86/include/asm/e820/api.h
>> index e8f58ddd06d9..4b3b01fafdd1 100644
>> --- a/arch/x86/include/asm/e820/api.h
>> +++ b/arch/x86/include/asm/e820/api.h
>> @@ -17,6 +17,7 @@ extern bool e820__mapped_all(u64 start, u64 end, enum
>> e820_type type);
>>  extern void e820__range_add   (u64 start, u64 size, enum e820_type
>> type);
>>  extern u64  e820__range_update(u64 start, u64 size, enum e820_type
>> old_type, enum e820_type new_type);
>>  extern u64  e820__range_remove(u64 start, u64 size, enum e820_type
>> old_type, bool check_type);
>> +extern u64  e820__range_set_crypto_capable(u64 start, u64 size);
>>
>>  extern void e820__print_table(char *who);
>>  extern int  e820__update_table(struct e820_table *table);
>> diff --git a/arch/x86/include/asm/e820/types.h
>> b/arch/x86/include/asm/e820/types.h
>> index 314f75d886d0..aef03c665f5e 100644
>> --- a/arch/x86/include/asm/e820/types.h
>> +++ b/arch/x86/include/asm/e820/types.h
>> @@ -46,6 +46,11 @@ enum e820_type {
>>  	E820_TYPE_RESERVED_KERN	= 128,
>>  };
>>
>> +enum e820_crypto_capabilities {
>> +	E820_NOT_CRYPTO_CAPABLE	= 0,
>> +	E820_CRYPTO_CAPABLE	= 1,
>> +};
>
> Is this expected to grow beyond a bool?
>

People commented that maybe it was a good idea to have the source of
the cryptographic capabilities, in this case that would be the EFI
memmap. So this could grow in that case.

Also the enum makes it self explanatory while using it in the code.