Message-ID: <ca73d2bd-5d40-d385-aeb0-8c04811690ff@linux.intel.com>
Date:   Thu, 30 Jun 2022 16:50:47 -0700
From:   Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
To:     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>,
        Kai Huang <kai.huang@...el.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

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

We might need similar files for GetQuote use case as well. IMO, such a design is 
more complicated than using IOCTL.

> 
> 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 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;
};


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?


-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Powered by blists - more mailing lists