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

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

> > > 
> > > > 
> > > > 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?
> 
> Sysfs is generally used for configuring values. As I have mentioned, it
> does not fit well for our use case where we want to send blob of data
> as input and get another blob as an output. But, if you really want to
> use it, you can implement it like you have mentioned (with multiple
> files). But I don't see any advantage in doing it.

I am not so sure about this argument.  Will leave this to maintainers.

My point is to me from security's perspective, IOCTL fits better.  And you can
add this to your changelog.

> 
> Also, in the future, if you want to support multiple requests in
> parallel, sysfs model will not work.
> 
> 
> > > 

I think you are mixing getting TDREPORT and GetQuote.  What's the point of
supporting get TDREPORT in parallel?  You can only get one TDREPORT from TDX
module at one time.

> > > > 
> > > > > + *
> > > > > + * @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?
> 
> TDG.MR.REPORT. It uses the same buffer as input and output.
> 
> 
It doesn't override the REPORTDATA:

Input:

RCX:	1024B-aligned guest physical address of newly created report structure
RDX:	64B-aligned guest physical address of additional data to be signed

As you can see the buffer to hold TDREPORT and REPORTDATA are not the same.

Anyway, we are talking about userspace ABI.  I don't see why this is related
here.

-- 
Thanks,
-Kai