netdev - Re: [E1000-devel] [net-next v4 7/8] i40e: sysfs and debugfs interfaces

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: <20130904173759.7c2eec52@nehalam.linuxnetplumber.net>
Date:	Wed, 4 Sep 2013 17:37:59 -0700
From:	Stephen Hemminger <stephen@...workplumber.org>
To:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc:	davem@...emloft.net, e1000-devel@...ts.sourceforge.net,
	netdev@...r.kernel.org,
	Jesse Brandeburg <jesse.brandeburg@...el.com>,
	gospo@...hat.com, sassmann@...hat.com
Subject: Re: [E1000-devel] [net-next v4 7/8] i40e: sysfs and debugfs
 interfaces

I don't think you need this. If you put a NULL pointer in for the __ATTR()
then it will do the right thing for you.

> +/**
> + * i40e_sys_store_ro - callback for readonly attributes in sysfs
> + * @kobj: object in the sysfs model
> + * @attr: attribute being read
> + * @buf: buffer to put data
> + * @count: buffer size
> + **/
> +static ssize_t i40e_sys_store_ro(struct kobject *kobj,
> +				 struct kobj_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	return -1;
> +}
> +

These are bogus, just return an error code or make don't define
the file in the first place. Returning error in contents
is not useful for programmatic usage.

> +
> +/**
> + * i40e_sys_veb_svlan_read - read the VEB svlan
> + **/
> +static ssize_t i40e_sys_veb_svlan_read(struct kobject *kobj,
> +				       struct kobj_attribute *attr,
> +				       char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "(not implemented)\n");
> +}
> +

The store routines should all be checking for permissions.

	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
		return -EPERM;

The sysfs API is doing writes to device state without locking.
You probably want to do:
	if (!rtnl_trylock())
		return restart_syscall();

...
	rtnl_unlock();

Also, anything in sysfs is device specific and you really need to make
a strong case for why your device is special and needs an exception.
Other devices will have hardware switches and doing something through
sysfs is going to create a pain for any controller application.

I vote against including the sysfs VEB stuff because it will become
a lifetime ABI.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html