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

Kai Huang
2022-07-05T12:14:46Z

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.

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.

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

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

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.

>
>
> static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> void __user *argp = (void __user *)arg;
> struct tdx_attest_req *areq = file->private_data;
> struct report_req *rep_data = NULL;
> struct tdx_report_req user_req;
> long ret = -EINVAL;
>
> switch (cmd) {
> case TDX_CMD_GET_REPORT:
> /* Allocate space for TDREPORT request */
> rep_data = kmalloc(sizeof(struct report_req), GFP_KERNEL);
> if (!rep_data)
> return -ENOMEM;
>
> /* Copy user request data */
> if (copy_from_user(&user_req, argp, sizeof(user_req))) {
> kfree(rep_data);
> return -EFAULT;
> }
>
> /* Copy user request details to areq->data */
> rep_data->subtype = user_req.subtype;
> areq->cmd = cmd;
> areq->data = rep_data;
>
> ret = 0;
> break;
> default:
> pr_debug("cmd %d not supported\n", cmd);
> break;
> }
>
> return ret;
> }
>
> static ssize_t tdx_attest_read(struct file *filp, char __user *buffer,
> size_t length, loff_t *offset)
> {
> struct tdx_attest_req *areq = filp->private_data;
> struct report_req *rep_data;
> void *tdreport;
> long ret;
>
> if (!areq)
> return -EINVAL;
>
> switch (areq->cmd) {
> case TDX_CMD_GET_REPORT:
>
> /* Check for valid length and offset */
> if (length != TDX_REPORT_LEN || offset != 0)
> return -EINVAL;
>
> rep_data = areq->data;
>
> /* Make sure we have valid reportdata */
> if (!rep_data->reportdata)
> return -EINVAL;
>
> /* Allocate space for output data */
> tdreport = kzalloc(length, GFP_KERNEL);
> if (!tdreport)
> return -ENOMEM;
> /*
> * 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(rep_data->reportdata), 0, 0, NULL);
> if (ret) {
> pr_debug("TDREPORT TDCALL failed, status:%lx\n", ret);
> kfree(tdreport);
> return -EIO;
> }
>
> /* Copy REPORTDATA from the user buffer */
> if (copy_to_user(buffer, tdreport, length)) {
> kfree(tdreport);
> return -EFAULT;
> }
>
> return length;
> default:
> pr_debug("cmd %d not supported\n", areq->cmd);
> break;
> }
>
> return 0;
> }
>
> static ssize_t tdx_attest_write(struct file *filp, const char __user *buffer,
> size_t length, loff_t *offset)
> {
> struct tdx_attest_req *areq = filp->private_data;
> struct report_req *rep_data;
>
> if (!areq)
> return -EINVAL;
>
> switch (areq->cmd) {
> case TDX_CMD_GET_REPORT:
>
> /* Check for valid length and offset */
> if (length != TDX_REPORTDATA_LEN || offset != 0)
> return -EINVAL;
>
> rep_data = areq->data;
>
> /* Allocate space to store REPORTDATA */
> rep_data->reportdata = kzalloc(length, GFP_KERNEL);
> if (!rep_data->reportdata)
> return -ENOMEM;
>
> /* Copy REPORTDATA from the user buffer */
> if (copy_from_user(rep_data->reportdata, buffer, length)) {
> kfree(rep_data->reportdata);
> rep_data->reportdata = NULL;
> return -EFAULT;
> }
>
> rep_data->reportdata_len = length;
>
> return length;
> default:
> pr_debug("cmd %d not supported\n", areq->cmd);
> break;
> }
>
> return 0;
> }
>
>
> static int tdx_attest_open(struct inode *inode, struct file *filp)
> {
> struct tdx_attest_req *areq;
>
> /* Allocate space to track attestation request */
> areq = kmalloc(sizeof(*areq), GFP_KERNEL);
> if (!areq)
> return -ENOMEM;
>
> filp->private_data = areq;
>
> return 0;
> }
>
> static int tdx_attest_release(struct inode *inode, struct file *filp)
> {
> kfree(filp->private_data);
> filp->private_data = NULL;
>
> return 0;
> }
>
> static const struct file_operations tdx_attest_fops = {
> .owner = THIS_MODULE,
> .open = tdx_attest_open,
> .read = tdx_attest_read,
> .write = tdx_attest_write,
> .unlocked_ioctl = tdx_attest_ioctl,
> .release = tdx_attest_release,
> .llseek = no_llseek,
> };
>
> > > > 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?
>
> I am not very clear about other possible use cases.
>
> Kirill/Kai/Isaku, Do you think we might need similar infrastructure for any
> other TDX Module calls or TDVMCALLs?
>
>

So far only attestation related TDCALL and TDVMCALL requires interaction to
userspace.

For attestation, conceptually, we need two "sets" of ABIs: 1) around getting the
TD report; 2) around getting the Quote.

For 1) we are discussing above.

For 2), currently we have only GetQuote TDVMCALL. It's very possible we will
need more sub-commands around Quote (not only get the Quote) -- logically, Quote
generation service defines Quote related commands anyway.

Theoretically, we only need one TDVMCALL (or a fixed set of TDVMCALLs) for
sending/receiving data as a communication channel (as an alternative to vsock,
etc) to support any Quote related sub-commands, but it seems we are not heading
that way.

--
Thanks,
-Kai