Re: [PATCH v3 1/7] livepatch: Fix NULL pointer dereference in klp_find_func()

On Mon, Jun 22, 2026 at 10:21 PM Miroslav Benes <[email protected]> wrote:
>
> On 2026-06-09 15:27:18+02:00, Petr Mladek wrote:
> > On Sun 2026-06-07 21:16:53, Yafang Shao wrote:
> >
> > > If a newly loaded livepatch provides a function entry with a NULL old_name,
> > > func->old_name will be NULL when evaluated in strcmp():
> > >
> > > klp_init_patch()
> > >     klp_add_nops()
> > >         klp_find_func()
> > >             strcmp(old_func->old_name, func->old_name)
> > >
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -92,7 +92,7 @@ static struct klp_func *klp_find_func(struct klp_object *obj,
> > >              * Besides identical old_sympos, also consider old_sympos
> > >              * of 0 and 1 are identical.
> > >              */
> > > -           if ((strcmp(old_func->old_name, func->old_name) == 0) &&
> > > +           if ((func->old_name && strcmp(old_func->old_name, func->old_name) == 0) &&
> >
> > I do not have a good feeling about this solution because it quietly
> > ignores a problem. As a result, klp_add_object_nops() would
> > call klp_alloc_func_nop() even though it does not make much sense.
> >
> > A livepatch where any func->oldname is not defined should get
> > rejected. It will actually happen but _later_ in:
> >
> >   + klp_init_patch()
> >     + klp_init_object()
> >       + klp_init_func()
> >
> > I see three better possibilities.
> >
> > 1. We could move/add the sanity checks into klp_init_patch_early()
> >    and return broken livepatches earlier.
> >
> > 2. We could move/add the sanity check into a new klp_check_patch()
> >    which will be called even before klp_init_patch_early().
> >
> > 3. We could allow klp_find_func() to return ERR_PTR(-EINVAL).
> >    klp_add_object_nops() should then return the error as well.
>
> If you change it to do so.
>
> > My preference:
> >
> > I would do the 3rd variant because it is much easier than
> > adding/moving all consistency checks between klp_init_*() and
> > klp_init_*_early() or klp_check_*() functions.
>
> Interesting issue. If I remember correctly, klp_init_*() path is meant
> for the initial checking originally (among others). That is why
> klp_init_func() has it right at the beginning. It got a lot complicated
> later. We introduced atomic replace feature which added klp_add_nops()
> and is called earlier. Then there was the kobject issue which introduced
> early init path. Both forgot about the check and that is what we have
> now. I would prefer it to fix cleanly. klp_init_patch_early() is not
> enough because klp_is_patch_compatible() is called even earlier. So your
> option 2 sounds best to me. On the other hand, it might be difficult to
> disentangle it all.
>
> Anyway, could you send the fix separately since it is an existing issue,
> please?

Does the following change look good to you ?

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 28d15ba58a26..317a3c866c76 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -799,9 +799,6 @@ void klp_free_replaced_patches_async(struct
klp_patch *new_patch)

 static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 {
-       if (!func->old_name)
-               return -EINVAL;
-
        /*
         * NOPs get the address later. The patched module must be loaded,
         * see klp_init_object_loaded().
@@ -1107,8 +1104,9 @@ static int __klp_enable_patch(struct klp_patch *patch)
  */
 int klp_enable_patch(struct klp_patch *patch)
 {
-       int ret;
        struct klp_object *obj;
+       struct klp_func *func;
+       int ret;

        if (!patch || !patch->mod || !patch->objs)
                return -EINVAL;
@@ -1116,9 +1114,12 @@ int klp_enable_patch(struct klp_patch *patch)
        klp_for_each_object_static(patch, obj) {
                if (!obj->funcs)
                        return -EINVAL;
+               klp_for_each_func_static(obj, func) {
+                       if (!func->old_name)
+                               return -EINVAL;
+               }
        }

-
        if (!is_livepatch_module(patch->mod)) {
                pr_err("module %s is not marked as a livepatch module\n",
                       patch->mod->name);

-- 
Regards
Yafang