When a module is loaded, klp_module_coming() iterates over patches and calls klp_init_object_loaded(). If initialization fails, it delegates cleanup to klp_cleanup_module_patches_limited().
However, the cleanup loop skips the failing patch. Each function called in klp_init_object_loaded() is supposed to clean its own changes. This works except for the changes done by klp_init_object_loaded(). The current code is a bit messy. The changes done by klp_init_object_loaded() should get cleared by klp_free_object_loaded(). But this function also clears obj->mod which is set by klp_module_coming(). And relocations are cleared separately. Fix the situations by updating klp_free_object_loaded(). It should revert all and only changes made by klp_init_object_loaded(). This requires some shuffling: + Clear obj->mod explicitly in klp_cleanup_module_patches_limited() and do not rely on klp_free_object_loaded(). + Clear relocations in klp_free_object_loaded(). Remove the explicit call from klp_cleanup_module_patches_limited(). This requires adding the @patch parameter. Finally, call klp_free_object_loaded() in the error path in klp_init_object_loaded(). Reported-by: [email protected] Closes: https://lore.kernel.org/r/[email protected] Signed-off-by: Petr Mladek <[email protected]> --- kernel/livepatch/core.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index cdb25949f73b..1e59a3cc0895 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -725,18 +725,20 @@ static void __klp_free_funcs(struct klp_object *obj, bool nops_only) } /* Clean up when a patched object is unloaded */ -static void klp_free_object_loaded(struct klp_object *obj) +static void klp_free_object_loaded(struct klp_patch *patch, + struct klp_object *obj) { struct klp_func *func; - obj->mod = NULL; - klp_for_each_func(obj, func) { func->old_func = NULL; if (func->nop) func->new_func = NULL; } + + if (klp_is_module(obj)) + klp_clear_object_relocs(patch, obj); } static void __klp_free_objects(struct klp_patch *patch, bool nops_only) @@ -875,7 +877,7 @@ static int klp_init_object_loaded(struct klp_patch *patch, */ ret = klp_apply_object_relocs(patch, obj); if (ret) - return ret; + goto err; } klp_for_each_func(obj, func) { @@ -883,7 +885,7 @@ static int klp_init_object_loaded(struct klp_patch *patch, func->old_sympos, (unsigned long *)&func->old_func); if (ret) - return ret; + goto err; /* * Aliased symbols share one address, so they would resolve to @@ -896,7 +898,8 @@ static int klp_init_object_loaded(struct klp_patch *patch, if (prev_func->old_func == func->old_func) { pr_err("'%s' and '%s' resolve to the same address, aliased symbols are not supported\n", prev_func->old_name, func->old_name); - return -EINVAL; + ret = -EINVAL; + goto err; } } @@ -905,7 +908,8 @@ static int klp_init_object_loaded(struct klp_patch *patch, if (!ret) { pr_err("kallsyms size lookup failed for '%s'\n", func->old_name); - return -ENOENT; + ret = -ENOENT; + goto err; } if (func->nop) @@ -916,11 +920,17 @@ static int klp_init_object_loaded(struct klp_patch *patch, if (!ret) { pr_err("kallsyms size lookup failed for '%s' replacement\n", func->old_name); - return -ENOENT; + ret = -ENOENT; + goto err; } } return 0; + +err: + klp_free_object_loaded(patch, obj); + + return ret; } static int klp_init_object(struct klp_patch *patch, struct klp_object *obj) @@ -1274,8 +1284,8 @@ static void klp_cleanup_module_patches_limited(struct module *mod, klp_unpatch_object(obj); klp_post_unpatch_callback(obj); - klp_clear_object_relocs(patch, obj); - klp_free_object_loaded(obj); + klp_free_object_loaded(patch, obj); + obj->mod = NULL; break; } } -- 2.55.0
