Re: [PATCHv7 06/13] kexec_file: Chain the stages into a pipeline

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

New issues:
- [High] Aliasing ctx->kernel and ctx->next_parsing_buf in KEXEC_BPF_CMD_DECOMPRESS leads to a Use-After-Free.
- [Medium] Memory leak of newly allocated buffer 'pn' in KEXEC_BPF_CMD_COPY when the pipeline array is full.

Pre-existing issues:
- [Critical] The BPF container decomposition stage completely bypasses KEXEC signature verification.
- [Critical] The bpf_buffer_parser kfunc lacks the required __sz suffix on its size argument, defeating the BPF verifier.
- [High] Memory leak of parsing buffers due to a double-negation logic error (!!context->parsing_buf[i]).
- [High] Severe memory leak of kexec_context dynamically allocated components on error and success paths.
- [High] vfree() is improperly used on memory allocated by kernel_read_file_from_fd().
--

--- Patch [6]: [PATCHv7 06/13] kexec_file: Chain the stages into a pipeline ---
commit 8070ede2f761980c4a2de11a2d72fb8a5309bc74
Author: Pingfan Liu <[email protected]>

kexec_file: Chain the stages into a pipeline

This commit enables kexec stages to be chained into a pipeline. It promotes
intermediate outputs from next_parsing_buf to parsing_buf for the subsequent
BPF parsing stages. It also allows multiple layers with distinct formats to be
parsed sequentially.

> diff --git a/kernel/kexec_bpf_loader.c b/kernel/kexec_bpf_loader.c
> index af16f7b685d9a..7f7884411e2c7 100644
> --- a/kernel/kexec_bpf_loader.c
> +++ b/kernel/kexec_bpf_loader.c
[ ... ]
> @@ -301,6 +305,23 @@ static int kexec_buff_parser(struct bpf_parser_context *parser)
>  				vfree(ctx->kernel);
>  				ctx->kernel = decompressed_buf;
>  				ctx->kernel_sz = decompressed_sz;
> +				if (fill_pipeline) {
> +					int i;
> +
> +					for (i = 0; i < MAX_PARSING_BUF_NUM; i++) {
> +						if (ctx->next_parsing_buf[i])
> +							continue;
> +						ctx->next_parsing_buf[i] = decompressed_buf;
> +						ctx->next_parsing_buf_sz[i] = decompressed_sz;
> +						break;
> +					}

[Severity: High]
Does this aliasing of decompressed_buf create a use-after-free risk?

If a nested BPF program parses a sub-kernel and issues another
KEXEC_BPF_SUBCMD_KERNEL command, kexec_buff_parser() will unconditionally call
vfree(ctx->kernel):

kexec_buff_parser()
   case KEXEC_BPF_SUBCMD_KERNEL:
      vfree(ctx->kernel);

Since ctx->kernel and ctx->next_parsing_buf[i] point to the exact same buffer,
wouldn't this free the memory that the nested parser is actively reading from?

> +					/* No enough parsing slot */
> +					if (i == MAX_PARSING_BUF_NUM) {
> +						ctx->kernel = NULL;
> +						vfree(decompressed_buf);
> +						return -ENOMEM;
> +					}
> +				}
>  				break;
[ ... ]
> @@ -313,6 +334,22 @@ static int kexec_buff_parser(struct bpf_parser_context *parser)
>  		if (!p)
>  			return -ENOMEM;
>  		memcpy(p, buf, cmd->payload_len);
> +		if (fill_pipeline) {
> +			pn = __vmalloc(cmd->payload_len, GFP_KERNEL | __GFP_ACCOUNT);
> +			if (!pn) {
> +				vfree(p);
> +				return -ENOMEM;
> +			}
> +			memcpy(pn, buf, cmd->payload_len);
> +			for (int i = 0; i < MAX_PARSING_BUF_NUM; i++) {
> +				if (!ctx->next_parsing_buf[i]) {
> +					ctx->next_parsing_buf[i] = pn;
> +					ctx->next_parsing_buf_sz[i] = cmd->payload_len;
> +					break;
> +				}
> +			}
> +		}

