Received-SPF: pass (google.com: domain of linux-kernel+bounces-160590-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 26 Apr 2024 20:59:51 +0200
From: Lukas Wunner <lukas@wunner.de>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	linux-kernel@vger.kernel.org,
	Alan Stern <stern@rowland.harvard.edu>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>
Subject: Re: [PATCH] sysfs: Allow bin_attributes to be added to groups
Message-ID: <Ziv5p3SMEIw3XZkK@wunner.de>
References: <e140f6bf8dc99c24cc387f7ac3c7908c8830587f.1714030457.git.lukas@wunner.de>
 <662be72549aaf_db82d294c7@dwillia2-xfh.jf.intel.com.notmuch>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <662be72549aaf_db82d294c7@dwillia2-xfh.jf.intel.com.notmuch>

On Fri, Apr 26, 2024 at 10:40:53AM -0700, Dan Williams wrote:
> Lukas Wunner wrote:
> > Commit dfa87c824a9a ("sysfs: allow attributes to be added to groups")
> > introduced dynamic addition of sysfs attributes to groups.
> > 
> > Allow the same for bin_attributes, in support of a forthcoming commit
> > which adds various bin_attributes every time a PCI device is
> > authenticated.
> > 
> > Addition of bin_attributes to groups differs from regular attributes in
> > that different kernfs_ops are selected by sysfs_add_bin_file_mode_ns()
> > vis-?-vis sysfs_add_file_mode_ns().
> > 
> > So call either of those two functions from sysfs_add_file_to_group()
> > based on an additional boolean parameter and add two wrapper functions,
> > one for bin_attributes and another for regular attributes.
> > 
> > Removal of bin_attributes from groups does not require a differentiation
> > for bin_attributes and can use the same code path as regular attributes.
> > 
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > ---
> > Submitting this ahead of my PCI device authentication v2 patches.
> > Not sure if the patch is acceptable without an accompanying user,
> > but even if it's not, perhaps someone has early review feedback
> > or wants to provide an Acked-by?  Thank you!
> 
> On the one hand it makes sense from a symmetry perspective, on the other
> hand the expectation and the infrastructure for dynamic sysfs visibilty
> has increased since 2007.
> 
> That is why I would like to see the use case to understand why a
> dynamically added a bin_attribute is needed compared with a statically
> defined attribute with dynamic visibility.

I assume "would like to see" means "on the mailing list", which would
(or will) be as part of the PCI device authentication v2 patches.

But in case you're curious, the use case is the log of signatures
presented by the device.  Each signature is exposed as a bin_attribute
in a signatures/ directory below a PCI device's sysfs directory,
for verification by a remote attester (or user space in general).

Here's the code:

https://github.com/l1k/linux/commit/ca420b22af05

The signature's bin_attribute is accompanied by several other
bin_attributes containing ancillary data, such as nonces
(to allow user space to ascertain that a fresh nonce has been used).

The signatures/ directory is an empty attribute group to which the
signatures received from the device are dynamically added using
sysfs_add_bin_file_to_group() (introduced by the present patch).

All of that was done to address an objection raised by James Bottomley
at Plumbers that it is "not good enough" if the kernel keeps the
challenge-response received from the device to itself and doesn't
allow a remote attester to verify it.

Here is the recording, in his own words:

https://www.youtube.com/watch?v=ZqMIlZ5lPAw&t=2345s

A static attribute with dynamic visibility doesn't cut it in this case:

Each signature needs a unique filename and the number of signatures
that can be generated is essentially unlimited.  (Though older ones
are likely uninteresting and can be culled.)  If you want to expose,
say, up to 1000 signatures per device, you'd have to allocate an array
of 1000 signatures (for each device!), even though the actual number
of signatures received might be much lower.  It would be a waste of
memory.  It is much more economical to add signature attributes
dynamically on demand.

It has the additional benefit that it allows user space to dynamically
adjust the maximum number of signatures retained in the log.
That's more difficult to implement with static attributes, as you'd
have to reallocate the attributes array and adjust all the pointers
pointing to it.

Thanks,

Lukas