Re: [patch 2/4] mISDN: add support for group membership check

[David Miller <davem@davemloft.net>]

From: Jiri Slaby <jslaby@suse.cz>
Date: Fri, 20 Sep 2013 15:44:33 +0200

> On 09/15/2013 01:28 AM, Ben Hutchings wrote:
>>> @@ -694,6 +699,10 @@ base_sock_ioctl(struct socket *sock, uns 
>>> case IMSETDEVNAME: { struct mISDN_devrename dn; +		if
>>> (!capable(CAP_SYS_ADMIN) && +				!gid_eq(misdn_permitted_gid,
>>> current_gid()) && +				!in_group_p(misdn_permitted_gid)) +
>>> return -EPERM; if (copy_from_user(&dn, (void __user *)arg, 
>>> sizeof(dn))) { err = -EFAULT;
>> 
>> This seems to be the important bit: renaming of devices (if allowed
>> at all) ought to be limited to CAP_SYS_ADMIN or possibly
>> CAP_NET_ADMIN. But why should the group that is allowed to use
>> mISDN data sockets also be allowed to do this?
> 
> This is based on an old patch we are dragging in SUSE since 2009:
> http://www.isdn4linux.de/pipermail/isdn4linux/2009-December/004493.html
> https://bugzilla.novell.com/show_bug.cgi?id=564423
> 
> The whole point of the gid-based access was to still allow some user
> group to manipulate the device in an arbitrary way.
> 
> So if everybody agrees I will just disallow rename to
> non-CAP_NET_ADMIN users and we are done?

No we are not done, sorry.

Having a device specific module parameter for this is wrong on several
fundamental levels.

Module parameters are wrong because you're going to eventually want to
do this for other ISDN or other devices, and nobody is going to make
sure the name of the parameter nor the semantics all match up.

This is the worst possible user experience.

You need to make it so that there is a standard, regular, interface
for performing this task or making this setting.  Module parameters
are not it.

The reason this patch has rotted since 2009 is that it really is far
from suitable for upstream inclusion, and I really don't see that much
thought or care has been put into how it is implemented, so I wonder
how much anyone really cares about the change.