Re: [PATCH v5 2/9] livepatch: Make klp_find_func() non static

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A malformed livepatch module with a missing `old_name` triggers a NULL pointer dereference in `klp_find_func()`.
--

--- Patch [2]: [PATCH v5 2/9] livepatch: Make klp_find_func() non static ---
commit c50e76ff358d6d9f27c1c156d6d6f492f2cdd3b1
Author: Yafang Shao <[email protected]>

livepatch: Make klp_find_func() non static

This commit makes klp_find_func() non-static to allow its use in other
source files by an upcoming patch. It also renames the parameter @old_func
to @func.

> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 28d15ba58a26d..5d05d6fcbf57c 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c

[ ... ]

> @@ -82,21 +82,21 @@ static bool klp_initialized(void)
>  	return !!klp_root_kobj;
>  }
>  
> -static struct klp_func *klp_find_func(struct klp_object *obj,
> -				      struct klp_func *old_func)
> +/* Check if @func is present in @obj. */
> +struct klp_func *klp_find_func(struct klp_object *obj, 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))) {
> -			return func;
> +		if ((strcmp(obj_func->old_name, func->old_name) == 0) &&

[Severity: High]
This is a pre-existing issue, but could this strcmp() trigger a null pointer
dereference if a malformed livepatch module has a missing old_name field?

When loading a livepatch module where patch->replace is true, if a
struct klp_func element has .new_func defined but .old_name set to null,
klp_init_patch_early() adds this unvalidated function to obj->func_list.

Subsequently, klp_add_object_nops() calls klp_find_func() on the unvalidated
new patch object before klp_init_object() performs the expected null check:

kernel/livepatch/core.c:klp_add_object_nops() {
    ...
		func = klp_find_func(obj, old_func);
    ...
}

This unconditionally passes the unvalidated, null obj_func->old_name to
strcmp().

Does this allow a malformed module to bypass validation and crash the
system instead of being gracefully rejected?

> +		    ((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 obj_func;
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2