[PATCH] module: abort module loading when sysfs setup suffer errors

[PATCH] module: abort module loading when sysfs setup suffer errors

[Chunhui Li]

When insmod a kernel module, if fails in add_notes_attrs or
add_sysfs_attrs such as memory allocation fail, mod_sysfs_setup
will still return success, but we can't access user interface
on android device.

Patch for make mod_sysfs_setup can check the error of
add_notes_attrs and add_sysfs_attrs

Signed-off-by: Xion Wang <xion.wang@mediatek.com>
Signed-off-by: Chunhui Li <chunhui.li@mediatek.com>
---
 kernel/module/sysfs.c | 49 ++++++++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 14 deletions(-)

diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
index 26efe1305c12..a9ee650d995d 100644
--- a/kernel/module/sysfs.c
+++ b/kernel/module/sysfs.c
@@ -69,12 +69,13 @@ static void free_sect_attrs(struct module_sect_attrs *sect_attrs)
 	kfree(sect_attrs);
 }
 
-static void add_sect_attrs(struct module *mod, const struct load_info *info)
+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;
+	int ret = 0;
 
 	/* Count loaded sections and allocate structures */
 	for (i = 0; i < info->hdr->e_shnum; i++)
@@ -85,7 +86,7 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info)
 	size[1] = (nloaded + 1) * sizeof(sect_attrs->grp.bin_attrs[0]);
 	sect_attrs = kzalloc(size[0] + size[1], GFP_KERNEL);
 	if (!sect_attrs)
-		return;
+		return -ENOMEM;
 
 	/* Setup section attributes. */
 	sect_attrs->grp.name = "sections";
@@ -103,8 +104,10 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info)
 		sattr->address = sec->sh_addr;
 		sattr->battr.attr.name =
 			kstrdup(info->secstrings + sec->sh_name, GFP_KERNEL);
-		if (!sattr->battr.attr.name)
+		if (!sattr->battr.attr.name) {
+			ret = -ENOMEM;
 			goto out;
+		}
 		sect_attrs->nsections++;
 		sattr->battr.read = module_sect_read;
 		sattr->battr.size = MODULE_SECT_READ_SIZE;
@@ -113,13 +116,16 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info)
 	}
 	*gattr = NULL;
 
-	if (sysfs_create_group(&mod->mkobj.kobj, &sect_attrs->grp))
+	if (sysfs_create_group(&mod->mkobj.kobj, &sect_attrs->grp)) {
+		ret = -EIO;
 		goto out;
+	}
 
 	mod->sect_attrs = sect_attrs;
-	return;
+	return 0;
 out:
 	free_sect_attrs(sect_attrs);
+	return ret;
 }
 
 static void remove_sect_attrs(struct module *mod)
@@ -158,15 +164,16 @@ static void free_notes_attrs(struct module_notes_attrs *notes_attrs,
 	kfree(notes_attrs);
 }
 
-static void add_notes_attrs(struct module *mod, const struct load_info *info)
+static int add_notes_attrs(struct module *mod, const struct load_info *info)
 {
 	unsigned int notes, loaded, i;
 	struct module_notes_attrs *notes_attrs;
 	struct bin_attribute *nattr;
+	int ret = 0;
 
 	/* failed to create section attributes, so can't create notes */
 	if (!mod->sect_attrs)
-		return;
+		return -EINVAL;
 
 	/* Count notes sections and allocate structures.  */
 	notes = 0;
@@ -176,12 +183,12 @@ static void add_notes_attrs(struct module *mod, const struct load_info *info)
 			++notes;
 
 	if (notes == 0)
-		return;
+		return 0;
 
 	notes_attrs = kzalloc(struct_size(notes_attrs, attrs, notes),
 			      GFP_KERNEL);
 	if (!notes_attrs)
-		return;
+		return -ENOMEM;
 
 	notes_attrs->notes = notes;
 	nattr = &notes_attrs->attrs[0];
@@ -201,19 +208,24 @@ static void add_notes_attrs(struct module *mod, const struct load_info *info)
 	}
 
 	notes_attrs->dir = kobject_create_and_add("notes", &mod->mkobj.kobj);
