linux-kernel - Re: [PATCH] sysfs: kill an extra put in sysfs_create

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a781481a0707180816r478d2512v953036a1e4145a72@mail.gmail.com>
Date:	Wed, 18 Jul 2007 20:46:35 +0530
From:	"Satyam Sharma" <satyam.sharma@...il.com>
To:	"Tejun Heo" <htejun@...il.com>
Cc:	"Gabriel C" <nix.or.die@...glemail.com>,
	"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
	"Christoph Lameter" <clameter@....com>, gregkh@...e.de,
	miles.lane@...il.com
Subject: Re: [PATCH] sysfs: kill an extra put in sysfs_create_link() failure path

Hi,

On 7/18/07, Tejun Heo <htejun@...il.com> wrote:
> Satyam Sharma wrote:
> > On 7/18/07, Tejun Heo <htejun@...il.com> wrote:
> >> There is a subtle bug in sysfs_create_link() failure path.  When
> >> symlink creation fails because there's already a node with the same
> >> name, the target sysfs_dirent is put twice - once by failure path of
> >> sysfs_create_link() and once more when the symlink is released.
> >
> > The "symlink" is released? But the creation of the symlink is
> > precisely what failed here ... did it not?
> >
> >> Fix it by making only the symlink node responsible for putting
> >> target_sd.
> >
> > And again ... the changelog sounds confusing indeed, perhaps I'm
> > not familiar enough with sysfs symlink-related terminology/semantics.
> > Care to elaborate?
> >
> >>         sd = sysfs_new_dirent(name, S_IFLNK|S_IRWXUGO, SYSFS_KOBJ_LINK);
> >>         if (!sd)
> >>                 goto out_put;
> >> +
> >>         sd->s_elem.symlink.target_sd = target_sd;
> >> +       target_sd = NULL;       /* reference is now owned by the
> >> symlink */
> >
> > Wow. This looks like a very mysterious way to fix a mysterious bug :-)
> > BTW I just looked over at sysfs_create_link() and ... it looks quite ...
> > unnecessarily complicated/obfuscated ...
>
> Well, I dunno.  Probably my taste just sucks.  Please feel free to
> submit patches and/or suggest better ideas.

OK, for example:

sysfs_find_dirent() -- to check for -EEXIST -- should be called
*before* we create the new dentry for the to-be-created symlink
in the first place. [ It's weird to grab a reference on the target
for ourselves (and in fact even allocate the new dirent for the
to-be-created symlink) and /then/ check for erroneous usage,
and then go about undoing all that we should never have done
at all. ] So this test could, and should, be made earlier, IMHO.

And some similar others ... so attached (sorry, Gmail web
interface) please find an attempt to make sysfs_create_link look
a trifle more like what it should look like, IMHO. The code cleanup
also leads to fewer LOC, smaller kernel image (lesser by 308 bytes),
and even speeding up the no-error common case of this function,
apart from the obvious readability benefits ... it's diffed on _top_ of
your bugfix here, but not the other patch. [ Compile-tested only. ]

BTW this bug was clearly *very* subtle. I spent a couple of hours or
so yesterday night on the same resulting use-after-free (which actually
gets triggered in a completely different codepath, and which is
completely temporally disconnected from its actual cause over here).

Thanks,
Satyam

View attachment "sanitize-sysfs_create_link-new.txt" of type "text/plain" (2848 bytes)