Re: [RFC PATCH v2 6/7] livepatch/klp-build: add pre-built object support for advanced OOT workflows

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The newly introduced `--symvers` option breaks standard and `--oot-dir` builds when provided with a relative path, and it fails to override the `$PWD/Module.symvers` validation check for `--oot-dir` builds.
- [Medium] The `--show-first-changed` (`-f`) option is silently ignored in the pre-built object workflow.

Pre-existing issues:
- [High] Arbitrary command execution via maliciously crafted filenames containing Make functions in pre-built object directories.
--

commit f9d7e858a4afcd50869696d974b4dd56100b6cf0
Author: Joe Lawrence <joe.lawrence@redhat.com>
Subject: livepatch/klp-build: add pre-built object support for advanced OOT workflows

This commit introduces `--orig-dir` and `--patched-dir` options to klp-build,
allowing users to supply pre-built original and patched objects directly
rather than relying on internal build stages. It performs binary comparison,
symbol extraction, and livepatch module assembly using these pre-built objects.

> diff --git a/scripts/livepatch/klp-build b/scripts/livepatch/klp-build
> index b60f5a5da31e8..e118f133e923c 100755
> --- a/scripts/livepatch/klp-build
> +++ b/scripts/livepatch/klp-build

[ ... ]

> @@ -456,7 +486,19 @@ validate_patches() {
>  do_init() {
>  	[[ ! "$PWD" -ef "$SCRIPT_DIR/../.." ]] && die "please run from the kernel root directory"
>  
> -	if [[ -n "$OOT_DIR" ]]; then
> +	if [[ -n "$USER_ORIG_DIR" ]]; then
> +		[[ -d "$USER_ORIG_DIR" ]] || die "directory not found: $USER_ORIG_DIR"
> +		[[ -d "$USER_PATCHED_DIR" ]] || die "directory not found: $USER_PATCHED_DIR"
> +		USER_ORIG_DIR="$(realpath "$USER_ORIG_DIR")"
> +		USER_PATCHED_DIR="$(realpath "$USER_PATCHED_DIR")"
> +		if [[ -n "$SYMVERS_PATH" ]]; then
> +			SYMVERS_PATH="$(realpath "$SYMVERS_PATH")"
> +			[[ -f "$SYMVERS_PATH" ]] || die "Module.symvers not found: $SYMVERS_PATH"
> +		else
> +			[[ -f "$PWD/Module.symvers" ]] ||
> +				die "no Module.symvers in $PWD; use --symvers to specify"
> +		fi
> +	elif [[ -n "$OOT_DIR" ]]; then

[Severity: Medium]
Will this leave `SYMVERS_PATH` as an unexpanded relative path when used with
`--oot-dir` or standard builds, since the `realpath` conversion is nested
inside the `--orig-dir` conditional branch?

Also, in the `elif [[ -n "$OOT_DIR" ]];` branch that follows, there is a check
for `[[ -f "$PWD/Module.symvers" ]]`.

Does this fail to respect the provided `SYMVERS_PATH` during OOT
initialization and enforce validation on the hardcoded `$PWD/Module.symvers`,
preventing alternative paths?

[ ... ]

> @@ -757,7 +804,11 @@ diff_objects() {
>  		cmd+=("klp")
>  		cmd+=("diff")
>  		(( ${#opts[@]} > 0 )) && cmd+=("${opts[@]}")
> -		[[ -n "$OOT_DIR" ]] && cmd+=("--symvers" "$PWD/Module.symvers")
> +		if [[ -n "$SYMVERS_PATH" ]]; then
> +			cmd+=("--symvers" "$SYMVERS_PATH")

[Severity: Medium]
If `SYMVERS_PATH` remains a relative path from the issue above, will this
command fail when it is executed from the `$ORIG_CSUM_DIR` subshell?

[ ... ]

> @@ -931,11 +982,54 @@ build_patch_module() {
>  }
>  
>  
> +setup_oot() {
> +	local files=()
> +	local rel
> +
> +	mkdir -p "$ORIG_DIR" "$PATCHED_DIR"
> +
> +	find "$USER_ORIG_DIR" -type f -name "*.o" -printf '%P\n' | mapfile -t files

[Severity: High]
This is a pre-existing issue, but this pipeline ingests user-supplied
filenames from the pre-built object directories without sanitization.

Later in build_patch_module(), these filenames are written directly into the
generated Kbuild file target list:

    echo -n " $rel_file" >> "$makefile"

Can this allow arbitrary command execution via maliciously crafted filenames
containing Make functions (e.g., `$(shell command).o`) when processing untrusted
pre-built objects?

[ ... ]

> +	status "Building patch module: $OUTFILE"
> +	build_patch_module
> +
> +	status "SUCCESS"
> +	exit 0
> +fi
> +

[Severity: Medium]
Because this block executes the pre-built workflow and exits immediately,
doesn't it fully bypass the `diff_checksums` hook?

Will this silently ignore the `--show-first-changed` (`-f`) option in the
pre-built object workflow?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826195000.455905-1-joe.lawrence@redhat.com?part=6