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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Tue, 03 Apr 2012 00:11:57 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Bruno Prémont <bonbons@...ux-vserver.org>
Cc:	Ingo Molnar <mingo@...nel.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


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.

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.

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.

Eric
> ---
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index dd1701c..0040ff2 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -32,7 +32,8 @@ static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
>  	struct attribute *const* attr;
>  	int error = 0, i;
>  
> -	for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> +	WARN_ON(!grp->attrs);
> +	for (i = 0, attr = grp->attrs; attr && *attr && !error; i++, attr++) {
>  		umode_t mode = 0;
>  
>  		/* in update mode, we're changing the permissions or
--
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/