On Sun, Jun 7, 2026 at 9:33 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [High] The newly introduced check in `klp_is_patch_compatible` causes a
> kernel panic if a livepatch module contains a `klp_func` entry with `old_name
> == NULL`.
> --
>
> commit ed678e88294b650a730cda1a47351fce99677c1e
> Author: Yafang Shao <[email protected]>
>
> livepatch: Support scoped atomic replace using replace_set
>
> Convert the replace attribute from a boolean to a u32 to function as a
> "replace set." A newly loaded livepatch will now atomically replace any
> existing patch belonging to the same set.
>
> Link: https://github.com/pmladek/linux/tree/klp-state-transfer-v1-iter12 [0]
>
> [ ... ]
> > --- a/kernel/livepatch/state.c
> > +++ b/kernel/livepatch/state.c
> [ ... ]
> > + /*
> > + * Refuse loading a livepatch which would want to modify a
> > + * function which is already livepatched with the livepatch
> > + * with another "replace_set".
> > + */
> > + klp_for_each_object_static(patch, obj) {
> > + klp_for_each_object(old_patch, old_obj) {
> > + if (!!obj->name != !!old_obj->name)
> > + continue;
> > + if (obj->name && strcmp(obj->name,
> > old_obj->name))
> > + continue;
> > + klp_for_each_func_static(obj, func) {
> > + if (klp_find_func(old_obj, func))
> > + return false;
> > + }
> > + }
> > + }
> > }
>
> [Severity: High]
> Can passing an unvalidated function to klp_find_func() cause a NULL pointer
> dereference here?
>
> If a new livepatch module is loaded with an improperly initialized klp_func
> array (e.g., accidentally missing old_name but providing new_func),
> klp_for_each_func_static() will still process it because structural
> validation in klp_init_patch() hasn't occurred yet at this point in the
> loading process.
>
> When this unvalidated func is passed as the second argument to
> klp_find_func(old_obj, func), the roles inside klp_find_func() are reversed
> from its usual context. The old_func parameter represents the new patch's
> unvalidated function, while the local func iterator represents the
> already-installed patch.
>
> Inside klp_find_func(), the check evaluates:
>
> if ((func->old_name && strcmp(old_func->old_name, func->old_name) == 0)
>
> Since the installed patch's func->old_name is guaranteed to be non-NULL,
> strcmp() will be called with old_func->old_name. If old_func->old_name is
> NULL, does this turn what would normally be a graceful -EINVAL module
> rejection into a system crash?
Oops, I missed that. It seems we should better check both func->name
and old_func->name in patch #1.
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -82,20 +82,22 @@ static bool klp_initialized(void)
return !!klp_root_kobj;
}
+/* Check if @func is present in @obj */
static struct klp_func *klp_find_func(struct klp_object *obj,
- struct klp_func *old_func)
+ struct klp_func *func)
{
- struct klp_func *func;
+ struct klp_func *obj_func;
- klp_for_each_func(obj, func) {
+ klp_for_each_func(obj, obj_func) {
/*
* Besides identical old_sympos, also consider old_sympos
* of 0 and 1 are identical.
*/
- if ((strcmp(old_func->old_name, func->old_name) == 0) &&
- ((old_func->old_sympos == func->old_sympos) ||
- (old_func->old_sympos == 0 && func->old_sympos == 1) ||
- (old_func->old_sympos == 1 && func->old_sympos == 0))) {
+ if ((obj_func->old_name && func->old_name &&
+ strcmp(obj_func->old_name, func->old_name) == 0) &&
+ ((obj_func->old_sympos == func->old_sympos) ||
+ (obj_func->old_sympos == 0 && func->old_sympos == 1) ||
+ (obj_func->old_sympos == 1 && func->old_sympos == 0))) {
return func;
}
}
--
Regards
Yafang
