On Mon, 16 Aug 2021 at 10:57, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote: > > On Fri, 13 Aug 2021 at 15:05, Andrew Scull <ascull@xxxxxxxxxx> wrote: > > > > On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote: > > > 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. > > > 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. > > > 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 > > [1] -- https://lwn.net/Articles/836693/ > > > > > +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. > > > > + 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; > > > +}