-	if (!notes_attrs->dir)
+	if (!notes_attrs->dir) {
+		ret = -ENOMEM;
 		goto out;
+	}
 
 	for (i = 0; i < notes; ++i)
 		if (sysfs_create_bin_file(notes_attrs->dir,
-					  &notes_attrs->attrs[i]))
+					  &notes_attrs->attrs[i])) {
+			ret = -EIO;
 			goto out;
+	}
 
 	mod->notes_attrs = notes_attrs;
-	return;
+	return 0;
 
 out:
 	free_notes_attrs(notes_attrs, i);
+	return ret;
 }
 
 static void remove_notes_attrs(struct module *mod)
@@ -385,11 +397,20 @@ int mod_sysfs_setup(struct module *mod,
 	if (err)
 		goto out_unreg_modinfo_attrs;
 
-	add_sect_attrs(mod, info);
-	add_notes_attrs(mod, info);
+	err = add_sect_attrs(mod, info);
+	if (err)
+		goto out_unreg_sect_attrs;
+
+	err = add_notes_attrs(mod, info);
+	if (err)
+		goto out_unreg_notes_attrs;
 
 	return 0;
 
+out_unreg_notes_attrs:
+	remove_notes_attrs(mod);
+out_unreg_sect_attrs:
+	remove_sect_attrs(mod);
 out_unreg_modinfo_attrs:
 	module_remove_modinfo_attrs(mod, -1);
 out_unreg_param:
-- 
2.45.2

Re: [PATCH] module: abort module loading when sysfs setup suffer errors

[kernel test robot]

Hi Chunhui,

kernel test robot noticed the following build errors:

[auto build test ERROR on mcgrof/modules-next]
[also build test ERROR on linus/master v6.11-rc5 next-20240830]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Chunhui-Li/module-abort-module-loading-when-sysfs-setup-suffer-errors/20240830-134417
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next
patch link:    https://lore.kernel.org/r/20240830054400.26622-1-chunhui.li%40mediatek.com
patch subject: [PATCH] module: abort module loading when sysfs setup suffer errors
config: openrisc-defconfig (https://download.01.org/0day-ci/archive/20240901/202409010016.3XIFSmRA-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240901/202409010016.3XIFSmRA-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409010016.3XIFSmRA-lkp@intel.com/

All errors (new ones prefixed by >>):

   kernel/module/sysfs.c: In function 'mod_sysfs_setup':
>> kernel/module/sysfs.c:400:13: error: void value not ignored as it ought to be
     400 |         err = add_sect_attrs(mod, info);
         |             ^
   kernel/module/sysfs.c:404:13: error: void value not ignored as it ought to be
     404 |         err = add_notes_attrs(mod, info);
         |             ^


vim +400 kernel/module/sysfs.c

   370	
   371	int mod_sysfs_setup(struct module *mod,
   372			    const struct load_info *info,
   373				   struct kernel_param *kparam,
   374				   unsigned int num_params)
   375	{
   376		int err;
   377	
   378		err = mod_sysfs_init(mod);
   379		if (err)
   380			goto out;
   381	
   382		mod->holders_dir = kobject_create_and_add("holders", &mod->mkobj.kobj);
   383		if (!mod->holders_dir) {
   384			err = -ENOMEM;
   385			goto out_unreg;
   386		}
   387	
   388		err = module_param_sysfs_setup(mod, kparam, num_params);
   389		if (err)
   390			goto out_unreg_holders;
   391	
   392		err = module_add_modinfo_attrs(mod);
   393		if (err)
   394			goto out_unreg_param;
   395	
   396		err = add_usage_links(mod);
   397		if (err)
   398			goto out_unreg_modinfo_attrs;
   399	
 > 400		err = add_sect_attrs(mod, info);
   401		if (err)
   402			goto out_unreg_sect_attrs;
   403	
   404		err = add_notes_attrs(mod, info);
   405		if (err)
   406			goto out_unreg_notes_attrs;
   407	
   408		return 0;
   409	
   410	out_unreg_notes_attrs:
   411		remove_notes_attrs(mod);
   412	out_unreg_sect_attrs:
   413		remove_sect_attrs(mod);
   414	out_unreg_modinfo_attrs:
   415		module_remove_modinfo_attrs(mod, -1);
   416	out_unreg_param:
   417		module_param_sysfs_remove(mod);
   418	out_unreg_holders:
   419		kobject_put(mod->holders_dir);
   420	out_unreg:
   421		mod_kobject_put(mod);
   422	out:
   423		return err;
   424	}
   425	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] module: abort module loading when sysfs setup suffer errors

[kernel test robot]

Hi Chunhui,

kernel test robot noticed the following build errors:

[auto build test ERROR on mcgrof/modules-next]
[also build test ERROR on linus/master v6.11-rc5 next-20240830]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Chunhui-Li/module-abort-module-loading-when-sysfs-setup-suffer-errors/20240830-134417
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next
patch link:    https://lore.kernel.org/r/20240830054400.26622-1-chunhui.li%40mediatek.com
patch subject: [PATCH] module: abort module loading when sysfs setup suffer errors
config: powerpc-mpc834x_itx_defconfig (https://download.01.org/0day-ci/archive/20240901/202409010209.FqYOzYEa-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 46fe36a4295f05d5d3731762e31fc4e6e99863e9)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240901/202409010209.FqYOzYEa-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409010209.FqYOzYEa-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from kernel/module/sysfs.c:13:
   In file included from include/linux/kallsyms.h:13:
   In file included from include/linux/mm.h:2228:
   include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     517 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> kernel/module/sysfs.c:400:6: error: assigning to 'int' from incompatible type 'void'
     400 |         err = add_sect_attrs(mod, info);
         |             ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
   kernel/module/sysfs.c:404:6: error: assigning to 'int' from incompatible type 'void'
     404 |         err = add_notes_attrs(mod, info);
         |             ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning and 2 errors generated.


vim +400 kernel/module/sysfs.c

   370	
   371	int mod_sysfs_setup(struct module *mod,
   372			    const struct load_info *info,
   373				   struct kernel_param *kparam,
   374				   unsigned int num_params)
   375	{
   376		int err;
   377	
   378		err = mod_sysfs_init(mod);
   379		if (err)
   380			goto out;
   381	
   382		mod->holders_dir = kobject_create_and_add("holders", &mod->mkobj.kobj);
   383		if (!mod->holders_dir) {
   384			err = -ENOMEM;
   385			goto out_unreg;
   386		}
   387	
   388		err = module_param_sysfs_setup(mod, kparam, num_params);
   389		if (err)
   390			goto out_unreg_holders;
   391	
   392		err = module_add_modinfo_attrs(mod);
   393		if (err)
   394			goto out_unreg_param;
   395	
   396		err = add_usage_links(mod);
   397		if (err)
   398			goto out_unreg_modinfo_attrs;
   399	
 > 400		err = add_sect_attrs(mod, info);
   401		if (err)
   402			goto out_unreg_sect_attrs;
   403	
   404		err = add_notes_attrs(mod, info);
   405		if (err)
   406			goto out_unreg_notes_attrs;
   407	
   408		return 0;
   409	
   410	out_unreg_notes_attrs:
   411		remove_notes_attrs(mod);
   412	out_unreg_sect_attrs:
   413		remove_sect_attrs(mod);
   414	out_unreg_modinfo_attrs:
   415		module_remove_modinfo_attrs(mod, -1);
   416	out_unreg_param:
   417		module_param_sysfs_remove(mod);
   418	out_unreg_holders:
   419		kobject_put(mod->holders_dir);
   420	out_unreg:
   421		mod_kobject_put(mod);
   422	out:
   423		return err;
   424	}
   425	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] module: abort module loading when sysfs setup suffer errors

[Petr Pavlu]

On 8/30/24 07:43, Chunhui Li wrote:
> When insmod a kernel module, if fails in add_notes_attrs or
> add_sysfs_attrs such as memory allocation fail, mod_sysfs_setup
> will still return success, but we can't access user interface
> on android device.
> 
> Patch for make mod_sysfs_setup can check the error of
> add_notes_attrs and add_sysfs_attrs

s/add_sysfs_attrs/add_sect_attrs/

I think it makes sense to propagate errors from these functions upward,
although I wonder if the authors of this code didn't intentionally make
the errors silent, possibly because the interface was mostly intended
for debugging only?

The original commits which added add_sect_attrs() and add_notes_attrs()
don't mention anything explicitly in this regard:
https://github.com/mpe/linux-fullhistory/commit/db939b519bea9b88ae1c95c3b479c0b07145f2a0
https://github.com/torvalds/linux/commit/6d76013381ed28979cd122eb4b249a88b5e384fa

> 
> Signed-off-by: Xion Wang <xion.wang@mediatek.com>
> Signed-off-by: Chunhui Li <chunhui.li@mediatek.com>
> ---
>  kernel/module/sysfs.c | 49 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
> index 26efe1305c12..a9ee650d995d 100644
> --- a/kernel/module/sysfs.c
> +++ b/kernel/module/sysfs.c
> @@ -69,12 +69,13 @@ static void free_sect_attrs(struct module_sect_attrs *sect_attrs)
>  	kfree(sect_attrs);
>  }
>  
> -static void add_sect_attrs(struct module *mod, const struct load_info *info)
> +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;
> +	int ret = 0;

