lkml.org 
[lkml]   [2021]   [Jul]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver
From
Date


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

\
 
 \ /
  Last update: 2021-07-09 01:57    [W:0.205 / U:0.664 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site