[Severity: Medium]
If the pipeline array is fully exhausted and ctx->next_parsing_buf has no
empty slots, the loop will complete without storing pn. Is there a missing
check after the loop to free pn when it cannot be stored?

[ ... ]
> @@ -637,6 +674,14 @@ static int process_bpf_parsers_container(const char *elf_buf, size_t elf_sz,
>  		/* Free the old parsing context, and reload the new */
>  		for (int i = 0; i < MAX_PARSING_BUF_NUM; i++) {
>  			if (!!context->parsing_buf[i])
>  				break;
>  			vfree(context->parsing_buf[i]);

[Severity: High]
This is a pre-existing issue, but does this double negation logic incorrectly
skip cleanup?

Because !!context->parsing_buf[i] evaluates to true for any non-NULL
pointer, the loop will immediately break instead of freeing valid buffers.

[Severity: High]
This is a pre-existing issue, but is it safe to use vfree() here?

Since these buffers are originally populated by kernel_read_file_from_fd()
which uses kvmalloc(), shouldn't kvfree() be used to avoid panics on
kmalloc'd memory?

>  			context->parsing_buf[i] = NULL;
>  			context->parsing_buf_sz[i] = 0;
>  		}
> +		for (int i = 0; i < MAX_PARSING_BUF_NUM; i++) {
> +			if (!context->next_parsing_buf[i])
> +				break;
> +			context->parsing_buf[i] = context->next_parsing_buf[i];


While reviewing this patch, I noticed a few pre-existing issues in other
related areas:

kernel/kexec_bpf_loader.c:decompose_kexec_image() {
...
		ret = process_bpf_parsers_container(parser_start, parser_sz, &ctx, 0);
		if (!ret) {
...
			vfree(ctx.cmdline);
		}
		return ret;
}

[Severity: High]
This is a pre-existing issue, but does this function leak memory on the error
path?

If process_bpf_parsers_container() returns an error, the function exits
directly and returns ret without freeing ctx.kernel, ctx.initrd, ctx.cmdline,
or the array of buffers in ctx.parsing_buf allocated by
kernel_read_file_from_fd().

Also, on the success path (!ret), the ctx.parsing_buf array appears to be
permanently leaked as well.


kernel/kexec_file.c:kimage_file_prepare_segments() {
...
	if (IS_ENABLED(CONFIG_KEXEC_BPF)) {
		/* Fill up image's kernel_buf, initrd_buf, cmdline_buf */
		ret = decompose_kexec_image(image, initrd_fd);
		switch (ret) {
		case 0:
			envelop = true;
...
#ifdef CONFIG_KEXEC_SIG
	if (!envelop) {
		ret = kimage_validate_signature(image);
		if (ret)
			goto out;
	}
#endif
...
}

[Severity: Critical]
This is a pre-existing issue, but doesn't skipping signature validation when
envelop is true create a security regression?

If decompose_kexec_image() successfully extracts the kernel/initrd from a
container via unverified BPF programs, could an attacker load an unsigned,
untrusted kernel simply by wrapping it in an ELF container, completely
defeating kernel signature enforcement and Lockdown?


kernel/bpf/bpf_buffer_parser.c:bpf_buffer_parser() {
...
__bpf_kfunc int bpf_buffer_parser(char *buf, int buf_sz,
		struct bpf_parser_context *context)
{
...
}

[Severity: Critical]
This is a pre-existing issue, but does the size parameter need the __sz suffix
to be properly verified?

Because the parameter is named buf_sz instead of buf__sz, the BPF verifier
treats it as a standard scalar and won't enforce bounds checking between the
pointer and the size. Could an unprivileged BPF program supply a small valid
pointer and an arbitrarily large buf_sz to cause an out-of-bounds read?

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