Messages in this thread |  | | Subject | Re: [PATCH v8 1/5] x86/tdx: Add TDX Guest attestation interface driver | From | Kai Huang <> | Date | Wed, 06 Jul 2022 00:07:22 +1200 |
| |
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
|  |