Nit: It isn't necessary to initialize this variable to 0 because the
code explicitly does "return 0;" on success. While on the error path,
the variable is always assigned.

>  
>  	/* Count loaded sections and allocate structures */
>  	for (i = 0; i < info->hdr->e_shnum; i++)
> @@ -85,7 +86,7 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info)
>  	size[1] = (nloaded + 1) * sizeof(sect_attrs->grp.bin_attrs[0]);
>  	sect_attrs = kzalloc(size[0] + size[1], GFP_KERNEL);
>  	if (!sect_attrs)
> -		return;
> +		return -ENOMEM;
>  
>  	/* Setup section attributes. */
>  	sect_attrs->grp.name = "sections";
> @@ -103,8 +104,10 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info)
>  		sattr->address = sec->sh_addr;
>  		sattr->battr.attr.name =
>  			kstrdup(info->secstrings + sec->sh_name, GFP_KERNEL);
> -		if (!sattr->battr.attr.name)
> +		if (!sattr->battr.attr.name) {
> +			ret = -ENOMEM;
>  			goto out;
> +		}
>  		sect_attrs->nsections++;
>  		sattr->battr.read = module_sect_read;
>  		sattr->battr.size = MODULE_SECT_READ_SIZE;
> @@ -113,13 +116,16 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info)
>  	}
>  	*gattr = NULL;
>  
> -	if (sysfs_create_group(&mod->mkobj.kobj, &sect_attrs->grp))
> +	if (sysfs_create_group(&mod->mkobj.kobj, &sect_attrs->grp)) {
> +		ret = -EIO;
>  		goto out;
> +	}

