Re: [PATCH 2/4] module: sysfs: Simplify section attribute allocation

[Petr Pavlu <petr.pavlu@suse.com>]

On 12/16/24 20:16, Thomas Weißschuh wrote:
> The existing allocation logic manually stuffs two allocations into one.
> This is hard to understand and of limited value, given that all the
> section names are allocated on their own anyways.
> Une one allocation per datastructure.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  kernel/module/sysfs.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
> index b7841f76a933114e6dbd0fc2d32a60b66b7966b6..935629ac21fa16504ddb5f3762af5539572c3bf1 100644
> --- a/kernel/module/sysfs.c
> +++ b/kernel/module/sysfs.c
> @@ -65,34 +65,37 @@ static void free_sect_attrs(struct module_sect_attrs *sect_attrs)
>  
>  	for (bin_attr = sect_attrs->grp.bin_attrs; *bin_attr; bin_attr++)
>  		kfree((*bin_attr)->attr.name);
> +	kfree(sect_attrs->grp.bin_attrs);
>  	kfree(sect_attrs);
>  }
>  
>  static int add_sect_attrs(struct module *mod, const struct load_info *info)
>  {
> -	unsigned int nloaded = 0, i, size[2];
>  	struct module_sect_attrs *sect_attrs;
>  	struct module_sect_attr *sattr;
>  	struct bin_attribute **gattr;
> +	unsigned int nloaded = 0, i;
>  	int ret;
>  
>  	/* Count loaded sections and allocate structures */
>  	for (i = 0; i < info->hdr->e_shnum; i++)
>  		if (!sect_empty(&info->sechdrs[i]))
>  			nloaded++;
> -	size[0] = ALIGN(struct_size(sect_attrs, attrs, nloaded),
> -			sizeof(sect_attrs->grp.bin_attrs[0]));
> -	size[1] = (nloaded + 1) * sizeof(sect_attrs->grp.bin_attrs[0]);
> -	sect_attrs = kzalloc(size[0] + size[1], GFP_KERNEL);
> +	sect_attrs = kzalloc(struct_size(sect_attrs, attrs, nloaded), GFP_KERNEL);
>  	if (!sect_attrs)
>  		return -ENOMEM;
>  
> +	gattr = kcalloc(nloaded + 1, sizeof(*gattr), GFP_KERNEL);
> +	if (!gattr) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +

Member sect_attrs->grp.bin_attrs is NULL at this point. If the above
kcalloc() call fails, the control goes to the out label which invokes
free_sect_attrs() and its code
"for (bin_attr = sect_attrs->grp.bin_attrs; *bin_attr; ..."
results in a NULL dereference.

>  	/* Setup section attributes. */
>  	sect_attrs->grp.name = "sections";
> -	sect_attrs->grp.bin_attrs = (void *)sect_attrs + size[0];
> +	sect_attrs->grp.bin_attrs = gattr;
>  
>  	sattr = &sect_attrs->attrs[0];
> -	gattr = &sect_attrs->grp.bin_attrs[0];
>  	for (i = 0; i < info->hdr->e_shnum; i++) {
>  		Elf_Shdr *sec = &info->sechdrs[i];
>  
> @@ -111,7 +114,6 @@ static int add_sect_attrs(struct module *mod, const struct load_info *info)
>  		sattr->battr.attr.mode = 0400;
>  		*(gattr++) = &(sattr++)->battr;
>  	}
> -	*gattr = NULL;
>  
>  	ret = sysfs_create_group(&mod->mkobj.kobj, &sect_attrs->grp);
>  	if (ret)
> 

-- 
Thanks,
Petr