Kexec bpf v5

[pfliu]

No description provided.

[pfliu]

/gemini review

[gemini-code-assist]

Summary of Changes

Hello @pfliu, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the Linux kernel's kexec functionality by introducing a BPF-driven mechanism for loading and parsing UEFI Portable Executable (PE) images. This new approach allows for flexible and dynamic handling of complex boot image formats, such as zboot, where various components like the kernel, initrd, and command line might be compressed or nested within the PE file. By integrating BPF, the kernel can delegate the intricate parsing logic to user-defined programs, making the kexec process more adaptable to diverse boot environments and future format evolutions without requiring kernel recompilation for each new format.

Highlights

• BPF-assisted PE Image Loading: Introduces a new kexec file loader (kexec_pe_image_ops) that leverages BPF programs to parse UEFI Portable Executable (PE) files, enabling dynamic and extensible handling of boot images.
• Dynamic Parsing with Embedded BPF: BPF programs, embedded within a dedicated .bpf section of a PE image, can now dynamically extract and decompress kernel, initrd, and command line components during the kexec loading process.
• New BPF Kernel Functions and Infrastructure: Adds essential BPF kernel functions (bpf_decompress, bpf_copy_to_kernel) and supporting data structures (mem_range_result, carrier_listener) to allow BPF programs to manage kernel memory, perform decompression, and pass extracted resources back to the kexec loader.
• Zboot Image Support and Tooling: Provides a userspace tool (zboot_image_builder) to inject BPF parsing logic into zboot EFI images, ensuring proper handling of compressed kernel payloads while maintaining PE header integrity. A corresponding BPF program (zboot_parser_bpf.c) is also introduced for this purpose.
• ARM64 Architecture Integration: Enables the KEXEC_PE_IMAGE configuration option for ARM64 architectures, allowing this new PE image loading mechanism to be utilized on ARM64 systems.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a significant new feature for kexec, allowing PE files to be loaded and parsed with the help of BPF programs. This is a complex change that adds a new BPF "carrier" framework for passing data from BPF to the kernel, a PE image loader, and BPF programs for parsing specific PE formats like zboot. While the overall architecture is sound, my review has identified several critical issues, including a mutex deadlock and use-after-free vulnerability in the BPF decompression helper, a memory leak in the PE image loader's error path, and other high-severity bugs related to incorrect resource management and logic. These critical issues must be addressed before this change can be merged.

[gemini-code-assist]

There are critical issues in this error handling path:

1. If decompress_method() returns NULL, the code jumps to the error: label while output_buf_mutex is still held. This will cause the function to return without releasing the mutex, leading to a deadlock on subsequent calls.
2. In the same error path, range is freed via kfree(range), but the function proceeds to return this now-dangling range pointer, which is a use-after-free vulnerability.

The error handling logic must be refactored to ensure the mutex is always unlocked and resources are cleaned up correctly on all paths. Failures after acquiring the mutex should jump to a cleanup path that unlocks the mutex before returning.

[gemini-code-assist]

There is a critical memory leak in the error handling path. The cleanup logic at the err: label is also used for the success path. It unconditionally sets r->buf = NULL before calling mem_range_result_put(r).

On a failure path, this prevents __mem_range_result_free from freeing the buffer pointed to by r->buf, causing a memory leak. The cleanup logic should differentiate between success and failure. On failure, mem_range_result_put(r) should be called without modifying r to ensure the buffer is freed.

err:
	/* On success, ldata is not an error pointer. */
	bool success = !IS_ERR(ldata);

	for (int i = 0; i < 3; i++) {
		r = parsed_resource[i].r;
		if (!r)
			continue;
		parsed_resource[i].r = NULL;
		/*
		 * On success, buffer ownership is transferred to kimage.
		 * Prevent freeing it here. On failure, the buffer should be
		 * freed by mem_range_result_put().
		 */
		if (success) {
			r->buf = NULL;
			r->buf_sz = 0;
		}
		mem_range_result_put(r);
	}

[gemini-code-assist]

There is a bug in this error handling block. You are checking range->alloc_type to determine which deallocation function to use for kbuf. However, range->alloc_type is not initialized until line 157. At this point, it holds an uninitialized value, which will lead to undefined behavior. You should use the local alloc_type variable, which holds the correct value.

if (alloc_type == TYPE_KALLOC)
	kfree(kbuf);
else
	vfree(kbuf);

[gemini-code-assist]

This check is too strict. A valid kexec image might not have an initrd, and the command line can be empty. The kexec_file_load syscall supports these cases. This check will cause pe_image_load to fail for valid scenarios. It should be relaxed to only require that the kernel is present.