Why does the logic return -EIO instead of propagating the error code
from sysfs_create_group()?

>  
>  	mod->sect_attrs = sect_attrs;
> -	return;
> +	return 0;
>  out:
>  	free_sect_attrs(sect_attrs);
> +	return ret;
>  }
>  
>  static void remove_sect_attrs(struct module *mod)
> @@ -158,15 +164,16 @@ static void free_notes_attrs(struct module_notes_attrs *notes_attrs,
>  	kfree(notes_attrs);
>  }
>  
> -static void add_notes_attrs(struct module *mod, const struct load_info *info)
> +static int add_notes_attrs(struct module *mod, const struct load_info *info)
>  {
>  	unsigned int notes, loaded, i;
>  	struct module_notes_attrs *notes_attrs;
>  	struct bin_attribute *nattr;
> +	int ret = 0;

Similarly here, the initialization is not necessary.

>  
>  	/* failed to create section attributes, so can't create notes */
>  	if (!mod->sect_attrs)
> -		return;
> +		return -EINVAL;

Since the patch modifies mod_sysfs_setup() to bail out when registering
section attributes fails, this condition can no longer be true and the
check can be removed.

>  
>  	/* Count notes sections and allocate structures.  */
>  	notes = 0;
> @@ -176,12 +183,12 @@ static void add_notes_attrs(struct module *mod, const struct load_info *info)
>  			++notes;
>  
>  	if (notes == 0)
> -		return;
> +		return 0;
>  
>  	notes_attrs = kzalloc(struct_size(notes_attrs, attrs, notes),
>  			      GFP_KERNEL);
>  	if (!notes_attrs)
> -		return;
> +		return -ENOMEM;
>  
>  	notes_attrs->notes = notes;
>  	nattr = &notes_attrs->attrs[0];
> @@ -201,19 +208,24 @@ static void add_notes_attrs(struct module *mod, const struct load_info *info)
>  	}
>  
>  	notes_attrs->dir = kobject_create_and_add("notes", &mod->mkobj.kobj);
> -	if (!notes_attrs->dir)
> +	if (!notes_attrs->dir) {
> +		ret = -ENOMEM;
>  		goto out;
> +	}
>  
>  	for (i = 0; i < notes; ++i)
>  		if (sysfs_create_bin_file(notes_attrs->dir,
> -					  &notes_attrs->attrs[i]))
> +					  &notes_attrs->attrs[i])) {
> +			ret = -EIO;
>  			goto out;
> +	}

