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]
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


Powered by blists - more mailing lists