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? 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? > 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? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
