[PATCH] sysfs:Addresses null pointer dereference in sysfs_merge_group and sysfs_unmerge_group.

[PATCH] sysfs:Addresses null pointer dereference in sysfs_merge_group and sysfs_unmerge_group.

[Rohan Kollambalath]

From: Rohan Kollambalath <rkollamb@digi.com>

These functions take a struct attribute_group as an input which has an
optional .name field. These functions rely on the .name field being
populated and do not check if its null. They pass this name into other
functions, eventually leading to a null pointer dereference.

This change adds a simple check that returns an error if the .name field
is null and clarifies this requirement in the comments.

Signed-off-by: Rohan Kollambalath <rkollamb@digi.com>
---
 fs/sysfs/group.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 138676463336..a221de8c95a2 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -318,12 +318,12 @@ void sysfs_remove_groups(struct kobject *kobj,
 EXPORT_SYMBOL_GPL(sysfs_remove_groups);
 
 /**
- * sysfs_merge_group - merge files into a pre-existing attribute group.
+ * sysfs_merge_group - merge files into a pre-existing named attribute group.
  * @kobj:	The kobject containing the group.
  * @grp:	The files to create and the attribute group they belong to.
  *
- * This function returns an error if the group doesn't exist or any of the
- * files already exist in that group, in which case none of the new files
+ * This function returns an error if the group doesn't exist, the .name field is NULL or
+ * any of the files already exist in that group, in which case none of the new files
  * are created.
  */
 int sysfs_merge_group(struct kobject *kobj,
@@ -336,6 +336,9 @@ int sysfs_merge_group(struct kobject *kobj,
 	struct attribute *const *attr;
 	int i;
 
+	if (!grp->name)
+		return -ENOENT;
+
 	parent = kernfs_find_and_get(kobj->sd, grp->name);
 	if (!parent)
 		return -ENOENT;
@@ -356,7 +359,7 @@ int sysfs_merge_group(struct kobject *kobj,
 EXPORT_SYMBOL_GPL(sysfs_merge_group);
 
 /**
- * sysfs_unmerge_group - remove files from a pre-existing attribute group.
+ * sysfs_unmerge_group - remove files from a pre-existing named attribute group.
  * @kobj:	The kobject containing the group.
  * @grp:	The files to remove and the attribute group they belong to.
  */
@@ -366,6 +369,9 @@ void sysfs_unmerge_group(struct kobject *kobj,
 	struct kernfs_node *parent;
 	struct attribute *const *attr;
 
+	if (!grp->name)
+		return -ENOENT;
+
 	parent = kernfs_find_and_get(kobj->sd, grp->name);
 	if (parent) {
 		for (attr = grp->attrs; *attr; ++attr)
-- 
2.25.1

Re: [PATCH] sysfs:Addresses null pointer dereference in sysfs_merge_group and sysfs_unmerge_group.

[Greg KH]

On Fri, Feb 09, 2024 at 09:36:26AM +1000, Rohan Kollambalath wrote:
> From: Rohan Kollambalath <rkollamb@digi.com>
> 
> These functions take a struct attribute_group as an input which has an
> optional .name field. These functions rely on the .name field being
> populated and do not check if its null. They pass this name into other
> functions, eventually leading to a null pointer dereference.

What in-kernel drivers cause this to trigger?  Why not fix them up
instead?

> This change adds a simple check that returns an error if the .name field
> is null and clarifies this requirement in the comments.
> 
> Signed-off-by: Rohan Kollambalath <rkollamb@digi.com>
> ---
>  fs/sysfs/group.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 138676463336..a221de8c95a2 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -318,12 +318,12 @@ void sysfs_remove_groups(struct kobject *kobj,
>  EXPORT_SYMBOL_GPL(sysfs_remove_groups);
>  
>  /**
> - * sysfs_merge_group - merge files into a pre-existing attribute group.
> + * sysfs_merge_group - merge files into a pre-existing named attribute group.
>   * @kobj:	The kobject containing the group.
>   * @grp:	The files to create and the attribute group they belong to.
>   *
> - * This function returns an error if the group doesn't exist or any of the
> - * files already exist in that group, in which case none of the new files
> + * This function returns an error if the group doesn't exist, the .name field is NULL or
> + * any of the files already exist in that group, in which case none of the new files

Please properly wrap comments at the correct column.

>   * are created.
>   */
>  int sysfs_merge_group(struct kobject *kobj,
> @@ -336,6 +336,9 @@ int sysfs_merge_group(struct kobject *kobj,
>  	struct attribute *const *attr;
>  	int i;
>  
> +	if (!grp->name)
> +		return -ENOENT;

Why that error value?


> +
>  	parent = kernfs_find_and_get(kobj->sd, grp->name);
>  	if (!parent)
>  		return -ENOENT;
> @@ -356,7 +359,7 @@ int sysfs_merge_group(struct kobject *kobj,
>  EXPORT_SYMBOL_GPL(sysfs_merge_group);
>  
>  /**
> - * sysfs_unmerge_group - remove files from a pre-existing attribute group.
> + * sysfs_unmerge_group - remove files from a pre-existing named attribute group.
>   * @kobj:	The kobject containing the group.
>   * @grp:	The files to remove and the attribute group they belong to.
>   */
> @@ -366,6 +369,9 @@ void sysfs_unmerge_group(struct kobject *kobj,
>  	struct kernfs_node *parent;
>  	struct attribute *const *attr;
>  
> +	if (!grp->name)
> +		return -ENOENT;

Again, why that value?

And again, why not fix up the callers?

thanks,

greg k-h

Re: [PATCH] sysfs:Addresses null pointer dereference in sysfs_merge_group and sysfs_unmerge_group.

[Greg KH]

On Sat, Feb 10, 2024 at 08:43:14PM +1000, Rohan wrote:
> Hey,

Please don't top-post, or send html email, as they get dropped from the
mailing lists.

> I found this bug when writing my own kernel driver. I believed the function
> wasn't explicit in stating its the requirement for the name field and made
> it easy for developers to mistake it with sysfs_create_group and cause a
> crash. I wanted to change the code for robustness purposes. Other functions
> in the same file already do this check such as sysfs_group_change_owner.

Adding documentation is great, but don't add checks that are not needed,
we require callers to set up things properly, otherwise the kernel would
be nothing but a constant set of checks for things that can never
happen.

> As for why I chose -ENOENT, I did this because it was the same error code
> returned if the group cannot be found by kernfs_find_and_get but I
> understand if this should be changed.

Yes, that is a different type of error entirely.  Just do the
documentation update, I'll gladly take that.

thanks,

greg k-h