Re: [PATCH] klp-build: Reuse the target kernel randstruct seed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] klp-build: set_kernelversion() clobbers randstruct.seed before pinning
- [Medium] klp-build: set_randstruct_seed() mtime update forces full rebuild on -S 2
--

commit 606c1a8e7c05ba3363ccb4c4faa0e0f6111aaeda
Author: Song Liu <song@kernel.org>

klp-build: Reuse the target kernel randstruct seed

This commit reuses the target kernel's randstruct seed in klp-build by
reading scripts/basic/randstruct.seed (or a file passed via
--randstruct-seed) before set_kernelversion() and pinning
scripts/gen-randstruct-seed.sh afterwards so that livepatch modules are
built with matching randomized struct layouts.

> diff --git a/scripts/livepatch/klp-build b/scripts/livepatch/klp-build
> index b52a8489d9f67..fd336f685a841 100755
> --- a/scripts/livepatch/klp-build
> +++ b/scripts/livepatch/klp-build

[ ... ]

> @@ -328,6 +332,51 @@ set_kernelversion() {
>  	sed -i "2i echo $kernelrelease; exit 0" scripts/setlocalversion
>  }
>  
> +# CONFIG_RANDSTRUCT randomizes struct layouts based on a seed which kbuild
> +# generates from /dev/urandom.  A livepatch has to observe the same layouts as
> +# the kernel it gets loaded into, so reuse that kernel's seed.  Otherwise the
> +# module loader rejects the module ("...this_module section size must match
> +# the kernel's built struct module size at run time"), and any randomized
> +# struct the patch touches would be accessed at the wrong offsets.
> +#
> +# This must run before set_kernelversion(), whose 'make syncconfig' also
> +# builds scripts/basic.  After a 'make clean' that would replace the seed
> +# before the default seed file gets read.
> +read_randstruct_seed() {
> +	local seed_file
> +
> +	[[ -v CONFIG_RANDSTRUCT ]] || return 0
> +
> +	if [[ -v RANDSTRUCT_SEED_FILE ]]; then
> +		seed_file="$RANDSTRUCT_SEED_FILE"
> +	else
> +		seed_file="$PWD/scripts/basic/randstruct.seed"
> +		[[ -r "$seed_file" ]] ||				\
> +			die "CONFIG_RANDSTRUCT is enabled but there's no $seed_file; pass --randstruct-seed with the seed used to build the kernel"
> +	fi
> +
> +	RANDSTRUCT_SEED="$(< "$seed_file")"
> +	[[ "$RANDSTRUCT_SEED" =~ ^[0-9a-f]{64}$ ]] ||			\
> +		die "malformed randstruct seed in $seed_file"
> +}
> +
> +# Pin the generator rather than the seed file: clean_kernel() removes
> +# scripts/basic/.randstruct.seed.cmd, so if_changed sees no saved command and
> +# regenerates the seed no matter what the file already holds.
> +#
> +# This must run after set_kernelversion(), otherwise setlocalversion sees the
> +# modified generator and appends "-dirty" to the kernel release.
> +set_randstruct_seed() {
> +	local gen="$PWD/scripts/gen-randstruct-seed.sh"
> +
> +	[[ -v RANDSTRUCT_SEED ]] || return 0
> +
> +	stash_file "$gen"
> +	sed -i "s|^SEED=.*|SEED=$RANDSTRUCT_SEED|" "$gen"
> +	command grep -q "^SEED=$RANDSTRUCT_SEED$" "$gen" ||		\
> +		die "failed to pin randstruct seed"
> +}

[Severity: Medium]
When running klp-build with --short-circuit=2 (-S 2 / -S patched) after an
initial -T run has built 1-orig and restored scripts/gen-randstruct-seed.sh
on exit via cleanup() -> restore_files(), set_randstruct_seed() runs sed -i
on scripts/gen-randstruct-seed.sh and updates its mtime to be newer than
scripts/basic/randstruct.seed from the 1-orig build.