Similarly here, the actual error from sysfs_create_bin_file() can be
returned.

>  
>  	mod->notes_attrs = notes_attrs;
> -	return;
> +	return 0;
>  
>  out:
>  	free_notes_attrs(notes_attrs, i);
> +	return ret;
>  }
>  
>  static void remove_notes_attrs(struct module *mod)
> @@ -385,11 +397,20 @@ int mod_sysfs_setup(struct module *mod,
>  	if (err)
>  		goto out_unreg_modinfo_attrs;
>  
> -	add_sect_attrs(mod, info);
> -	add_notes_attrs(mod, info);
> +	err = add_sect_attrs(mod, info);
> +	if (err)
> +		goto out_unreg_sect_attrs;
> +
> +	err = add_notes_attrs(mod, info);
> +	if (err)
> +		goto out_unreg_notes_attrs;
>  
>  	return 0;
>  
> +out_unreg_notes_attrs:
> +	remove_notes_attrs(mod);
> +out_unreg_sect_attrs:
> +	remove_sect_attrs(mod);

Upon a failure from add_sect_attrs(), the caller doesn't need to unwind
its operation. It is the responsibility of add_sect_attrs() to clean up
after itself on error. Instead, the code in mod_sysfs_setup() needs to
unwind all previous successful operations leading up to this point,
which means here additionally to invoke del_usage_links().

I think you want something as follows:

err = add_sect_attrs(mod, info);
if (err)
	goto out_unreg_usage_links;

err = add_notes_attrs(mod, info);
if (err)
	goto out_unreg_sect_attrs;

return 0;

out_unreg_sect_attrs:
	remove_sect_attrs(mod);
out_unreg_usage_links:
	del_usage_links(mod);
[...]

>  out_unreg_modinfo_attrs:
>  	module_remove_modinfo_attrs(mod, -1);
>  out_unreg_param:

-- 
Cheers,
Petr