Re: [PATCH v5 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Kai Huang <kai.huang@intel.com>]

> 
> 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