On Fri, May 15, 2026 at 6:40 AM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s)
> to consider:
> - [Critical] Out-of-bounds ELF relocations causing memory corruption
> - [High] Silent failure of livepatch state callbacks due to missing
> initialization
> - [Low] Build failures due to unmigrated selftests and samples
> --
>
> commit deb761bd65e9a96a95798282bde72ddb362e1c0d
> Author: Yafang Shao <[email protected]>
>
> livepatch: Remove obsolete per-object callbacks
>
> This commit removes obsolete per-object callbacks from the livepatch framework
> in favor of per-state callbacks. It drops the callback fields from the klp
> object structures and updates objtool and the livepatch core scripts.
>
> > All selftests have been migrated to the new per-state callbacks, making the
> > per-object callbacks redundant.
>
> Is this description accurate?
Selftests are untouched in this RFC. I'll revise the commit log accordingly.
>
> The callbacks field was removed from struct klp_object, but if modules like
> samples/livepatch/livepatch-callbacks-demo.c or the selftests still use it
> in their klp_object declarations, won't this cause build failures?
>
> > diff --git a/scripts/livepatch/init.c b/scripts/livepatch/init.c
> > index 9265e0e504e0c..570366b9268c4 100644
> > --- a/scripts/livepatch/init.c
> > +++ b/scripts/livepatch/init.c
> > @@ -63,8 +63,6 @@ static int __init livepatch_mod_init(void)
> >
> > obj->name = obj_ext->name;
> > obj->funcs = funcs;
> > -
> > - memcpy(&obj->callbacks, &obj_ext->callbacks, sizeof(struct
> > klp_callbacks));
> > }
> >
> > patch->mod = THIS_MODULE;
>
> Will the state callbacks be executed when the patch is enabled?
>
> Looking at livepatch_mod_init() after these changes, patch->states is never
> initialized:
>
> scripts/livepatch/init.c:livepatch_mod_init() {
> ...
> patch->mod = THIS_MODULE;
> patch->objs = objs;
>
> /* TODO patch->states */
>
> patch->replace_set = KLP_REPLACE_TAG;
> return klp_enable_patch(patch);
> }
>
> When klp_states_pre_patch() iterates over patch->states, won't it silently
> skip the uninitialized callbacks?
Since these callbacks have been moved to klp_state, they must be
initialized first. Note that the KLP_*_PATCH_CALLBACK macros are
broken after patch #6; I will fix this in the next version.
>
> > diff --git a/tools/objtool/klp-diff.c b/tools/objtool/klp-diff.c
> > index 4d1f9e9977eb9..6e2762de3d512 100644
> > --- a/tools/objtool/klp-diff.c
> > +++ b/tools/objtool/klp-diff.c
> [ ... ]
> > @@ -1566,8 +1566,8 @@ static int create_klp_sections(struct elfs *e)
> > reloc = find_reloc_by_dest(e->out, sym->sec, sym->offset);
> >
> > if (!elf_create_reloc(e->out, obj_sec,
> > - offsetof(struct klp_object_ext,
> > callbacks) +
> > - offsetof(struct klp_callbacks,
> > pre_patch),
> > + offsetof(struct klp_state_ext,
> > callbacks) +
> > + offsetof(struct klp_state_callbacks,
> > pre_patch),
> > reloc->sym, reloc_addend(reloc),
> > R_ABS64))
> > return -1;
> > }
>
> Does this result in out-of-bounds memory writes during ELF relocation?
>
> Earlier in create_klp_sections(), obj_sec is allocated using the size of
> struct klp_object_ext, which is 24 bytes:
>
> tools/objtool/klp-diff.c:create_klp_sections() {
> size_t obj_size = sizeof(struct klp_object_ext);
> ...
> obj_sec = elf_create_section_pair(e->out, KLP_OBJECTS_SEC, obj_size,
> 0, 0);
>
> However, the new relocations are created using offsets from a completely
> different structure, struct klp_state_ext.
>
> Because offsetof(struct klp_state_ext, callbacks) is 16, and the callback
> offsets go up to 24, this applies relocations at offsets 16, 24, 32, and 40
> within obj_sec.
>
> Won't the relocation at offset 16 corrupt klp_object_ext.nr_funcs, and the
> subsequent relocations write entirely out of bounds of the 24-byte obj_sec
> buffer?
Nice catch. I will fix it in the next version.
--
Regards
Yafang
