linux-kernel - Re: [PATCH] sysfs:Addresses null pointer dereference in sysfs_merge_group and sysfs_unmerge

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] [day] [month] [year] [list]

Message-ID: <2024020946-reexamine-unwitting-6277@gregkh>
Date: Fri, 9 Feb 2024 10:27:56 +0000
From: Greg KH <gregkh@...uxfoundation.org>
To: Rohan Kollambalath <rohankollambalath@...il.com>
Cc: linux-kernel@...r.kernel.org, Rohan Kollambalath <rkollamb@...i.com>
Subject: Re: [PATCH] sysfs:Addresses null pointer dereference in
 sysfs_merge_group and sysfs_unmerge_group.

On Fri, Feb 09, 2024 at 09:36:26AM +1000, Rohan Kollambalath wrote:
> From: Rohan Kollambalath <rkollamb@...i.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@...i.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