linux-kernel - Re: [PATCH 1/3] sysfs: Fix crash on empty group attributes array

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] [day] [month] [year] [list]

Date: Mon, 29 Apr 2024 10:47:46 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Lukas Wunner <lukas@...ner.de>, Dan Williams <dan.j.williams@...el.com>
CC: <gregkh@...uxfoundation.org>, Pierre-Louis Bossart
	<pierre-louis.bossart@...ux.intel.com>, Marc Herbert
	<marc.herbert@...el.com>, "Rafael J. Wysocki" <rafael@...nel.org>,
	<linux-kernel@...r.kernel.org>, <linux-pci@...r.kernel.org>,
	<linux-coco@...ts.linux.dev>, <alsa-devel@...a-project.org>
Subject: Re: [PATCH 1/3] sysfs: Fix crash on empty group attributes array

Lukas Wunner wrote:
> On Sat, Apr 27, 2024 at 02:33:24PM -0700, Dan Williams wrote:
> > Lukas Wunner wrote:
> > > Perhaps an optional ->is_group_visible() callback in struct attribute_group
> > > which gets passed only the struct kobject pointer?
> > > 
> > > At least for PCI device authentication, that would be sufficient.
> > > I could get from the kobject to the corresponding struct device,
> > > then determine whether the device supports authentication or not.
> > > 
> > > Because it's a new, optional callback, there should be no compatibility
> > > issues.  The SYSFS_GROUP_INVISIBLE return code from the ->is_visible()
> > > call for individual attributes would not be needed then, at least in my
> > > use case.
> > 
> > That's where I started with this, but decided it was overkill to
> > increase the size of that data structure globally for a small number of
> > use cases.
> 
> Memory is cheap and memory-constrained devices can set CONFIG_SYSFS=n.

That sounds severe, but point taken that someone could config-off the
cases that need this extension.

> There aren't that many struct attribute_groups and this is just
> 8 additional bytes on a 64-bit machine.  (There are way more
> struct attribute than struct attribute_group.)  The contortions
> necessary to overload individual attribute ->is_visible() callbacks
> to also govern the group's visibility aren't worth it.

I agree that most systems would not care about growing this structure,
but the same is true for almost all other data structure growth in the
kernel. It is a typical kernel pastime to squeeze functionality into
existing data structures.

> Having an ->is_group_visible() callback has the additional benefit that
> the mode of directories no longer needs to be hardcoded to 0755 in
> sysfs_create_dir_ns(), but can be set to, say, 0500 or 0700 or 0511,
> depending on the use case.  So more flexibility there as well.

Unnecessary growth is unnecessary growth. In this case all the known use
cases can use the SYSFS_GROUP_INVISIBLE flag returned from is_visible().
The awkwardness around cases that want to have an empty attributes array
and invisible group directory is noted and puts the solution on notice
for running afoul of the sunk cost fallacy in the future.