Re: [RFC PATCH] platform/x86: Add sysfs interface for Intel TXT status

From: Greg KH <gregkh@linuxfoundation.org>
To: Jonathan McDowell <noodles@fb.com>
Cc: Dmitrii Okunev <xaionaro@fb.com>,
	Hans de Goede <hdegoede@redhat.com>,
	Mark Gross <markgross@kernel.org>,
	Qiaowei Ren <qiaowei.ren@intel.com>,
	Matthew Garrett <mjg59@srcf.ucam.org>,
	Xiaoyan Zhang <xiaoyan.zhang@intel.com>,
	Pavel Machek <pavel@denx.de>, "x86@kernel.org" <x86@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"platform-driver-x86@vger.kernel.org" 
	<platform-driver-x86@vger.kernel.org>
Subject: Re: [RFC PATCH] platform/x86: Add sysfs interface for Intel TXT status
Date: Wed, 9 Mar 2022 12:29:27 +0100	[thread overview]
Message-ID: <YiiPl/zThbkKToar@kroah.com> (raw)
In-Reply-To: <YiiIRLZGq5c5uBjX@noodles-fedora-PC23Y6EG.dhcp.thefacebook.com>

On Wed, Mar 09, 2022 at 10:58:17AM +0000, Jonathan McDowell wrote:
> On Wed, Mar 09, 2022 at 11:48:23AM +0100, Greg KH wrote:
> > On Wed, Mar 09, 2022 at 10:40:03AM +0000, Jonathan McDowell wrote:
> > > (This is an RFC to see if the approach is generally acceptable; unlike
> > > the previous driver this exposes the information purely as read-only
> > > status information, so userspace can make an informed decision about the
> > > system state without having to poke about in /dev/mem. There are still a
> > > few extra registers I'm trying to dig up information for before a proper
> > > submission.)
> > > 
> > > This module provides read-only access to the Intel TXT (Trusted
> > > Execution Technology) status registers, allowing userspace to determine
> > > the status of measured boot and whether the dynamic root of trust for
> > > measurement (DRTM) has been fully enabled.
> > > 
> > > Tools such as txt-stat from tboot
> > > <https://sourceforge.net/projects/tboot/ > can make use of this driver to
> > > display state rather than relying on access to /dev/mem.
> > > 
> > > See Documentation/x86/intel_txt.rst for more information about Intel
> > > TXT.
> > > 
> > > Signed-off-by: Jonathan McDowell <noodles@fb.com>
> > > ---
> > >  arch/x86/include/asm/txt.h             |  34 +++++
> > >  drivers/platform/x86/intel/Kconfig     |  14 ++
> > >  drivers/platform/x86/intel/Makefile    |   2 +
> > >  drivers/platform/x86/intel/txt_sysfs.c | 185 +++++++++++++++++++++++++
> > 
> > No Documentation/ABI/ entry for your new sysfs entry?  How can we
> > evaluate if this is a good api then?
> 
> As a read-only export of configuration registers is a full set of info
> in Documentation/ABI/ required? I didn't get a feel for how required
> that was from the existing files there.

For all sysfs entries, yes, it is required.  Run the scripts/get_abi.pl
tool as proof :)

> > Wait, I don't see any sysfs code in here, are you sure you sent a viable
> > patch?
> 
> The export to sysfs is via securityfs, as that seemed to be the
> appropriate route (it fits into a similar area as
> /sys/kernel/security/integrity/ima/ or /sys/kernel/security/tpm0/,
> providing userspace with some visibility of what the kernel thinks the
> state is).

Then this is securityfs, NOT sysfs.  securityfs just happens to be
mounted at that location.  You could mount it anywhere else as well.
Please fix up the terminology here, it is very confusing and has nothing
to do with sysfs at all.

thanks,

greg k-h