| Message ID | 20211203192148.585399-1-martin.fernandez@eclypsium.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | x86: Show in sysfs if a memory node is able to do encryption | expand |
Hi Martin, On Fri, Dec 03, 2021 at 04:21:43PM -0300, Martin Fernandez wrote: > Show for each node if every memory descriptor in that node has the > EFI_MEMORY_CPU_CRYPTO attribute. > > fwupd project plans to use it as part of a check to see if the users > have properly configured memory hardware encryption capabilities. It's > planned to make it part of a specification that can be passed to > people purchasing hardware. It's called Host Security ID: > https://fwupd.github.io/libfwupdplugin/hsi.html > > This also can be useful in the future if NUMA decides to prioritize > nodes that are able to do encryption. I'm missing a description about *how* the new APIs/ABIs are going to be used. This comment also applies to the changelogs of the patches that mostly describe what the patch does and do not describe why is it needed. > Changes since v2: > > e820__range_mark_crypto -> e820__range_mark_crypto_capable. > > In e820__range_remove: Create a region with crypto capabilities > instead of creating one without it and then mark it. > > > Changes since v1: > > Modify __e820__range_update to update the crypto capabilities of a > range; now this function will change the crypto capability of a range > if it's called with the same old_type and new_type. Rework > efi_mark_e820_regions_as_crypto_capable based on this. > > Update do_add_efi_memmap to mark the regions as it creates them. > > Change the type of crypto_capable in e820_entry from bool to u8. > > Fix e820__update_table changes. > > Remove memblock_add_crypto_capable. Now you have to add the region and > mark it then. > > Better place for crypto_capable in pglist_data. > > > Martin Fernandez (5): > mm/memblock: Tag memblocks with crypto capabilities > mm/mmzone: Tag pg_data_t with crypto capabilities > Tag e820_entry with crypto capabilities > x86/efi: Tag e820_entries as crypto capable from EFI memmap > drivers/node: Show in sysfs node's crypto capabilities > > arch/x86/include/asm/e820/api.h | 1 + > arch/x86/include/asm/e820/types.h | 1 + > arch/x86/kernel/e820.c | 59 ++++++++++++++++++++++++------- > arch/x86/platform/efi/efi.c | 25 +++++++++++++ > drivers/base/node.c | 10 ++++++ > include/linux/memblock.h | 5 +++ > include/linux/mmzone.h | 3 ++ > mm/memblock.c | 49 +++++++++++++++++++++++++ > mm/page_alloc.c | 1 + > 9 files changed, 142 insertions(+), 12 deletions(-) > > -- > 2.30.2 >
On Sun, 5 Dec 2021 at 06:04, Mike Rapoport <rppt@kernel.org> wrote: > On Fri, Dec 03, 2021 at 04:21:43PM -0300, Martin Fernandez wrote: > > fwupd project plans to use it as part of a check to see if the users > > have properly configured memory hardware encryption capabilities. > I'm missing a description about *how* the new APIs/ABIs are going to be > used. We're planning to use this feature in the Host Security ID checks done at every boot. Please see https://fwupd.github.io/libfwupdplugin/hsi.html for details. I'm happy to answer questions or concerns. Thanks! Richard
Hi Richard, On Mon, Dec 06, 2021 at 07:58:10PM +0000, Richard Hughes wrote: > On Sun, 5 Dec 2021 at 06:04, Mike Rapoport <rppt@kernel.org> wrote: > > On Fri, Dec 03, 2021 at 04:21:43PM -0300, Martin Fernandez wrote: > > > fwupd project plans to use it as part of a check to see if the users > > > have properly configured memory hardware encryption capabilities. > > I'm missing a description about *how* the new APIs/ABIs are going to be > > used. > > We're planning to use this feature in the Host Security ID checks done > at every boot. Please see > https://fwupd.github.io/libfwupdplugin/hsi.html for details. I'm happy > to answer questions or concerns. Thanks! Can you please describe the actual check for the memory encryption and how it would impact the HSI rating? I wonder, for example, why did you choose per-node reporting rather than per-region as described in UEFI spec.
On 12/7/21, Mike Rapoport <rppt@kernel.org> wrote: > Hi Richard, > > On Mon, Dec 06, 2021 at 07:58:10PM +0000, Richard Hughes wrote: >> On Sun, 5 Dec 2021 at 06:04, Mike Rapoport <rppt@kernel.org> wrote: >> > On Fri, Dec 03, 2021 at 04:21:43PM -0300, Martin Fernandez wrote: >> > > fwupd project plans to use it as part of a check to see if the users >> > > have properly configured memory hardware encryption capabilities. >> > I'm missing a description about *how* the new APIs/ABIs are going to be >> > used. >> >> We're planning to use this feature in the Host Security ID checks done >> at every boot. Please see >> https://fwupd.github.io/libfwupdplugin/hsi.html for details. I'm happy >> to answer questions or concerns. Thanks! > > Can you please describe the actual check for the memory encryption and how > it would impact the HSI rating? > > I wonder, for example, why did you choose per-node reporting rather than > per-region as described in UEFI spec. Some time ago we discussed about this and concluded with Dave Hansen that it was better to do it in this per-node way. This is the archive of the relevant discussion: http://lkml.iu.edu/hypermail/linux/kernel/2006.2/06753.html
On 12/7/21 11:45 AM, Martin Fernandez wrote: >> I wonder, for example, why did you choose per-node reporting rather than >> per-region as described in UEFI spec. > Some time ago we discussed about this and concluded with Dave Hansen > that it was better to do it in this per-node way. Physical memory regions aren't exposed to userspace in any meaningful way. An ABI that says "everything is encrypted" is pretty meaningless and only useful for this one, special case. A per-node ABI is useful for this case and is also useful going forward if folks want to target allocations from applications to NUMA nodes which have encryption capabilities. The ABI in this set is useful for the immediate case and is useful to other folks.
On Tue, Dec 07, 2021 at 11:52:54AM -0800, Dave Hansen wrote: > On 12/7/21 11:45 AM, Martin Fernandez wrote: > >> I wonder, for example, why did you choose per-node reporting rather than > >> per-region as described in UEFI spec. > > Some time ago we discussed about this and concluded with Dave Hansen > > that it was better to do it in this per-node way. > > Physical memory regions aren't exposed to userspace in any meaningful way. Well, we have /sys/firmware/memory that exposes e820... > An ABI that says "everything is encrypted" is pretty meaningless and > only useful for this one, special case. > > A per-node ABI is useful for this case and is also useful going forward > if folks want to target allocations from applications to NUMA nodes > which have encryption capabilities. The ABI in this set is useful for > the immediate case and is useful to other folks. I don't mind per-node ABI, I'm just concerned that having a small region without the encryption flag set will render the entire node "not encryptable". This may happen because a bug in firmware, a user that shoot themself in a leg with weird memmap= or some hidden gem in interaction between e820, EFI and memblock that we still didn't discover. I agree that per-node flag is useful, but maybe we should also have better granularity as well.
On 12/7/21 12:06 PM, Mike Rapoport wrote: >> An ABI that says "everything is encrypted" is pretty meaningless and >> only useful for this one, special case. >> >> A per-node ABI is useful for this case and is also useful going forward >> if folks want to target allocations from applications to NUMA nodes >> which have encryption capabilities. The ABI in this set is useful for >> the immediate case and is useful to other folks. > I don't mind per-node ABI, I'm just concerned that having a small region > without the encryption flag set will render the entire node "not > encryptable". This may happen because a bug in firmware, a user that shoot > themself in a leg with weird memmap= or some hidden gem in interaction > between e820, EFI and memblock that we still didn't discover. That's a good point. But, that seems more in the realm of a pr_{info,warn}_once() than something deserving of its own specific ABI. If we have a 100GB of a node that supports encryption, and 4k that causes the whole thing to be considered un-encryptable, a warning is be appropriate and feasible.
On Tue, 7 Dec 2021 at 07:25, Mike Rapoport <rppt@kernel.org> wrote: > Can you please describe the actual check for the memory encryption and how > it would impact the HSI rating? The problem HSI is trying to solve is that customers are buying systems where the CPU supports memory encryption, where the motherboard and dram controller support memory encryption and where the vendor says it's supported. But in some cases it's not working, either because the system firmware is not working properly, or some component requires updating to enable the feature. We're found quite a few cases where people assumed this was all working fine, but on looking closer, finding out that it's not working at all. The higher HSI rating would only be available where most of the system RAM is encrypted, although we've not worked out a heuristic number for "good enough" yet. > I wonder, for example, why did you choose per-node reporting rather than > per-region as described in UEFI spec. I think Dave is better to answer this question. Richard.