Re: [PATCH v3 4/7] livepatch: Deprecate stack_order

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Returning `-1` as the dummy value for the deprecated `stack_order` sysfs attribute risks breaking backward compatibility.
- [Low] Test coverage for the deprecated `stack_order` sysfs attribute was completely removed instead of being updated.
- [Low] The deprecated `stack_order` ABI documentation is placed in the wrong directory and contains an outdated description.

Pre-existing issues:
- [High] A NULL pointer dereference occurs in `klp_find_func()` when verifying patch compatibility with a malformed livepatch that omits `.old_name`.
--

commit ea264255177cd1b15e55c6d6aa2b21dbb4abd6c2
Author: Yafang Shao <laoar.shao@gmail.com>

livepatch: Deprecate stack_order

stack_order is no longer needed for atomic-replace livepatches, as a
single function can only be modified by a unique replace_set.
To maintain backward compatibility, print a dummy value, as suggested by
sashiko-bot.

> diff --git a/Documentation/ABI/removed/sysfs-kernel-livepatch b/Documentation/ABI/removed/sysfs-kernel-livepatch
> new file mode 100644
> index 0000000000000..72c1ce0ea8647
> --- /dev/null
> +++ b/Documentation/ABI/removed/sysfs-kernel-livepatch
> @@ -0,0 +1,9 @@
> +What:		/sys/kernel/livepatch/<patch>/stack_order
> +Date:		Jan 2025
> +KernelVersion:	6.14.0
> +Description:
> +		This attribute specifies the sequence in which live patch modules
> +		are applied to the system. If multiple live patches modify the same
> +		function, the implementation with the biggest 'stack_order' number
> +		is used, unless a transition is currently in progress.

[Severity: Low]
Is Documentation/ABI/removed/ the right place for this file since the
attribute is still present in the kernel?

According to Documentation/ABI/README, the removed/ directory is strictly
for interfaces that have been completely deleted. Since the attribute still
exists and returns a dummy value, should this be placed in obsolete/ instead?

Also, should the description be updated to explain that the attribute is now
obsolete and just returns a dummy value, rather than claiming it specifies
the sequence in which live patch modules are applied?

> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 361999e5ce3da..c73eccd2cd139 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
[ ... ]
> @@ -436,25 +436,11 @@ static ssize_t replace_set_show(struct kobject *kobj,
>  	return sysfs_emit(buf, "%u\n", patch->replace_set);
>  }
>  
> +/* This interface is deprecated and will be removed in the next release */
>  static ssize_t stack_order_show(struct kobject *kobj,
>  				struct kobj_attribute *attr, char *buf)
>  {
> -	struct klp_patch *patch, *this_patch;
> -	int stack_order = 0;
> -
> -	this_patch = container_of(kobj, struct klp_patch, kobj);
> -
> -	mutex_lock(&klp_mutex);
> -
> -	klp_for_each_patch(patch) {
> -		stack_order++;
> -		if (patch == this_patch)
> -			break;
> -	}
> -
> -	mutex_unlock(&klp_mutex);
> -
> -	return sysfs_emit(buf, "%d\n", stack_order);
> +	return sysfs_emit(buf, "-1\n");

[Severity: Medium]
Will returning -1 break userspace tools that expect a non-negative integer
or parse this as unsigned?

The commit message mentions printing a dummy value to maintain backward
compatibility. Since the previous implementation returned a strictly positive
sequence (1, 2, ...), could we use a previously valid integer like 1 to
ensure older scripts don't fail when encountering a negative sign?

[Severity: High]
This isn't a bug introduced by this patch, but an earlier commit in this
series ("livepatch: Support scoped atomic replace using replace_set")
appears to introduce a NULL pointer dereference when a malformed patch
omits .old_name.

When klp_enable_patch() is called, it invokes klp_is_patch_compatible()
before klp_init_patch() validates that func->old_name is non-NULL.

The klp_is_patch_compatible() function passes unvalidated functions to
klp_find_func() as the old_func argument:

kernel/livepatch/state.c:klp_is_patch_compatible() {
    ...
    if (klp_find_func(old_obj, func))
        return false;
    ...
}

Inside klp_find_func(), old_func->old_name is unconditionally dereferenced
in the first argument to strcmp():

kernel/livepatch/core.h:klp_find_func() {
    ...
    if ((func->old_name && strcmp(old_func->old_name, func->old_name) == 0) &&
    ...
}

Could this trigger a kernel panic during patch load?

> diff --git a/tools/testing/selftests/livepatch/test-sysfs.sh b/tools/testing/selftests/livepatch/test-sysfs.sh
> index 58fe1d96997cd..0c31759f34f63 100755
> --- a/tools/testing/selftests/livepatch/test-sysfs.sh
> +++ b/tools/testing/selftests/livepatch/test-sysfs.sh
> @@ -21,8 +21,6 @@ check_sysfs_rights "$MOD_LIVEPATCH" "enabled" "-rw-r--r--"
>  check_sysfs_value  "$MOD_LIVEPATCH" "enabled" "1"
>  check_sysfs_rights "$MOD_LIVEPATCH" "force" "--w-------"
>  check_sysfs_rights "$MOD_LIVEPATCH" "replace" "-r--r--r--"
> -check_sysfs_rights "$MOD_LIVEPATCH" "stack_order" "-r--r--r--"
> -check_sysfs_value  "$MOD_LIVEPATCH" "stack_order" "1"

[Severity: Low]
Does removing these checks completely leave the deprecated attribute untested?

Since stack_order is intentionally kept to maintain backward compatibility,
should the test suite still verify its presence, correct permissions, and
the new dummy output? This would ensure future changes don't accidentally
remove the sysfs node before the deprecation period ends.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607131659.29281-1-laoar.shao@gmail.com?part=4