linux-kernel - Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes

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: <20150908153013.GA6758@roeck-us.net>
Date:	Tue, 8 Sep 2015 08:30:13 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	Emilio López <emilio.lopez@...labora.co.uk>
Cc:	gregkh@...uxfoundation.org, olof@...om.net, kgene@...nel.org,
	k.kozlowski@...sung.com, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes

Emilio,

On Tue, Sep 08, 2015 at 09:07:44AM -0300, Emilio López wrote:
> According to the sysfs header file:
> 
>     "The returned value will replace static permissions defined in
>      struct attribute or struct bin_attribute."
> 
> but this isn't the case, as is_visible is only called on
> struct attribute only. This patch adds the code paths required
> to support is_visible() on binary attributes.
> 
> Signed-off-by: Emilio López <emilio.lopez@...labora.co.uk>
> ---
>  fs/sysfs/group.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 39a0199..eb6996a 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -37,10 +37,10 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>  {
>  	struct attribute *const *attr;
>  	struct bin_attribute *const *bin_attr;
> -	int error = 0, i;
> +	int error = 0, i = 0;
>  
>  	if (grp->attrs) {
> -		for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> +		for (attr = grp->attrs; *attr && !error; i++, attr++) {
>  			umode_t mode = (*attr)->mode;
>  
>  			/*
> @@ -73,13 +73,27 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>  	}
>  
>  	if (grp->bin_attrs) {
> -		for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> +		for (bin_attr = grp->bin_attrs; *bin_attr; i++, bin_attr++) {
> +			umode_t mode = (*bin_attr)->attr.mode;
> +
>  			if (update)
>  				kernfs_remove_by_name(parent,
>  						(*bin_attr)->attr.name);
> +			if (grp->is_visible) {
> +				mode = grp->is_visible(kobj,
> +						       &(*bin_attr)->attr, i);

With this, if 'n' is the number of non-binary attributes,

for i < n:
	The index passed to is_visible points to a non-binary attribute.
for i >= n:
	The index passed to is_visible points to the (index - n)th binary
	attribute.

Unless I am missing something, this is not explained anywhere, but it is
not entirely trivial to understand. I think it should be documented.

Also, it might be a good idea to check through existing code to ensure
that this change doesn't accidentially cause trouble (existing drivers
implementing both attribute types may not expect to see an index variable
pointing to a value larger than the number of elements in the attrs array).

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