linux-kernel - Re: [PATCH] Prevent crash on missing sysfs attribute group

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: <20120403071543.GA17502@gmail.com>
Date:	Tue, 3 Apr 2012 09:15:43 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Bruno Prémont <bonbons@...ux-vserver.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Peter Zijlstra <peterz@...radead.org>,
	linux-kernel@...r.kernel.org,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] Prevent crash on missing sysfs attribute group


* Eric W. Biederman <ebiederm@...ssion.com> wrote:

> Nacked-by: "Eric W. Biederman" <ebiederm@...ssion.com>
> 
> Bruno Prémont <bonbons@...ux-vserver.org> writes:
> 
> > Prevent kernel from crashing when a device is being registered with sysfs
> > but has no (aka NULL) group attributes, but warn about it so calling path
> > can get fixed.
> 
> The idea is reasonable but the implementation is horrible.
> 
> >> Will do - but the underlying generic bug should be fixed as 
> >> well: we must not crash just because some attributes are missing 
> >> in a rarely used sub-driver ...
> >> 
> >> We should WARN_ON(), etc. - but not crash.
> 
> FIX perf to include sanity checks.

Huh, so put repeated, duplicated, inconsistently applied sanity 
checks into dozens of sysfs attribute using kernel subsystems?

Major FAIL, dude.

> Anything we do in sysfs is just pointless because perf was 
> clever and the offender did not show up in the backtrace.
> 
> Right now perf is so bad we just waste everyone's time.
>
> > Greg, is this ok for you or should the check be moved out to 
> > calling internal_create_group()?
> 
> Please put changes in internal_create_group where all of the 
> rest of the checks are.

So you *do* agree that a check in a generic place is useful 
after all? ;-)

> We should do something like:
> if (!grp->attrs) {
> 	WARN(1, "sysfs: idiot subsystem did not include attrs for group: %s/%s\n"
>         	kobj->name, grp->name?"":grp->name);
> 	return -EINVAL;
> }
>
> As it stands your patch is horrible it leaves sysfs in an 
> inconsistent state.  Creating the directory and leaving it 
> there.  Not returning an error code.  It looks like there are 
> all kinds of weird problems that removing the group or 
> updating the group could get into if we go with your patch.

This is actually a sensible suggestion. Bruno, mind updating 
your patch to do something like this? Assuming Greg agrees with 
putting the check/warning there.

Eric's rant about putting sanit checks at every usage site is 
just crazy talk.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/