Re: [PATCH v8 1/5] x86/tdx: Add TDX Guest attestation interface driver

[Dave Hansen]

On 6/27/22 07:50, Sathyanarayanan Kuppuswamy wrote:
>> On high-level comment: this whole series is quite agnostic about what is
>> actually attested.  On some level, I guess it doesn't matter.  But, it
>> makes me a bit uneasy.  There needs to be at least *some* kind of claim
>> about that somewhere, preferably a sentence in the cover letter and
>> sentence or two here.
> 
> We are attesting that the given TD Guest is secure and trustworthy, and
> the remote server requesting attestation can safely transfer the secrets.

What is *actually* attested?

> Are you looking for something like the above?

No, I'd like some actual facts, please.  I want key examples of things
visible to an end user that might change that might determine if a guest
is "secure and trustworthy".

Does a kernel command-line parameter change attestation?  How about the
disk image?  The kernel image?  initrd?  Memory *size*?  Devices?

Compare this mechanism to something that exists outside the TDX world.

>> Second, how can someone test this code?  It appears that they need to
>> assemble a veritable Rube Goldberg machine.  The least we could do is
>> have a selftest that just calls the ioctl() and makes sure that
>> something halfway sane comes out of it.
> 
> My initial submission included a test app. But I removed it later to
> reduce the review load. I thought to submit the test app once feature
> support patches are merged.
> 
> https://lore.kernel.org/all/9247fade9db5ae6eb183b2f92fdedb898282376a.1648664666.git.sathyanarayanan.kuppuswamy@xxxxxxxxx/
> 
> If you prefer, I can add it back to the next submission with the latest changes.

I doubt anyone will ever run a "test app".  Why not just make this a
selftest?

>>> In such
>>> case, since REPORTDATA is a secret, using sysfs to share it is insecure
>>> compared to sending it via IOCTL.
>>
>> Huh?  How is sysfs "insecure"?
> 
> REPORTDATA (input) we pass to the Module call might come from attestation
> service as a per session unique ID.  If it is shared via sysfs, there is
> a possibility for untrusted software to read it and trigger some form of
> reply attack. So in this context, sysfs is insecure compared to IOCTL
> approach. You can find the related discussion in,
> 
> https://lore.kernel.org/lkml/b8eadd3079101a2cf93ee87d36dbedf93d8a2725.camel@xxxxxxxxx/

I still don't get it.

How is a 400 sysfs file "insecure"?  This sounds "if the filesystem
permissions are broken" paranoia.  In other words, you're protecting
against a malicious root user.

In other words, I don't think the ABI here has been well thought out.

Also, this is basically a:

	Inputs:

		* nonce
		* report type

	Outputs:

		* report

I have to wonder how hard it would be to have this be:

	fd = open("/dev/foo");
	ioctl(fd, REPORT, type, flags??);
	write(fd, &inputs, inputs_len);
	read(fd,  &output, outputs_len);



>>> +	/*
>>> +	 * Generate TDREPORT using "TDG.MR.REPORT" TDCALL.
>>> +	 *
>>> +	 * Get the TDREPORT using REPORTDATA as input. Refer to
>>> +	 * section 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0
>>> +	 * Specification for detailed information.
>>> +	 */
>>> +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
>>> +				virt_to_phys(reportdata), 0, 0, NULL);
>>
>> One of those 0's is a "Report sub type".  Those bits in the module call
>> are presumably because it won't *always* be zero.  But, if there's ever
>> a new report type, we'll need another user/kernel ABI.  That doesn't
>> seem great.
>>
>> The TDX module spec doesn't even give a name to "sub report type 0".
>> That is not super helpful for deciding what it *means*.
>>
>> Right now, the entire ABI is basically:
>>
>> 	ret = ioctl(TDX_GET_REPORT, &buffer);
>>
>> That _implies_ a ton of stuff:
>>
>> 	1. report sub type 0
>> 	2. a specific input length REPORTDATA
>> 	3. a specific output length
>>
>> I don't want to over-engineer this thing, but it would make a lot of
>> sense to me to feed the ioctl() with something like this:
>>
>> struct tdx_report
>> {
>> 	u8 report_sub_type;
>> 	u64 report_input_data;
>> 	u64 report_input_data_len;
>>
>> 	u64 report_output_data;
>> 	u64 report_output_data_len;
>> }
>>
>> That makes *everything* explicit.  It makes it utterly clear what goes
>> where, *AND* it makes userspace declare it.
> 
> Ok. Parameter to handle subtype makes sense. But regarding the input and
> output length, currently there is no indication in specification that it
> will change and there is no way to pass it in the module call. So do you
> still prefer to add parameters for it?

It seems like a more flexible ABI to me.  What you have here seems to be
a *PURE* passthrough of the module ABI.

>> But, you have:
>>
>>> +/**
>>> + * struct tdx_report_req: Get TDREPORT using REPORTDATA as input.
>>> + *
>>> + * @reportdata : User-defined 64-Byte REPORTDATA to be included into
>>> + *		 TDREPORT. Typically it can be some nonce provided by
>>> + *		 attestation service, 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];
>>> +	};
>>> +};
>>> +
>>
>> and a bunch of code copying in and out of this structure:
>>
>>> +static long tdx_get_report(void __user *argp)
>>> +{
>>> +	void *reportdata = NULL, *tdreport = NULL;
>> ...
>>> +	/* Copy REPORTDATA from the user buffer */
>>> +	if (copy_from_user(reportdata, argp, TDX_REPORTDATA_LEN)) {
>>> +		ret = -EFAULT;
>>> +		goto out;
>>> +	}
>>
>> But none of that code even bothers to *use* the structure!
> 
> I have created that struct mainly to document the ABI details for the
> userspace clarity (like input and output). Since the length of the input
> and output is defined as macro, it worked fine for driver implementation.
> I am fine with modifying the code to use it. Please let me know if you
> prefer it.

Let's say I'm reviewing kernel code.  I want to know what the ABI of
'reportdata' is.  How do I find it?

Now, imagine that the code is:


	struct tdx_report_req __user *user_rd = argp;
	struct tdx_report_req *kern_rd;
	...

	if (copy_from_user(&kern_rd->reportdata,
			   &user_rd->reportdata,
			   sizeof(kern_rd->reportdata)) {


This tells me a *LOT*.  It says that the user and kernel buffers are the
same ABI, and I'm also much more that the copy matches the data
structure sizes.

We don't put structures in kernel headers just for our health.  We put
them so the kernel can use them.

>> What's with the union?  Are you really just trying to save 64 bytes of
>> space?  Is that worth it?
> 
> Makes sense. I will change it to separate variables.
> 
>> How many of these "drivers" are we going to need which are thinly veiled
>> ioctl()s that are only TDX module call wrappers?

This is actually a really big question.  This code is obviously just
trying to do one thing very narrowly and not thinking about the future
at all.  Can we please consider what the future will be like and try to
*architect* this instead of having the kernel just play a glorified game
of telephone?