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

[Dan Williams <dan.j.williams@intel.com>]

Huang, Kai wrote:
> 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@linux.intel.com/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.

James and Dionna answered the nonce question. The kernel could enforce
"nonce || pubkey" where only pubkey is user provided. It's a
contract that the kernel need not enforce, but maybe it should.

As for sysfs and multiple requesters it is indeed awkward especially
with the suggestion that this is not a configure once and done after
establishing a channel with the attestation agent. That said the kernel
gets to pick which use cases it wants to maintain. Lets compare Keys and
sysfs side-by-side with actual code.