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@xxxxxxxx>
> 
> 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@xxxxxxxx>
> ---
>  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