Message-ID: <b8eadd3079101a2cf93ee87d36dbedf93d8a2725.camel@intel.com>
Date:   Tue, 03 May 2022 10:30:57 +1200
From:   Kai Huang <kai.huang@...el.com>
To:     Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org
Cc:     "H . Peter Anvin" <hpa@...or.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Tony Luck <tony.luck@...el.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Wander Lairson Costa <wander@...hat.com>,
        Isaku Yamahata <isaku.yamahata@...il.com>,
        marcelo.cerri@...onical.com, tim.gardner@...onical.com,
        khalid.elmously@...onical.com, philip.cox@...onical.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/3] x86/tdx: Add TDX Guest attestation interface
 driver


> 
> Also note that the MAC added to the TDREPORT is bound to the platform.
> So TDREPORT can only be verified locally. Intel SGX Quote Enclave (QE)
> can be leveraged to verify the TDREPORT locally and convert it to a
> remote verifiable Quote to support remote attestation of the TDREPORT.

Why "can be"?  TDX must use QE to generate the Quote.

[...]

> 
> > 
> > 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.
> > 
> > Anyway, my 2cents above.
> 
> IMO, since TDREPORT is not a secret we don't have to hightlight security
> concern here. 
> 

Right the TDREPORT itself isn't a secret.  However my thinking is the REPORTDATA
might be.  It's typically provided by the attestation service to the TD so the
Quote can be verified for instance per-session or per-connect or whatever.  The
REPORTDATA is the only thing that can be used to prevent reply attack anyway. 
>From this perspective, it is kinda secret.  However the TDREPORT can be captured
by untrusted software and used for reply attack if no crypto-protection is used
when it is sent to the QE, so I am not sure how bad can the reply attack cause.

> How about following?
> 
> Operations like getting TDREPORT or Quote generation involves sending
> a blob of data as input and getting another blob of data as output. It
> was considered to use a sysfs interface for this, but it doesn't fit
> well into the standard sysfs model for configuring values. It would be
> possible to do read/write on files, but it would need multiple file
> descriptors, which would be somewhat messy. IOCTLs seems to be the best
> fitting and simplest model here.
> 
> 

Let's forget about GetQuote now.  As you can see it has couple of problems.  

If we don't argue from security perspective, what's wrong with the approach
using /sysfs I mentioned above?

[...]

> > > +
> > > +	/*
> > > +	 * Generate TDREPORT using "TDG.MR.REPORT" TDCALL.
> > > +	 *
> > > +	 * Pass the physical address of user generated REPORTDATA
> > > +	 * and the physical address of the output buffer to the TDX
> > > +	 * module to generate the TDREPORT. Generated data contains
> > > +	 * measurements/configuration data of the TD guest. More info
> > > +	 * about ABI can be found in TDX 1.0 Module specification, sec
> > > +	 * titled "TDG.MR.REPORT".
> > 
> > I guess you can get rid of the entire second paragraph.  If the reference to the
> > spec is useful, then keep it but other sentences are not quite useful.  Perhaps:
> > 
> > 	Get the TDREPORT using REPORTDATA as input.  Refer to 22.3.3
> > 	TDG.MR.REPORT leaf in the TDX Module 1.0 Specification for detail
> > 	information.
> 
> How about following?
> 
> Pass REPORTDATA as input and generate TDREPORT using "TDG.MR.REPORT"
> TDCALL. Refer to 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0
> Specification for detailed information.

No problem.

> 
> > 
> > > +	 */
> > > +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
> > > +				virt_to_phys(reportdata), 0, 0, NULL);
> > > +	if (ret) {
> > > +		pr_debug("TDREPORT TDCALL failed, status:%lx\n",
> > > +				TDCALL_STATUS_CODE(ret));
> > 
> > You can just print out the exact error code.  It's more informative and can help
> > to debug.
> 
> As per spec, only upper 32 bits has status code. 0:32 does not have any
> useful info.

Bits 0:31 are also defined by TDX error codes.  For instance, it also prints
which argument caused this error in case of OPERAND_INVALID.  Why is it not
useful?

[...]

> > > +	ret = misc_register(&miscdev);
> > > +	if (ret) {
> > > +		pr_err("misc device registration failed\n");
> > 
> > pr_debug() is used when __tdx_module_call() fails, and in the default case in
> > tdx_attest_ioctl() too.
> > 
> > Shouldn't those error msg be printed using the same way?
> 
> For IOCTL case, I expect userspace to print the error. But for init
> code error, it needs to be handled by kernel. So I have used pr_err
> here.

I don't quite get.  Why "userspace will print the error or not" has anything to
do with using pr_debug() vs pr_err() here?

[...]

> 
> > > +
> > > +/**
> > > + * struct tdx_report_req: Get TDREPORT from the TDX module.
> > 
> > Just get the TDREPORT is enough I guess.
> 
> Get TDREPORT using REPORTDATA as input?

No problem.

> 
> > 
> > > + *
> > > + * @reportdata : User-defined 64-Byte REPORTDATA to be included into
> > > + *		 TDREPORT. Typically it can be some nonce provided by
> > > + *		 attestation software so the generated TDREPORT can be
> > > + *		 uniquely verified.
> > > + * @tdreport   : TDREPORT output from TDCALL[TDG.MR.REPORT] of size
> > > + *		 TDX_REPORT_LEN.
> > > + *
> > > + * Used in TDX_CMD_GET_REPORT IOCTL request.
> > > + */
> > > +struct tdx_report_req {
> > > +	union {
> > > +		__u8 reportdata[TDX_REPORTDATA_LEN];
> > > +		__u8 tdreport[TDX_REPORT_LEN];
> > > +	};
> > > +};
> > 
> > I am not sure overriding the input is a good idea, but will leave to others.
> 
> TDCALL uses it that way. So I have followed the same model.

Which TDCALL?

And TDCALL is kernel internal implementation, but we are talking about userspace
ABI here.  I don't see any connection between them.

> 
> > 
> > > +
> > > +/* Get TDREPORT from the TDX module using TDCALL[TDG.MR.REPORT) */
> > 
> > Just get the TDREPORT is enough I guess.
> 
> May be following?
> 
> Get TDREPORT using TDCALL[TDG.MR.REPORT)

My thinking is you don't need to call out the exact TDCALL in the uapi header. 
But no opinion here.  Will leave to maintainers.

> 
> > 
> > > +#define TDX_CMD_GET_REPORT		_IOWR('T', 0x01, struct tdx_report_req)
> > > +
> > > +#endif /* _UAPI_ASM_X86_TDX_H */
> > 
> 


-- 
Thanks,
-Kai

Powered by blists - more mailing lists