Re: [PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets

On Mon, 16 Aug 2021 at 10:57, Ard Biesheuvel [off-list ref] wrote:

On Fri, 13 Aug 2021 at 15:05, Andrew Scull [off-list ref] wrote:

quoted

On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote:

quoted

The new sev_secret module exposes the confidential computing (coco)
secret area via securityfs interface.

When the module is loaded (and securityfs is mounted, typically under
/sys/kernel/security), a "coco/sev_secret" directory is created in
securityfs.  In it, a file is created for each secret entry.  The name
of each such file is the GUID of the secret entry, and its content is
the secret data.

This allows applications running in a confidential computing setting to
read secrets provided by the guest owner via a secure secret injection
mechanism (such as AMD SEV's LAUNCH_SECRET command).

Removing (unlinking) files in the "coco/sev_secret" directory will zero
out the secret in memory, and remove the filesystem entry.  If the
module is removed and loaded again, that secret will not appear in the
filesystem.

We've also been looking into a similar secret mechanism recently in the
context of Android and protected KVM [1]. Our secrets would come from a
different source, likely described as a reserved-memory node in the DT,
but would need to be exposed to userspace in the same way as the SEV
secrets. Originally I tried using a character device, but this approach
with securityfs feels neater to me.

Agreed. I particularly like how deleting the file wipes the secret from memory.

quoted

We're also looking to pass secrets from the bootloader to Linux, outside
of any virtualization or confidential compute context (at least a far as
I have understood the meaning of the term). Again, this feels like it
would be exposed to userspace in the same way.

Indeed.

quoted

It would be good to be able to share the parts that would be common. I
expect that would mean the operations for a secret file and for a
directory of secrets at a minimum. But it might also influence the paths
in securityfs; I see, looking back, that the "coco" directory was added
since the RFC but would a generalized "secret" subsystem make sense? Or
would it be preferable for each case to define their own path?

I think we should avoid 'secret', to be honest. Even if protected KVM
is not riding the SEV/TDX wave, I think confidential computing is
still an accurate description of its semantics.

I agree that protected KVM fits with the ideas of confidential
computing. It was the non-virtualization context that I was less
certain about. For example, the Open Profile for DICE [2] starts with
a hardware secret and derives, at each boot stage, a secret that is
passed to the next stage. It's a process that applies both to a VM,
matching confidential compute as I understand it, but also the host
Linux, which is the part that I wasn't so clear on.

[2] -- https://pigweed.googlesource.com/open-dice/+/refs/heads/main/docs/specification.md

quoted

[1] -- https://lwn.net/Articles/836693/

quoted

+static int sev_secret_unlink(struct inode *dir, struct dentry *dentry)
+{
+     struct sev_secret *s = sev_secret_get();
+     struct inode *inode = d_inode(dentry);
+     struct secret_entry *e = (struct secret_entry *)inode->i_private;
+     int i;
+
+     if (e) {
+             /* Zero out the secret data */
+             memzero_explicit(e->data, secret_entry_data_len(e));

Would there be a benefit in flushing these zeros?

Do you mean cache clean+invalidate? Better to be precise here.

At least a clean, to have the zeros written back to memory from the
cache, in order to overwrite the secret.

quoted

quoted

+             e->guid = NULL_GUID;
+     }
+
+     inode->i_private = NULL;
+
+     for (i = 0; i < SEV_SECRET_NUM_FILES; i++)
+             if (s->fs_files[i] == dentry)
+                     s->fs_files[i] = NULL;
+
+     /*
+      * securityfs_remove tries to lock the directory's inode, but we reach
+      * the unlink callback when it's already locked
+      */
+     inode_unlock(dir);
+     securityfs_remove(dentry);
+     inode_lock(dir);
+
+     return 0;
+}