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

Message-ID: <662beb6ad280f_db82d29458@dwillia2-xfh.jf.intel.com.notmuch>
Date: Fri, 26 Apr 2024 10:59:06 -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:
[..]
> > --- a/fs/sysfs/group.c
> > +++ b/fs/sysfs/group.c
> > @@ -33,10 +33,10 @@ static void remove_files(struct kernfs_node *parent,
> >  
> >  static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
> >  {
> > -	if (grp->attrs && grp->is_visible)
> > +	if (grp->attrs && grp->attrs[0] && grp->is_visible)
> >  		return grp->is_visible(kobj, grp->attrs[0], 0);
> >  
> > -	if (grp->bin_attrs && grp->is_bin_visible)
> > +	if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible)
> >  		return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
> >  
> >  	return 0;
> 
> I'm wondering why 0 is returned by default and not SYSFS_GROUP_INVISIBLE.
> 
> An empty attribute list (containing just the NULL sentinel) will now
> result in the attribute group being visible as an empty directory.
> 
> I thought the whole point was to hide such empty directories.
> 
> Was it a conscious decision to return 0?

Perhaps there should be a comment here because yes, this is on purpose.

> Did you expect breakage if SYSFS_GROUP_INVISIBLE is returned?

Yes, the history is here:

    https://lore.kernel.org/all/YwZCPdPl2T+ndzjU@kroah.com/

..where an initial attempt to hide empty group directories resulted in
boot failures. The concern is that there might be user tooling that
depends on that empty directory. So the SYSFS_GROUP_INVISIBLE behavior
can only be enabled by explicit result from an is_visible() handler.

That way there is no regression potential for legacy cases where the
empty directory might matter.