Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Initialization failures in klp_init_object_loaded() leave livepatch module relocations uncleared, leading to state corruption and permanently preventing target module loading. -- --- Patch [1]: [PATCH v2] livepatch: Reject livepatches with aliased old_func --- commit 230a471112d181b2e9810c9877ffebb05a42d61c Author: Harry Hsu <[email protected]> livepatch: Reject livepatches with aliased old_func This patch adds a check during livepatch initialization to reject configurations where multiple functions resolve to the same alias address. It prevents ambiguity when the redirection logic evaluates the func_stack in the ftrace handler. Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index 28d15ba58a26d..c35cf08c27c86 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -866,7 +866,7 @@ static void klp_clear_object_relocs(struct klp_patch > *patch, > static int klp_init_object_loaded(struct klp_patch *patch, > struct klp_object *obj) > { > - struct klp_func *func; > + struct klp_func *func, *prev_func; > int ret; > > if (klp_is_module(obj)) { [ ... ] > @@ -888,6 +888,21 @@ static int klp_init_object_loaded(struct klp_patch > *patch, > if (ret) > return ret; > > + /* > + * Aliased symbols share one address, so they would resolve to > + * the same klp_ops and stack up on a single ops->func_stack, > + * leaving the redirection ambiguous. Reject the livepatch. > + */ > + klp_for_each_func(obj, prev_func) { > + if (prev_func == func) > + break; > + 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; [Severity: High] This is a pre-existing issue, but returning an error here leaves the module relocations applied earlier in this function uncleared. When a module is loaded, klp_module_coming() iterates over patches and calls klp_init_object_loaded(). If initialization fails, it delegates cleanup to the error path: kernel/livepatch/core.c:klp_module_coming() { ... ret = klp_init_object_loaded(patch, obj); if (ret) { ... goto err; } ... err: ... klp_cleanup_module_patches_limited(mod, patch); ... } However, the cleanup loop skips the failing patch entirely: kernel/livepatch/core.c:klp_cleanup_module_patches_limited() { ... klp_for_each_patch(patch) { if (patch == limit) break; ... klp_clear_object_relocs(patch, obj); } } Because it breaks when reaching the limit patch, klp_clear_object_relocs() is never called for the relocations that were successfully applied at the start of klp_init_object_loaded() via klp_apply_object_relocs(). Could this permanently corrupt the livepatch module's state and prevent the target module from safely loading again, since the relocations are never reverted? > + } > + } > + > ret = kallsyms_lookup_size_offset((unsigned long)func->old_func, > &func->old_size, NULL); > if (!ret) { -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
