Message-ID: <CAGxU2F4EG_uAcSbdjroW=sUo5yu6zQw8yJ4R_b+zxX7Sk-bDZg@mail.gmail.com>
Date: Wed, 11 Dec 2024 17:55:49 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Tom Lendacky <thomas.lendacky@....com>
Cc: linux-coco@...ts.linux.dev, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, Peter Huewe <peterhuewe@....de>, 
	"H. Peter Anvin" <hpa@...or.com>, linux-integrity@...r.kernel.org, 
	James Bottomley <James.Bottomley@...senpartnership.com>, x86@...nel.org, 
	Joerg Roedel <jroedel@...e.de>, Jason Gunthorpe <jgg@...pe.ca>, Jarkko Sakkinen <jarkko@...nel.org>, 
	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>, 
	Thomas Gleixner <tglx@...utronix.de>, Claudio Carvalho <cclaudio@...ux.ibm.com>, 
	Dov Murik <dovmurik@...ux.ibm.com>
Subject: Re: [PATCH 3/3] x86/sev: add a SVSM vTPM platform device

On Wed, Dec 11, 2024 at 5:31 PM Tom Lendacky <thomas.lendacky@....com> wrote:
>
> On 12/10/24 08:34, Stefano Garzarella wrote:
> > From: James Bottomley <James.Bottomley@...senPartnership.com>
> >
> > If the SNP boot has a SVSM, probe for the vTPM device by sending a
> > SVSM_VTPM_QUERY call (function 8). The SVSM will return a bitmap with
> > the TPM_SEND_COMMAND bit set only if the vTPM is present and it is able
> > to handle TPM commands at runtime.
> >
> > If a vTPM is found, register a platform device as "platform:tpm" so it
> > can be attached to the tpm_platform.c driver.
> >
> > Signed-off-by: James Bottomley <James.Bottomley@...senPartnership.com>
> > [CC] Used SVSM_VTPM_QUERY to probe the TPM
> > Signed-off-by: Claudio Carvalho <cclaudio@...ux.ibm.com>
> > [SG] Code adjusted with some changes introduced in 6.11
> > [SG] Used macro for SVSM_VTPM_CALL
> > Signed-off-by: Stefano Garzarella <sgarzare@...hat.com>
> > ---
> >  arch/x86/coco/sev/core.c | 64 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> > index c5b0148b8c0a..ec0153fddc9e 100644
> > --- a/arch/x86/coco/sev/core.c
> > +++ b/arch/x86/coco/sev/core.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/cpumask.h>
> >  #include <linux/efi.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/tpm_platform.h>
> >  #include <linux/io.h>
> >  #include <linux/psp-sev.h>
> >  #include <linux/dmi.h>
> > @@ -2578,6 +2579,51 @@ static struct platform_device sev_guest_device = {
> >       .id             = -1,
> >  };
> >
> > +static struct platform_device tpm_device = {
> > +     .name           = "tpm",
> > +     .id             = -1,
> > +};
> > +
> > +static int snp_issue_svsm_vtpm_send_command(u8 *buffer)
> > +{
> > +     struct svsm_call call = {};
> > +
> > +     call.caa = svsm_get_caa();
> > +     call.rax = SVSM_VTPM_CALL(SVSM_VTPM_CMD);
> > +     call.rcx = __pa(buffer);
> > +
> > +     return svsm_perform_call_protocol(&call);
> > +}
> > +
> > +static bool is_svsm_vtpm_send_command_supported(void)
> > +{
> > +     struct svsm_call call = {};
> > +     u64 send_cmd_mask = 0;
> > +     u64 platform_cmds;
> > +     u64 features;
> > +     int ret;
> > +
> > +     call.caa = svsm_get_caa();
> > +     call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
> > +
> > +     ret = svsm_perform_call_protocol(&call);
> > +
> > +     if (ret != SVSM_SUCCESS)
> > +             return false;
> > +
> > +     features = call.rdx_out;
> > +     platform_cmds = call.rcx_out;
> > +
> > +     /* No feature supported, it must be zero */
> > +     if (features)
> > +             return false;
>
> I think this check should be removed. The SVSM currently returns all
> zeroes for the features to allow for future support. If a new feature is
> added in the future, this then allows a driver that supports that
> feature to operate with a version of an SVSM that doesn't have that
> feature implemented. It also allows a version of the driver that doesn't
> know about that feature to work with an SVSM that has that feature.

I couldn't find much in the specification, but is a feature considered
additive only?

Let me explain, since there's no negotiation, the driver can't disable
features, so if these are just additive, it's perfectly fine to remove
this check, but if these can change the behavior of the device, then
it's risky.

I'll give an example, let's say a future version of TCG TPM changes
the format of requests for whatever reason, I guess in that case we
could use a feature to tell the driver to use the new format. What
happens if the driver is old and doesn't support it?

Maybe in this case we can define a new supported command, so if we are
sure that the features are just additive, we can remove this check.

>
> A feature added to the vTPM shouldn't alter the behavior of something
> that isn't using or understands that feature.

Okay, so this confirms that features are only additive.
BTW it wasn't perfectly clear from the specification, so if it can be
added it would be better IMHO.

>
> > +
> > +     /* TPM_SEND_COMMAND - platform command 8 */
> > +     send_cmd_mask = 1 << 8;
> > +
> > +     return (platform_cmds & send_cmd_mask) == send_cmd_mask;
> > +}
> > +
> >  static int __init snp_init_platform_device(void)
> >  {
> >       struct sev_guest_platform_data data;
> > @@ -2593,6 +2639,24 @@ static int __init snp_init_platform_device(void)
> >               return -ENODEV;
> >
> >       pr_info("SNP guest platform device initialized.\n");
> > +
> > +     /*
> > +      * The VTPM device is available only if we have a SVSM and
> > +      * its VTPM supports the TPM_SEND_COMMAND platform command
>
> s/VTPM/vTPM/g

I'll fix it!

Thanks for the review,
Stefano

Powered by blists - more mailing lists