Message-ID: <331abea18e728061979301772a9d0d61543f59fb.camel@intel.com>
Date:   Wed, 06 Jul 2022 00:07:22 +1200
From:   Kai Huang <kai.huang@...el.com>
To:     Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org
Cc:     "H . Peter Anvin" <hpa@...or.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Tony Luck <tony.luck@...el.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Wander Lairson Costa <wander@...hat.com>,
        Isaku Yamahata <isaku.yamahata@...il.com>,
        marcelo.cerri@...onical.com, tim.gardner@...onical.com,
        khalid.elmously@...onical.com, philip.cox@...onical.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 1/5] x86/tdx: Add TDX Guest attestation interface
 driver

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

Powered by blists - more mailing lists