if (kernel == NULL) {

[gemini-code-assist]

There is a bug in the error message construction. c = '\0'; assigns the null character's value (0) to the pointer c, which is incorrect and will likely cause a crash. To null-terminate the string, you should write to the memory location pointed to by c.

		*c = '\0';

[gemini-code-assist]

The help text has some minor grammatical errors and could be clearer. Consider rephrasing for better readability.

  This option enables the kexec_file_load() syscall to cooperate with a BPF
  program to parse a PE format file.

[gemini-code-assist]

The section name fentry.s/bpf_handle_pefile appears to be incorrect. The standard format for fentry/fexit programs is fentry/FUNCTION_NAME. The .s is likely a typo and should be removed.

SEC("fentry/bpf_handle_pefile")

[gemini-code-assist]

Similar to the other fentry program, the section name fentry.s/bpf_post_handle_pefile appears to be incorrect. The .s is likely a typo and should be removed.

SEC("fentry/bpf_post_handle_pefile")

[gemini-code-assist]

This function has resource leaks. The file pointer bin_fp is opened with fopen but never closed. Additionally, bpf_fd is closed but was never opened, which will attempt to close an arbitrary file descriptor.

	close(base_fd);
	close(out_fd);
	fclose(bin_fp);

[gemini-code-assist]

The expression payload_size - 4 uses a magic number 4. Please add a comment to explain why 4 bytes are being subtracted from the payload size before decompression. This will improve code readability and maintainability.

[gemini-code-assist]

Code Review

This pull request introduces a significant new feature enabling kexec to load and parse PE files using BPF programs. This is an innovative approach that involves adding new BPF helpers, a generic "carrier" mechanism for BPF-to-kernel data transfer, and new tooling for embedding BPF programs into PE files. The changes are extensive and touch multiple kernel subsystems. My review has identified a few critical and high-severity bugs in the implementation of the new BPF helpers and the PE image loading logic, along with some medium-severity issues in the supporting tools and the BPF program itself. These should be addressed to ensure the stability and correctness of this new functionality.

[gemini-code-assist]

This error handling block has a couple of issues:

1. copy_from_kernel_nofault() returns an unsigned long indicating the number of bytes not copied. A non-zero value means failure. The current check ret < 0 is incorrect because the return value is never negative. Also, storing the result in an int can cause truncation.
2. The free logic uses range->alloc_type, but this field is not initialized until line 157. The local alloc_type variable should be used instead.

Here's a suggested fix that addresses both points. I've also changed the error code to -EFAULT which is more appropriate for a copy failure.

if (unlikely(copy_from_kernel_nofault(kbuf, buf, size))) {
		if (alloc_type == TYPE_KALLOC)
			kfree(kbuf);
		else
			vfree(kbuf);
		kfree(range);
		ret = -EFAULT;
		goto err;
	}

[gemini-code-assist]

This error handling logic has several flaws:

1. It treats a missing initrd or cmdline as a fatal error by checking them with ||. These are often optional, and only a missing kernel should be a fatal error.
2. The error message construction is buggy. It uses strcpy in a loop, which overwrites the buffer on each iteration instead of appending. For example, if both kernel and initrd are NULL, the buffer will only contain "initrd ".
3. c = '\0'; is incorrect for null-terminating a string. It should be *c = '\0';.

I suggest refactoring this to only fail on a missing kernel and issue warnings for other missing components.

if (!kernel) {
		pr_err("Can not extract kernel from PE container\n");
		ldata = ERR_PTR(-EINVAL);
		goto err;
	}

	if (!initrd)
		pr_warn("Could not find initrd in PE container\n");

	if (!cmdline)
		pr_warn("Could not find cmdline in PE container\n");

[gemini-code-assist]

The use of a global mutex (output_buf_mutex) and a single global allocator instance (dcmpr_allocator) makes the bpf_decompress kfunc non-reentrant. While this might be acceptable for the current kexec use case where concurrent calls are unlikely, it limits the general applicability of this helper for other potential use cases in the future. As noted in the comment on line 3724, making this mechanism re-entrant would be a valuable improvement for wider adoption.

[gemini-code-assist]

The file descriptor bpf_fd is closed here, but it was never initialized or opened. Meanwhile, the file pointer bin_fp is opened with fopen but never closed, leading to a resource leak. You should close bin_fp using fclose.

fclose(bin_fp);

[gemini-code-assist]

The subtraction of 4 from payload_size appears to be a magic number. To improve code clarity and maintainability, please add a comment explaining why these 4 bytes are being excluded from the decompression payload. For instance, it might be a checksum or size field that isn't part of the compressed stream.