Re: [PATCH 1/4] keys: Introduce tsm keys

[Peter Gonda]

On Mon, Jul 31, 2023 at 11:48 AM Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
>
> Peter Gonda wrote:
> > On Fri, Jul 28, 2023 at 1:31 PM Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> > >
> > > One of the common operations of a TSM (Trusted Security Module) is to
> > > provide a way for a TVM (confidential computing guest execution
> > > environment) to take a measurement of its run state and use that with a
> > > key-exchange protocol to establish a shared secret with a third-party /
> > > remote attestation agent. The concept is common across TSMs, but the
> > > implementations are unfortunately vendor specific. While the industry
> > > grapples with a common definition of this attestation format [1], Linux
> > > need not make this problem worse by defining a new ABI per TSM that
> > > wants to perform a similar operation. The current momentum has been to
> > > invent new ioctl-ABI per TSM per function which at best is an abdication
> > > of the kernel's responsibility to make common infrastructure concepts
> > > share common ABI.
> > >
> > > The proposal, targeted to conceptually work with TDX, SEV, COVE if not
> > > more, is to define a new key type that produces a TSM common blob format
> > > and moves the vendor specificity inside that envelope. The common Linux
> > > definition is:
> > >
> > >     "<hex encoded pubkey> <blob descriptor> <hex encoded attestation blob>"
> > >
> > > This approach later allows for the standardization of the attestation
> > > blob format without needing to change the Linux ABI. TSM specific
> > > options are encoded in the frontend request format where the options
> > > like SEV:vmpl (privilege level) can be specified and TSMs that do not
> > > support them can decide to ignore them or fail if they are specified.
> > > For now, "privlevel=" and "format=" are the only implemented options.
> > >
> > > Example of establishing a tsm key and dumping the provider-specific
> > > report:
> > >
> > >     dd if=/dev/urandom of=pubkey bs=1 count=64
> > >     keyctl add tsm tsm_test "auth $(xxd -p -c 0 < pubkey) privlevel=2" @u
> > >     keyctl print 280877394 | awk '{ print $3 }' | xxd -p -c 0 -r | hexdump -C
> >
> > What is the purpose of this report? What does it prove to whom? I'm a
> > bit confused because it doesn't seem like there is an ability for a
> > remote party to participate in a challenge and response to introduce
> > any freshness into this protocol.
> >
> > Also shouldn't the report have a little more context into the key we
> > are signing? For instance what type of public key is this? And what is
> > its purpose? In your example this isn't even a valid public key.
> >
> > >
> > > Now, this patch ends up being a fairly simple custom-key format because
> > > most of the follow-on work that happens after publishing a TSM-wrapped
> > > public-key is performed by userspace. The TSM key is just one step in
> > > establishing a shared secret that can be used to unlock other keys. For
> > > example a user-key could be populated with the resulting shared secret
> > > and that could be used as a master-key for an encrypted-key
> > > (security/keys/encrypted-keys/encrypted.c).
> > >
> > > While the discussion that led to this proposal hinted at a new
> > > trusted-key (security/keys/trusted-keys/trusted_core.c) type rooted in
> > > the TSM [2], more work is needed to fetch a secret from the TSM
> > > directly. The trusted-key core expects a pre-established secure channel
> > > to seal and unseal secrets locally. For that reason a "tsm" flavor
> > > trusted-key is saved for follow on work. That will likely starting as a
> > > wrapper around SNP_GET_DERIVED_KEY.
> > >
> > > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@xxxxxxxxxxxxxxxxxxxxxxxxx.notmuch [1]
> > > Link: http://lore.kernel.org/r/CAAH4kHYLETfPk-sMD-QSJd0fJ7Qnt04FBwFuEkpnehB5U7D_yw@xxxxxxxxxxxxxx [2]
> > > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> > > Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> > > Cc: David Howells <dhowells@xxxxxxxxxx>
> > > Cc: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> > > Cc: Dionna Amalie Glaze <dionnaglaze@xxxxxxxxxx>
> > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > Cc: Samuel Ortiz <sameo@xxxxxxxxxxxx>
> > > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > > ---
> > >  include/keys/tsm.h     |   71 ++++++++++++
> > >  security/keys/Kconfig  |   12 ++
> > >  security/keys/Makefile |    1
> > >  security/keys/tsm.c    |  282 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 366 insertions(+)
> > >  create mode 100644 include/keys/tsm.h
> > >  create mode 100644 security/keys/tsm.c
> > >
> > > diff --git a/include/keys/tsm.h b/include/keys/tsm.h
> > > new file mode 100644
> > > index 000000000000..61a81017d8f5
> > > --- /dev/null
> > > +++ b/include/keys/tsm.h
> > > @@ -0,0 +1,71 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef __TSM_H
> > > +#define __TSM_H
> > > +
> > > +#include <linux/types.h>
> > > +#include <linux/module.h>
> > > +
> > > +/*
> > > + * @TSM_DATA_MAX: a reasonable max with enough space for known attestation
> > > + * report formats. This mirrors the trusted/encrypted key blob max size.
> > > + */
> > > +#define TSM_DATA_MAX 32767
> > > +#define TSM_PUBKEY_MAX 64
> > > +#define TSM_FORMAT_MAX 16
> > > +
> > > +/**
> > > + * DOC: TSM Keys
> > > + *
> > > + * Trusted Security Module Keys are a common provider of blobs that
> > > + * facilitate key-exchange between a TVM (confidential computing guest)
> > > + * and an attestation service. A TSM key combines a user-defined blob
> >
> > Are we limited to only doing key-exchanges between guests and
> > attestation services? What if some user would like to handle the
> > attestation verification without a service?
>
> From the kernel perspective it does not matter, it is just marshalling
> the quote data. I assume local attestation could be built around this,
> but that's all user-space policy.

Makes sense. Can we tweak the language of these comments then?

>
> >
> > > + * (likely a public-key for a key-exchance protocol) with a signed
> >
> > key-exchange
>
> got it.
>
> >
> > > + * attestation report. That combined blob is then used to obtain
> > > + * secrets provided by an agent that can validate the attestation
> > > + * report.
> > > + *
> > > + * A full implementation uses a tsm key to, for example, establish a
> >
> > Should 'TSM' be capitalized everywhere? Or does it not matter?
>
> Probably should be.
>
> > > + * shared secret and then use that communication channel to instantiate
> > > + * other keys. The expectation is that the requester of the tsm key
> > > + * knows a priori the key-exchange protocol associated with the
> > > + * 'pubkey'.
> >
> > Can we instead be very specific about what protocols and cryptography
> > are being used?
>
> Again this is a contract to which the kernel is not a party. The
> requester knows the significance of the user-data, and it knows where to
> send the combined user-data plus quote to provision further secrets.
>
> Not that I like that arrangement, but the kernel is not enabled by these
> TSM implementations to know much more than "user-data in", "report out".

Can you explain why using this key API is better than the ioctl
version? Is there an overhead to adding keys? You could imagine some
userspace application that receives RPCs and does some attestation for
each one, would adding then deleting a huge number of keys present any
issues?

>
> >
> > > + *
> > > + * The attestation report format is TSM provider specific, when / if a
> >
> > I'm confused about the TSM terminology and what a TSM provider is. Is
> > TSM the confidential compute framework of the vendor? So for Intel
> > this is TDX, and the TSM provider is the SEAM module?
>
> Yes, I borrowed this term from the TDISP specification where the "Trusted
> Security Module" is the overarching term for the confidential compute
> infrastructure for the vendor. Yes, TDX + SEAM is a TSM and SEV-SNP +
> PSP is a TSM.

Thanks for the explanation.

>
> >
> > > + * standard materializes it is only a change to the auth_blob_desc
> > > + * member of 'struct tsm_key_payload', to convey that common format.
> > > + */
> > > +
> > > +/**
> > > + * struct tsm_key_payload - generic payload for vendor TSM blobs
> > > + * @privlevel: optional privilege level to associate with @pubkey
> > > + * @pubkey_len: how much of @pubkey is valid
> > > + * @pubkey: the public key-exchange blob to include in the attestation report
> > > + * @auth_blob_desc: base ascii descriptor of @auth_blob
> > > + * @auth_blob_format: for TSMs with multiple formats, extend @auth_blob_desc
> > > + * @auth_blob_len: TSM provider length of the array it publishes in @auth_blob
> > > + * @auth_blob: TSM specific attestation report blob
> > > + */
> > > +struct tsm_key_payload {
> > > +       int privlevel;
> > > +       size_t pubkey_len;
> > > +       u8 pubkey[TSM_PUBKEY_MAX];
> > > +       const char *auth_blob_desc;
> > > +       char auth_blob_format[TSM_FORMAT_MAX];
> > > +       size_t auth_blob_len;
> > > +       u8 *auth_blob;
> > > +};
> >
> > How is freshness incorporated into the key exchange protocol? Wouldn't
> > we need to do a challenge response between each remote party that we
> > need to attest the provenance of @pubkey too?
>
> That's left to userspace.

But you haven't allowed userspace to add any data into the quote other
than just the raw public key.

The current sevguest ioctl allows users to pass arbitrary userdata.
This would allow for some nonce to be included.

At a highlevel I'm not sure why this is better than each vendor having
their own driver. It doesn't seem that difficult for userspace to deal
with these systems given userspace will need to be written carefully
for these PKI protocols anyways.


>
> >
> > > + *                request, if the platform TSM supports that concept. To
> > > + *                date only SEV accepts this option. Default 0.
> >
> > SEV-SNP or just SNP? Plain SEV or SEV-ES doesn't actually support this
> > interface at all.
>
> Yeah, I was using "SEV" as a shorthand for the AMD "TSM". So maybe just
> "SNP" is appropriate?

I'm not sure what AMD prefers. I'll let them chime in.