Message-ID: <2024042736-radiance-unaired-c69c@gregkh>
Date: Sat, 27 Apr 2024 12:47:11 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Lukas Wunner <lukas@...ner.de>
Cc: Dan Williams <dan.j.williams@...el.com>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	linux-kernel@...r.kernel.org,
	Alan Stern <stern@...land.harvard.edu>,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH] sysfs: Allow bin_attributes to be added to groups

On Fri, Apr 26, 2024 at 08:59:51PM +0200, Lukas Wunner wrote:
> 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@...ner.de>
> > > Cc: Alan Stern <stern@...land.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 for the details, that makes sense.

And overall, the patch also looks good, so just include it in the patch
series that uses it and I'll be glad to ack it then.

thanks,

greg k-h

Powered by blists - more mailing lists