linux-kernel - Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <24d8fd58-36c1-0e89-4142-28f29e2c434b@linux.intel.com>
Date:   Thu, 8 Jul 2021 16:57:24 -0700
From:   "Kuppuswamy, Sathyanarayanan" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <mgross@...ux.intel.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Peter H Anvin <hpa@...or.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Kirill Shutemov <kirill.shutemov@...ux.intel.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Kuppuswamy Sathyanarayanan <knsathya@...nel.org>,
        X86 ML <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        platform-driver-x86@...r.kernel.org, bpf@...r.kernel.org,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest
 attestation interface driver



On 7/8/21 4:36 PM, Dan Williams wrote:
>> +static int tdg_attest_open(struct inode *inode, struct file *file)
>> +{
>> +       /*
>> +        * Currently tdg_event_notify_handler is only used in attestation
>> +        * driver. But, WRITE_ONCE is used as benign data race notice.
>> +        */
>> +       WRITE_ONCE(tdg_event_notify_handler, attestation_callback_handler);
> Why is this ioctl not part of the driver that registered the interrupt

We cannot club them because they are not functionally related. Even notification
is a separate common feature supported by TDX and configured using
SetupEventNotifyInterrupt hypercall. It is not related to TDX attestation.
Attestation just uses event notification interface to get the quote
completion event.

> handler for this callback in the first instance? I've never seen this
> style of cross-driver communication before.

This is similar to x86_platform_ipi_callback() acrn_setup_intr_handler()
use cases.

> 
>> +
>> +       file->private_data = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
>> +                                                     get_order(QUOTE_SIZE));
> Why does this driver abandon all semblance of type-safety and use
> ->private_data directly? This also seems an easy way to consume
> memory, just keep opening this device over and over again.
> 
> AFAICS this buffer is only used ephemerally. I see no reason it needs
> to be allocated once per open file. Unless you need several threads to
> be running the attestation process in parallel just allocate a single
> buffer at module init (statically defined or on the heap) and use a
> lock to enforce only one user of this buffer at a time. That would
> also solve your direct-map fracturing problem.

Theoretically attestation requests can be sent in parallel. I have
allocated the memory in open() call mainly for this reason. But current
TDX ABI specification does not clearly specify this possibility and I am
not sure whether TDX KVM supports it. Let me confirm about it again with
TDX KVM owner. If such model is not currently supported, then I will move
the memory allocation to init code.

> 
> All that said, this new user ABI for passing blobs in and out of the
> kernel is something that the keyutils API already does. Did you
> consider add_key() / request_key() for this case? That would also be
> the natural path for the end step of requesting the drive decrypt key.
> I.e. a chain of key payloads starting with establishing the
> attestation blob.

I am not sure whether we can use keyutil interface for attestation. AFAIK,
there are other use cases for attestation other than  getting keys for
encrypted drives.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer