Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

[Huang, Kai]

On Sun, 2023-07-30 at 08:59 -0400, James Bottomley wrote:
> On Sat, 2023-07-29 at 21:56 -0700, Dan Williams wrote:
> > James Bottomley wrote:
> > > On Fri, 2023-07-28 at 12:30 -0700, Dan Williams wrote:
> > > > The bulk of the justification for this patch kit is in "[PATCH
> > > > 1/4] keys: Introduce tsm keys". The short summary is that the
> > > > current approach of adding new char devs and new ioctls, for what
> > > > amounts to the same functionality with minor formatting
> > > > differences across vendors, is untenable. Common concepts and the
> > > > community benefit from common infrastructure.
> > > 
> > > I agree with this, but ...
> > > 
> > > > Use Keys to build common infrastructure for confidential
> > > > computing attestation report blobs, convert sevguest to use it
> > > > (leaving the deprecation question alone for now), and pave the
> > > > way for tdx-guest and the eventual risc-v equivalent to use it in
> > > > lieu of new ioctls.
> > > > 
> > > > The sevguest conversion is only compile-tested.
> > > > 
> > > > This submission is To:David since he needs to sign-off on the
> > > > idea of a new Keys type, the rest is up to the confidential-
> > > > computing driver maintainers to adopt.
> > > 
> > > So why is this a keys subsystem thing?  The keys in question cannot
> > > be used to do any key operations.  It looks like a transport layer
> > > for attestation reports rather than anything key like.
> > 
> > Yes, it has ended up as just a transport layer.
> > 
> > > To give an analogy with the TPM: We do have a TPM interface to keys
> > > because it can be used for things like sealing (TPM stores a
> > > symmetric key) and even asymmetric operations (although TPM key
> > > support for that in 1.2 was just removed).  However, in direct
> > > analogy with confidential computing: the TPM does have an
> > > attestation interface: TPM2_Quote and TPM2_Certify (among others)
> > > which is deliberately *not* wired in to the keys subsystem because
> > > the outputs are intended for external verifiers.
> > > 
> > > If the goal is to unify the interface for transporting attestation
> > > reports, why not pull the attestation ioctls out of sevguest into
> > > something common?
> > 
> > That's fair. I originally started out with a draft trusted-keys
> > implementation, but abandoned it because that really wants a vTPM
> > backend. There is no kernel consumer for attestation reports like
> > other key blobs, so that leaves either a key-type that is just a
> > transport layer or a new ABI.
> >  
> > I have a personal distaste for ioctls and the presence of user-
> > defined blobs in the Keyring subsystem made me think "why not just
> > have a key-type to convey the per-TSM attestation reports". Is that a
> > fair observation?
> 
> The trouble with this argument is that it's an argument for every new
> ioctl becoming a key type.  We have a ton of interfaces for
> transporting information across the kernel to user boundary: sysfs,
> filesystem, configfs, debugfs, etc ... although to be fair the
> fashionably acceptable one does seem to change each year.  Since
> there's nothing really transactional about this, what about a simple
> sysfs one?  You echo in the nonce to a binary attribute and cat the
> report.  Any additional stuff, like the cert chain, can appear as
> additional attributes?
> 

Sorry perhaps a dumb question to ask:

As it has been adequately put, the remote verifiable report normally contains a
nonce.  For instance, it can be a per-session or per-request nonce from the
remote verification service to the confidential VM.  

IIUC, exposing attestation report via /sysfs means many processes (in the
confidential VM) can potentially see the report and the nonce.  My question is
whether such nonce should be considered as a secret thus should be only visible
to the process which is responsible for talking to the remote verification
service?  Using IOCTL seems can avoid such exposure.

Probably exposing nonce is fine, but I don't know.

In fact, I raised whether we should use /sysfs to get TDX's TDREPORT (which can
only be verified on local machine, thus needs to be singed as a Quote by the SGX
Quoting Enclave) when we were upstreaming (the first part of) TDX attestation:

https://lore.kernel.org/lkml/20220501183500.2242828-1-sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx/T/#m18fd5167dfa32c4702dd6b4bd472ad9e8f579ad8

Quote the relevant part here:

> 
> Implement a basic attestation driver to allow TD userspace to get the
> TDREPORT, which is sent to QE by the attestation software to generate
> a Quote for remote verification.
> 
> Also note that explicit access permissions are not enforced in this
> driver because the quote and measurements are not a secret. However
> the access permissions of the device node can be used to set any
> desired access policy. The udev default is usually root access
> only.

The IOCTL vs /sysfs isn't discussed.

For instance, after rough thinking, why is the IOCTL better than below approach
using /sysfs?

echo <REPORTDATA> > /sys/kernel/coco/tdx/attest/reportdata
cat /sys/kernel/coco/tdx/attest/tdreport

Each "echo <REPORTDATA>" to '/sys/.../reportdata' triggers the driver to call
TDCALL to get the TDREPORT, which is available at '/sys/.../tdreport'.

The benefit of using IOCTL I can think of now is it is perhaps more secure, as
with IOCTL the REPORTDATA and the TDREPORT is visible to the process which calls
the IOCTL, while using the /sysfs they are potentially visible to any process. 
Especially the REPORTDATA, i.e. it can come from attestation service after the
TD attestation agent sets up a secure connection with it.