| 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
| ||
|
Message-ID: <c50f5364-ca94-4d5b-be99-3a3ffcf79648@suse.com>
Date: Tue, 3 Sep 2024 13:22:58 +0200
From: Petr Pavlu <petr.pavlu@...e.com>
To: Chunhui Li <chunhui.li@...iatek.com>
Cc: Luis Chamberlain <mcgrof@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
wsd_upstream@...iatek.com, Xion Wang <xion.wang@...iatek.com>
Subject: Re: [PATCH] module: abort module loading when sysfs setup suffer
errors
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@...iatek.com>
> Signed-off-by: Chunhui Li <chunhui.li@...iatek.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, §_attrs->grp))
> + if (sysfs_create_group(&mod->mkobj.kobj, §_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 = ¬es_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,
> - ¬es_attrs->attrs[i]))
> + ¬es_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
Powered by blists - more mailing lists