Re: [RFC PATCH 5/6] livepatch: Remove obsolete per-object callbacks

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