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

["Kuppuswamy, Sathyanarayanan" <sathyanarayanan.kuppuswamy@linux.intel.com>]

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