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

Sathyanarayanan Kuppuswamy
2022-07-05T18:45:16Z

Hi,

On 7/5/22 5:07 AM, Kai Huang wrote:
> On Thu, 2022-06-30 at 16:50 -0700, Sathyanarayanan Kuppuswamy wrote:
>> Hi,
>>
>> On 6/27/22 10:24 AM, Dave Hansen wrote:
>>> On 6/27/22 07:50, Sathyanarayanan Kuppuswamy wrote:
>>
>>>>> 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@intel.com/
>>>>
>>>> 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?
>>
>> Fine with me. I can change it into 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@intel.com/
>>>
>>> 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.
>>
>> AFAIK, root is not the only user of the attestation interface. General users can
>> also use it (For example, in a use case like pytorch, attestation may be requested
>> by server before passing the training data). So if the permission is not "root
>> only", then other users or application in TD can access the sysfs file to read
>> its contents.
>>
>> Also, the security concern mentioned is just an additional point. Following are
>> the main reasons for choosing IOCTL over sysfs.
>>
>> 1. Sysfs is generally used for config purposes. Not for request/response kind of
>> use case (Attestation falls under this category).
>> 2. If we only use sysfs model for GetReport, the design might look like below:
>>
>> /sys/kernel/../report/input
>> /sys/kernel/../report/subtype
>> /sys/kernel/../report/input_len
>> /sys/kernel/../report/output
>> /sys/kernel/../report/output_len
>> /sys/kernel/../report/status
>> /sys/kernel/../report/trigger
>
> I don't think you need all those if using /sysfs approach. You only need
> 'reportdata' and 'tdreport' to start with (see below my old reply).
>
> 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'.
>
> You can add more (such as subtype) in the future if needed (and I doubt it will
> ever happen) but this doesn't break any existing ABI. 'output/output_len' also
> not needed, kernel can return the report with right size.

I have included *_len files for readability. You can get away with it. But I think
you still need a status file to understand whether TDCALL is successful or not.
Another way is, you can try to write -1 or something to tdreport file if failed.

Yes, there are many ways to design this. But I still think IOCTL is a better fit
for the request/response kind of use case.

I will let the maintainer decide whether you want to take this route.

>
> Btw, although the /sysfs may not be as secure as IOCTL -- as you mentioned
> above, other programs with the same permission can get the TD report by reading
> /sysfs and use it as a "reply attack" -- but I am not sure whether such
> "potential reply attack" is a true issue or not. For instance, typically the
> attestation service should already have established a secure TLS connection with
> TD attestation agent before it provides the 'nonce' (reportdata), and the
> attestation should reject the TD report from other connection, etc.

As I have mentioned, security issue is just an additional point. If this is not
very valid or real, I can remove it.

>
>>
>> We might need similar files for GetQuote use case as well. IMO, such a design is
>> more complicated than using IOCTL.
>
> Using /sysfs for TD report doesn't necessarily mean you must use /sysfs for
> Quote. I don't think we should mixing them up. For instance, even if we use
> /dev/xxx for getting TD report, we can use a separate device node for getting
> the Quote:
>
> /dev/tdreport
> /dev/tdquote
>
> I believe there should be pros/cons comparing to using single /dev/attest, but I
> haven't thought this very carefully.

Yes. There are pros and cons for different approaches. But I personally think
using one device and IOCTL ABI model is a simpler design for this use case. AMD also
uses a similar model for their attestation use case.

>
>
>>>
>>> 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);
>
> It looks like the kernel and userspace still need data structures to agree on
> the input/output data format. I guess with this approach, we can start with
> what we need now, and if we need more in the future, we define new data
> structures for input and output?
>
>>>
>>
>> It is not hard to implement it this way. But I don't see it being
>> very different from just using IOCTL. config/{read/write} model is
>> usually preferred for applications in which you have a requirement to do
>> multiple read/writes after one time config (use cases like serial
>> port read, printer write or reading temperature, etc). But in our case
>> we will mostly use it once after every config.
>>
>> Also, splitting input over IOCTL and write system call will require us
>> to add additional code to store the state.
>>
>> I have attempted a sample implementation (untested) for reference. But I
>> feel our current code is simpler. It handles allocation and input/output
>> validation in one place compared to spreading it across multiple handlers.
>>
>> struct report_req {
>> int subtype;
>> void *reportdata;
>> int reportdata_len;
>> };
>>
>> struct tdx_attest_req {
>> unsigned int cmd;
>> void *data;
>> };
>
> Is it supposed to be used for Quote too?

Yes. It is to save the command request in kernel during IOCTL request.

>
> I dislike the idea of mixing up getting TD report and getting Quote (make TD
> report and Quote both as a sub-commands, etc).
>
> As we have adequately put, the new IOCTLs to support getting Quote isn't even
> mandatory -- we just need some communication channel between TD attestation
> agent and the QE, such as vsock.

Yes. There are other ways to get the Quote. But we have a requirement to support
TDVMCALL based method.

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer