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

[Petr Pavlu]

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 <li...@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