Re: [PATCH v3 3/7] livepatch: Support scoped atomic replace using replace_set

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