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

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?

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