linux-kernel - Re: [PATCH] sysfs: group: allow is

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: <54BB102B.1060606@roeck-us.net>
Date:	Sat, 17 Jan 2015 17:45:15 -0800
From:	Guenter Roeck <linux@...ck-us.net>
To:	Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:	linux-kernel@...r.kernel.org, kernel@...oirfairelinux.com
Subject: Re: [PATCH] sysfs: group: allow is_visible to drop permissions

On 01/17/2015 02:09 PM, Vivien Didelot wrote:
> Hi Guenter, Greg,
>
[ .. ]

>
> BTW Guenter, does this patch make sense to you?
>

It does make sense to me to only use the return value from is_visible
for the mode.

As for which bits to use, I am not entirely sure. I think it would be
more important to first decide which bits should be acceptable to start with.

Then I would _always_ only use the bits from mode, masked against the
valid bits, whatever they are.

	umode_t mode = (*attr)->mode;
	...
	if (grp->is_visible) {
		mode = grp->is_visible(kobj, *attr, i);
		if (!mode)
			continue;
	}

	WARN(mode & ~(S_IRUGO | S_IWUGO | SYSFS_PREALLOC),	/* optional */
	     "Attribute %s: Invalid permission 0x%x\n", (*attr)->name, mode);

	mode &= S_IRUGO | S_IWUGO | SYSFS_PREALLOC;
	error = sysfs_add_file_mode_ns(parent, *attr, false, mode, NULL);
	...

>
> My assumption here was that the attribute group is_visible function
> should just be able to adjust the UGO bits. Am I correct?
>
I would think so.

> I'm not even sure about the execute permission though. Only one driver
> uses it for an attribute and it seems wrong, in drivers/hid/hid-lg4ff.c:
>
> static DEVICE_ATTR(range, S_IRWXU | S_IRWXG | S_IROTH, lg4ff_range_show, lg4ff_range_store);
>
That seems wrong.

>
> The actual behavior seems wrong to me. Again, what happens is you return
> SYSFS_PREALLOC, that the underlying sysfs_add_file_mode_ns() function is
> actually checking?
>
Ultimately, the implementor asked for it.

> IMHO, if we want an attribute group to only be able to "hide or show" an
> attribute, then is_visible (as the name suggests) should return a
> boolean. If we want it be able to adjust permissions (as it seems
> correct, given the examples), we should identify which permissions are
> OK to change, deprecate is_visible function (to avoid code break) in
> favor of a new one which limits the bits to that scope.
>

Up to Greg to decide. From my perspective, we have lived with is_visible
for several years and overall it seems to work. Sure, it lacks a clear
API, but that can be fixed without changing a lot of code just to replace
the function name.

Guenter

--
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/