linux-kernel - Re: [PATCH v4 1/3] x86/tdx: 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: <a597c1fffab01e1048421887d5ef7aa2ac01c46e.camel@intel.com>
Date:   Wed, 27 Apr 2022 17:15:41 +1200
From:   Kai Huang <kai.huang@...el.com>
To:     Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@...ux.intel.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>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface
 driver

On Tue, 2022-04-26 at 12:07 -0700, Sathyanarayanan Kuppuswamy wrote:
> > Is there any particular reason to use platform driver and support it as a
> > module?
> > 
> > SGX driver uses misc_register() to register /dev/sgx_enclave during boot. 
> > Looks
> > it would be simpler.
> 
> Main reason is to use a proper device in dma_alloc* APIs.
> 
> My initial version only used misc device as you have mentioned. But
> Hans raised a concern about using proper struct device in dma_alloc*
> APIs and suggested modifying the driver to use platform device
> model. You can find relevant discussion here.
> 
> https://lore.kernel.org/all/47d06f45-c1b5-2c8f-d937-3abacbf10321@redhat.com/

Thanks for the info.  Sorry I didn't dig review comments for previous version.

However, after digging more, I am wondering why do you need to use DMA API at
the first place.

Firstly, for this basic driver to report TDREPORT to userspace, there's no need
to use any DMA API, nor we need to use shared memory, as we just get the report
into some buffer (doesn't need to be shared) and copy the report back to
userspace.  So it doesn't make a lot sense to use platform device here.

Secondly, in terms of GetQuote support, it seems Dave/Andi suggested you can use
vmalloc()/vmap() and just use set_memory_decrypted() to convert it to shared:

https://lore.kernel.org/all/ce0feeec-a949-35f8-3010-b0d69acbbc2e@linux.intel.com/

I don't see why it cannot work?  Then wouldn't this approach be simpler than
using DMA API?  Any reason to choose platform device?

Btw, a side topic:

Andy suggested we don't do memory allocation and private-shared conversion at
IOCTL time as the conversion is expensive:

https://lore.kernel.org/all/06c85c19-e16c-3121-ed47-075cfa779b67@kernel.org/

This is reasonable (and sorry I didn't see this when I commented in v3).

To avoid IOCTL time private-shared conversion, and yet support dynamic Quote
length, can we do following:

- Allocate a *default* size buffer at driver loading time (i.e. 4 pages), and
convert to shared.  This default size should cover 99% cases as Intel QGS
currently generates Quote smaller than 8K, and Intel attestation agent hard-code
a 4 pages buffer for Quote.

- In GetQuote IOCTL, when the len is larger than default size, we discard the
original one and allocate a larger buffer.

How does this sound?

-- 
Thanks,
-Kai