Re: [RFC PATCH v2 0/3] Allow access to confidential computing secret area

From: Borislav Petkov
Date: Mon Jun 28 2021 - 15:28:38 EST


Just a couple of notes below:

On Mon, Jun 28, 2021 at 06:34:28PM +0000, Dov Murik wrote:
> Confidential computing hardware such as AMD SEV (Secure Encrypted
> Virtualization) allows guest owners to inject secrets into the VMs
> memory without the host/hypervisor being able to read them. In SEV,
> secret injection is performed early in the VM launch process, before the
> guest starts running.
>
> Support for secret injection is already available in OVMF (in its AmdSev
> package; see edk2 commit 01726b6d23d4 "OvmfPkg/AmdSev: Expose the Sev
> Secret area using a configuration table" [1]), but the secrets were not
> available in the guest kernel.
>
> The patch series copies the secrets from the EFI-provided memory to
> kernel reserved memory, and optionally exposes them to userspace via
> securityfs using a new sev_secret kernel module.
>
> The first patch in efi/libstub copies the secret area from the EFI
> memory to specially allocated memory; the second patch reserves that
> memory block; and the third patch introduces the new sev_secret module
> that exposes the content of the secret entries as securityfs files, and
> allows clearing out secrets with a file unlink interface.
>
> This has been tested with AMD SEV guests, but the kernel side of
> handling the secret area has no SEV-specific dependencies, and therefore
> should be usable for any confidential computing hardware that can
> publish the secret area via the standard EFI config table entry.
>
> Here is a simple example for usage of the sev_secret module in a guest to which
> secrets were injected during launch:

That's all fine and good but I miss the "why" in this explanation. I.e.,
a proper use case of a guest owner providing those sekrits to the guest
would be good.

>
> # modprobe sev_secret
> # ls -la /sys/kernel/security/sev_secret/

So that sysfs URL becomes an ABI. Shouldn't this be:

/sys/kernel/security/coco/

instead which stands for "confidential computing" and contains all kinds
of protected guest things. TDX might wanna do something similar there,
for example.

> total 0
> drwxr-xr-x 2 root root 0 Jun 28 11:54 .
> drwxr-xr-x 3 root root 0 Jun 28 11:54 ..
> -r--r----- 1 root root 0 Jun 28 11:54 736870e5-84f0-4973-92ec-06879ce3da0b
> -r--r----- 1 root root 0 Jun 28 11:54 83c83f7f-1356-4975-8b7e-d3a0b54312c6
> -r--r----- 1 root root 0 Jun 28 11:54 9553f55d-3da2-43ee-ab5d-ff17f78864d2
> -r--r----- 1 root root 0 Jun 28 11:54 e6f5a162-d67f-4750-a67c-5d065f2a9910
>
> # xxd /sys/kernel/security/sev_secret/e6f5a162-d67f-4750-a67c-5d065f2a9910
> 00000000: 7468 6573 652d 6172 652d 7468 652d 6b61 these-are-the-ka
> 00000010: 7461 2d73 6563 7265 7473 0001 0203 0405 ta-secrets......
> 00000020: 0607 ..
>
> # rm /sys/kernel/security/sev_secret/e6f5a162-d67f-4750-a67c-5d065f2a9910
>
> # ls -la /sys/kernel/security/sev_secret/
> total 0
> drwxr-xr-x 2 root root 0 Jun 28 11:55 .
> drwxr-xr-x 3 root root 0 Jun 28 11:54 ..
> -r--r----- 1 root root 0 Jun 28 11:54 736870e5-84f0-4973-92ec-06879ce3da0b
> -r--r----- 1 root root 0 Jun 28 11:54 83c83f7f-1356-4975-8b7e-d3a0b54312c6
> -r--r----- 1 root root 0 Jun 28 11:54 9553f55d-3da2-43ee-ab5d-ff17f78864d2
>
>
> [1] https://github.com/tianocore/edk2/commit/01726b6d23d4
>
> v2 changes:
> - Add unlink support in sev_secret securityfs.
>
>
> Dov Murik (3):
> efi/libstub: Copy confidential computing secret area
> efi: Reserve confidential computing secret area
> virt: Add sev_secret module to expose confidential computing secrets
>
> drivers/firmware/efi/Makefile | 2 +-
> drivers/firmware/efi/confidential-computing.c | 41 +++
> drivers/firmware/efi/efi.c | 5 +
> drivers/firmware/efi/libstub/Makefile | 3 +-
> .../efi/libstub/confidential-computing.c | 68 ++++
> drivers/firmware/efi/libstub/efi-stub.c | 2 +
> drivers/firmware/efi/libstub/efistub.h | 2 +
> drivers/firmware/efi/libstub/x86-stub.c | 2 +
> drivers/virt/Kconfig | 2 +
> drivers/virt/Makefile | 1 +
> drivers/virt/sev_secret/Kconfig | 11 +
> drivers/virt/sev_secret/Makefile | 2 +
> drivers/virt/sev_secret/sev_secret.c | 298 ++++++++++++++++++
> include/linux/efi.h | 11 +
> 14 files changed, 448 insertions(+), 2 deletions(-)
> create mode 100644 drivers/firmware/efi/confidential-computing.c
> create mode 100644 drivers/firmware/efi/libstub/confidential-computing.c
> create mode 100644 drivers/virt/sev_secret/Kconfig
> create mode 100644 drivers/virt/sev_secret/Makefile
> create mode 100644 drivers/virt/sev_secret/sev_secret.c

Those "confidential-computing.c" filenames are too long. I'd vote for
coco.c.

Same for your naming: efi_copy_confidential_computing_secret_area() -
that is a wow and doesn't look like kernel code to me. :)

Another example why it is too long:

+ {LINUX_EFI_CONFIDENTIAL_COMPUTING_SECRET_AREA_GUID,
+ &efi.confidential_computing_secret,
+ "ConfCompSecret"},

I'd do

{ LINUX_EFI_COCO_SECRET_AREA_GUID, &efi.coco_secret, "ConfCompSecret" },


--
Regards/Gruss,
Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg