[PATCH] sysfs: Fix internal_create_group() for named group updates

[PATCH] sysfs: Fix internal_create_group() for named group updates

[Rajat Jain]

There are a couple of problems with named group updates in the code
today:

* sysfs_update_group() will always fail for a named group, because
  internal_create_group() will try to create a new sysfs directory
  unconditionally, which will ofcourse fail with -EEXIST.

* We can leak the kernfs_node for grp->name if some one tries to:
  - rename a group (change grp->name), or
  - update a named group, to an unnamed group

It appears that the whole purpose of sysfs_update_group() was to
allow changing the permissions or visibility of attributes and not
the names. So make it clear in the comments, and allow it to update
an existing named group.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 fs/sysfs/group.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 4802ec0e1e3a..8bd10dc730ae 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -119,12 +119,23 @@ static int internal_create_group(struct kobject *kobj, int update,
 		return -EINVAL;
 	}
 	if (grp->name) {
-		kn = kernfs_create_dir(kobj->sd, grp->name,
-				       S_IRWXU | S_IRUGO | S_IXUGO, kobj);
-		if (IS_ERR(kn)) {
-			if (PTR_ERR(kn) == -EEXIST)
-				sysfs_warn_dup(kobj->sd, grp->name);
-			return PTR_ERR(kn);
+		if (update) {
+			kn = kernfs_find_and_get(kobj->sd, grp->name);
+			if (!kn) {
+				WARN(1,
+				     "Can't update unknown attr grp name: %s/%s\n",
+				     kobj->name, grp->name);
+				return -EINVAL;
+			}
+		} else {
+			kn = kernfs_create_dir(kobj->sd, grp->name,
+					       S_IRWXU | S_IRUGO | S_IXUGO,
+					       kobj);
+			if (IS_ERR(kn)) {
+				if (PTR_ERR(kn) == -EEXIST)
+					sysfs_warn_dup(kobj->sd, grp->name);
+				return PTR_ERR(kn);
+			}
 		}
 	} else
 		kn = kobj->sd;
@@ -199,7 +210,8 @@ EXPORT_SYMBOL_GPL(sysfs_create_groups);
  * of the attribute files being created already exist.  Furthermore,
  * if the visibility of the files has changed through the is_visible()
  * callback, it will update the permissions and add or remove the
- * relevant files.
+ * relevant files. Changing a group's name (subdirectory name under
+ * kobj's directory in sysfs) is not allowed.
  *
  * The primary use for this function is to call it after making a change
  * that affects group visibility.
-- 
2.18.0.rc1.244.gcf134e6275-goog

Re: [PATCH] sysfs: Fix internal_create_group() for named group updates

[Greg Kroah-Hartman]

On Fri, Jun 15, 2018 at 06:29:10PM -0700, Rajat Jain wrote:
> There are a couple of problems with named group updates in the code
> today:
> 
> * sysfs_update_group() will always fail for a named group, because
>   internal_create_group() will try to create a new sysfs directory
>   unconditionally, which will ofcourse fail with -EEXIST.
> 
> * We can leak the kernfs_node for grp->name if some one tries to:
>   - rename a group (change grp->name), or
>   - update a named group, to an unnamed group
> 
> It appears that the whole purpose of sysfs_update_group() was to
> allow changing the permissions or visibility of attributes and not
> the names. So make it clear in the comments, and allow it to update
> an existing named group.

Who uses sysfs_update_group() today that has these problems?  Or do you
want to use it in new code?  How can it be broken today so badly that it
does not work?

> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
>  fs/sysfs/group.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 4802ec0e1e3a..8bd10dc730ae 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -119,12 +119,23 @@ static int internal_create_group(struct kobject *kobj, int update,
>  		return -EINVAL;
>  	}
>  	if (grp->name) {
> -		kn = kernfs_create_dir(kobj->sd, grp->name,
> -				       S_IRWXU | S_IRUGO | S_IXUGO, kobj);
> -		if (IS_ERR(kn)) {
> -			if (PTR_ERR(kn) == -EEXIST)
> -				sysfs_warn_dup(kobj->sd, grp->name);
> -			return PTR_ERR(kn);
> +		if (update) {
> +			kn = kernfs_find_and_get(kobj->sd, grp->name);
> +			if (!kn) {
> +				WARN(1,
> +				     "Can't update unknown attr grp name: %s/%s\n",
> +				     kobj->name, grp->name);
> +				return -EINVAL;

This is going to cause the syzbot to bug the heck out of us, as people
do run with panic-on-warning.  Just make this a "normal" error message
and dump the stack if you want that.

But maybe we should just get rid of this function entirely, it feels
very ackward and I can't remember why we added it...

thanks,

greg k-h

Re: [PATCH] sysfs: Fix internal_create_group() for named group updates

[Rajat Jain]

Hi Greg,

Thanks for your review.

On Sat, Jun 16, 2018 at 12:11 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Jun 15, 2018 at 06:29:10PM -0700, Rajat Jain wrote:
>> There are a couple of problems with named group updates in the code
>> today:
>>
>> * sysfs_update_group() will always fail for a named group, because
>>   internal_create_group() will try to create a new sysfs directory
>>   unconditionally, which will ofcourse fail with -EEXIST.
>>
>> * We can leak the kernfs_node for grp->name if some one tries to:
>>   - rename a group (change grp->name), or
>>   - update a named group, to an unnamed group
>>
>> It appears that the whole purpose of sysfs_update_group() was to
>> allow changing the permissions or visibility of attributes and not
>> the names. So make it clear in the comments, and allow it to update
>> an existing named group.
>
> Who uses sysfs_update_group() today that has these problems?  Or do you
> want to use it in new code?  How can it be broken today so badly that it
> does not work?

Sorry, my bad, I need to provide more clarification: None of the
existing users of this API use it on a named attribute group, thus
will not see this issue. I hit this issue while writing new code
(However, since then my code logic has changed so that it does not
need to use this API anymore).

I'm just trying to fix an issue with this API, that I stumbled upon,
which might be seen in future by any new code that uses this API with
named attribute groups. At the minimum, I think we should clarify in
comments (or better yet, ensure) that this API cannot be misused.


>
>> Signed-off-by: Rajat Jain <rajatja@google.com>
>> ---
>>  fs/sysfs/group.c | 26 +++++++++++++++++++-------
>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
>> index 4802ec0e1e3a..8bd10dc730ae 100644
>> --- a/fs/sysfs/group.c
>> +++ b/fs/sysfs/group.c
>> @@ -119,12 +119,23 @@ static int internal_create_group(struct kobject *kobj, int update,
>>               return -EINVAL;
>>       }
>>       if (grp->name) {
>> -             kn = kernfs_create_dir(kobj->sd, grp->name,
>> -                                    S_IRWXU | S_IRUGO | S_IXUGO, kobj);
>> -             if (IS_ERR(kn)) {
>> -                     if (PTR_ERR(kn) == -EEXIST)
>> -                             sysfs_warn_dup(kobj->sd, grp->name);
>> -                     return PTR_ERR(kn);
>> +             if (update) {
>> +                     kn = kernfs_find_and_get(kobj->sd, grp->name);
>> +                     if (!kn) {
>> +                             WARN(1,
>> +                                  "Can't update unknown attr grp name: %s/%s\n",
>> +                                  kobj->name, grp->name);
>> +                             return -EINVAL;
>
> This is going to cause the syzbot to bug the heck out of us, as people
> do run with panic-on-warning.  Just make this a "normal" error message
> and dump the stack if you want that.

Agreed, I will turn into a normal error.

>
> But maybe we should just get rid of this function entirely, it feels
> very ackward and I can't remember why we added it...

There are a couple of existing in-tree users of this API, and I do not
understand a lot about that code.

Thanks & Best Regards,

Rajat

>
> thanks,
>
> greg k-h

[PATCH v2] sysfs: Fix internal_create_group() for named group updates

[Rajat Jain]

There are a couple of problems with named group updates in the code
today:

* sysfs_update_group() will always fail for a named group, because
  internal_create_group() will try to create a new sysfs directory
  unconditionally, which will ofcourse fail with -EEXIST.

* We can leak the kernfs_node for grp->name if some one tries to:
  - rename a group (change grp->name), or
  - update a named group, to an unnamed group

It appears that the whole purpose of sysfs_update_group() was to
allow changing the permissions or visibility of attributes and not
the names. So make it clear in the comments, and allow it to update
an existing named group.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
v2: Use pr_warn() instead of WARN()

 fs/sysfs/group.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 4802ec0e1e3a..2b7d3536e6cb 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -119,12 +119,22 @@ static int internal_create_group(struct kobject *kobj, int update,
 		return -EINVAL;
 	}
 	if (grp->name) {
-		kn = kernfs_create_dir(kobj->sd, grp->name,
-				       S_IRWXU | S_IRUGO | S_IXUGO, kobj);
-		if (IS_ERR(kn)) {
-			if (PTR_ERR(kn) == -EEXIST)
-				sysfs_warn_dup(kobj->sd, grp->name);
-			return PTR_ERR(kn);
+		if (update) {
+			kn = kernfs_find_and_get(kobj->sd, grp->name);
+			if (!kn) {
+				pr_warn("Can't update unknown attr grp name: %s/%s\n",
+					kobj->name, grp->name);
+				return -EINVAL;
+			}
+		} else {
+			kn = kernfs_create_dir(kobj->sd, grp->name,
+					       S_IRWXU | S_IRUGO | S_IXUGO,
+					       kobj);
+			if (IS_ERR(kn)) {
+				if (PTR_ERR(kn) == -EEXIST)
+					sysfs_warn_dup(kobj->sd, grp->name);
+				return PTR_ERR(kn);
+			}
 		}
 	} else
 		kn = kobj->sd;
@@ -199,7 +209,8 @@ EXPORT_SYMBOL_GPL(sysfs_create_groups);
  * of the attribute files being created already exist.  Furthermore,
  * if the visibility of the files has changed through the is_visible()
  * callback, it will update the permissions and add or remove the
- * relevant files.
+ * relevant files. Changing a group's name (subdirectory name under
+ * kobj's directory in sysfs) is not allowed.
  *
  * The primary use for this function is to call it after making a change
  * that affects group visibility.
-- 
2.18.0.rc1.244.gcf134e6275-goog

Re: [PATCH v2] sysfs: Fix internal_create_group() for named group updates

[Greg Kroah-Hartman]

On Sat, Jun 16, 2018 at 01:18:37AM -0700, Rajat Jain wrote:
> There are a couple of problems with named group updates in the code
> today:
> 
> * sysfs_update_group() will always fail for a named group, because
>   internal_create_group() will try to create a new sysfs directory
>   unconditionally, which will ofcourse fail with -EEXIST.
> 
> * We can leak the kernfs_node for grp->name if some one tries to:
>   - rename a group (change grp->name), or
>   - update a named group, to an unnamed group
> 
> It appears that the whole purpose of sysfs_update_group() was to
> allow changing the permissions or visibility of attributes and not
> the names. So make it clear in the comments, and allow it to update
> an existing named group.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v2: Use pr_warn() instead of WARN()

Looks good, I'll queue it up after 4.18-rc1 is out, thanks for the
update so quickly.

greg k-h

[PATCH v3] sysfs: Fix internal_create_group() for named group updates

[Rajat Jain]

There are a couple of problems with named group updates in the code
today:

* sysfs_update_group() will always fail for a named group, because
  internal_create_group() will try to create a new sysfs directory
  unconditionally, which will ofcourse fail with -EEXIST.

* We can leak the kernfs_node for grp->name if some one tries to:
  - rename a group (change grp->name), or
  - update a named group, to an unnamed group

It appears that the whole purpose of sysfs_update_group() was to
allow changing the permissions or visibility of attributes and not
the names. So make it clear in the comments, and allow it to update
an existing named group.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
v2: Use pr_warn() instead of WARN()
v3: drop the extra reference taken by kernfs_find_and_get()

 fs/sysfs/group.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 4802ec0e1e3a..38240410f831 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -119,12 +119,22 @@ static int internal_create_group(struct kobject *kobj, int update,
 		return -EINVAL;
 	}
 	if (grp->name) {
-		kn = kernfs_create_dir(kobj->sd, grp->name,
-				       S_IRWXU | S_IRUGO | S_IXUGO, kobj);
-		if (IS_ERR(kn)) {
-			if (PTR_ERR(kn) == -EEXIST)
-				sysfs_warn_dup(kobj->sd, grp->name);
-			return PTR_ERR(kn);
+		if (update) {
+			kn = kernfs_find_and_get(kobj->sd, grp->name);
+			if (!kn) {
+				pr_warn("Can't update unknown attr grp name: %s/%s\n",
+					kobj->name, grp->name);
+				return -EINVAL;
+			}
+		} else {
+			kn = kernfs_create_dir(kobj->sd, grp->name,
+					       S_IRWXU | S_IRUGO | S_IXUGO,
+					       kobj);
+			if (IS_ERR(kn)) {
+				if (PTR_ERR(kn) == -EEXIST)
+					sysfs_warn_dup(kobj->sd, grp->name);
+				return PTR_ERR(kn);
+			}
 		}
 	} else
 		kn = kobj->sd;
@@ -135,6 +145,10 @@ static int internal_create_group(struct kobject *kobj, int update,
 			kernfs_remove(kn);
 	}
 	kernfs_put(kn);
+
+	if (grp->name && update)
+		kernfs_put(kn);
+
 	return error;
 }
 
@@ -199,7 +213,8 @@ EXPORT_SYMBOL_GPL(sysfs_create_groups);
  * of the attribute files being created already exist.  Furthermore,
  * if the visibility of the files has changed through the is_visible()
  * callback, it will update the permissions and add or remove the
- * relevant files.
+ * relevant files. Changing a group's name (subdirectory name under
+ * kobj's directory in sysfs) is not allowed.
  *
  * The primary use for this function is to call it after making a change
  * that affects group visibility.
-- 
2.18.0.rc1.244.gcf134e6275-goog