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

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?