In scripts/basic/Makefile:

    cmd_create_randstruct_seed = \
        $(CONFIG_SHELL) $(gen-randstruct-seed) \
            $@ $(objtree)/include/generated/randstruct_hash.h
    $(obj)/randstruct.seed: $(gen-randstruct-seed) FORCE
        $(call if_changed,create_randstruct_seed)

Because $(gen-randstruct-seed) has a newer mtime than $(obj)/randstruct.seed,
if_changed re-executes scripts/gen-randstruct-seed.sh and rewrites
include/generated/randstruct_hash.h (as well as triggering a rebuild of
randomize_layout_seed.h, randomize_layout_plugin.so, and
include/generated/gcc-plugins.h when CONFIG_GCC_PLUGIN_RANDSTRUCT is
enabled).

Since include/linux/compiler-version.h is force-included in every translation
unit:

    #ifdef RANDSTRUCT
    #include <generated/randstruct_hash.h>
    #endif

wouldn't touching include/generated/randstruct_hash.h cause build_kernel
"patched" under -S 2 to recompile the entire kernel tree instead of only the
patched files?

> +
>  get_patch_input_files() {
>  	local patch="$1"
>  
> @@ -464,7 +513,9 @@ do_init() {
>  
>  	validate_config
>  	set_module_name
> +	read_randstruct_seed
>  	set_kernelversion
> +	set_randstruct_seed
>  }

[Severity: Medium]
In do_init(), read_randstruct_seed() reads the seed before
set_kernelversion(), while set_randstruct_seed() pins
scripts/gen-randstruct-seed.sh only after set_kernelversion() finishes:

    set_kernelversion() {
        ...
        if [[ -n "$(make -s listnewconfig 2>/dev/null)" ]]; then
            die ".config mismatch, check your .config or run 'make olddefconfig'"
        fi
        make syncconfig &>/dev/null || die "make syncconfig failed"

Both "make -s listnewconfig" and "make syncconfig" match the top-level
Makefile rule:

    %config: outputmakefile scripts_basic FORCE
        $(Q)$(MAKE) $(build)=scripts/kconfig $@

When scripts/basic/.randstruct.seed.cmd is missing (for instance after "make
clean") or when scripts/gen-randstruct-seed.sh has a newer mtime than
scripts/basic/randstruct.seed (such as after a previous run where
stash_file() copied scripts/gen-randstruct-seed.sh with "cp -f" without
preserving timestamps and restore_files() moved it back on exit), building
scripts_basic in set_kernelversion() runs the unpinned
scripts/gen-randstruct-seed.sh. That overwrites scripts/basic/randstruct.seed
and include/generated/randstruct_hash.h with a new random seed from
/dev/urandom.

Because set_randstruct_seed() only edits scripts/gen-randstruct-seed.sh
without stashing or restoring scripts/basic/randstruct.seed and
include/generated/randstruct_hash.h, can this overwrite cause two issues?

First, if klp-build exits before build_kernel() runs (for example, when
listnewconfig or syncconfig fails in set_kernelversion(), or when
validate_patches() fails before build_kernel()), cleanup() restores
scripts/gen-randstruct-seed.sh while leaving scripts/basic/randstruct.seed on
disk overwritten with the random seed. Subsequent runs relying on the default
scripts/basic/randstruct.seed will then silently read that corrupted seed.

Second, when invoked with --short-circuit (-S 3, -S 4, or -S 5),
clean_kernel() and build_kernel() do not run, and build_patch_module() runs
an external module build ("make --directory=. M=$KMOD_DIR") which does not
build scripts_basic. Because the pinned scripts/gen-randstruct-seed.sh never
runs (and even under -S 2, if_changed can skip running it if
set_kernelversion() and set_randstruct_seed() execute within the same
filesystem timestamp tick), wouldn't the livepatch module be compiled using
the wrong seed left in scripts/basic/randstruct.seed and
include/generated/randstruct_hash.h?

>  
>  # Refresh the patch hunk headers, specifically the line numbers and counts.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020212341.706367-1-song@kernel.org?part=1