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

Pingfan Liu posted 13 patches 2 weeks, 1 day ago
[PATCHv7 06/13] kexec_file: Chain the stages into a pipeline
Posted by Pingfan Liu 2 weeks, 1 day ago
Images may consist of multiple layers, each with a distinct format. For
example, an AArch64 UKI image typically embeds a zboot image in the
.linux section. The parser therefore determines whether its output
should be forwarded to the next stage.

Intermediate results are stored in next_parsing_buf and then promoted to
parsing_buf for the subsequent stage.

Signed-off-by: Pingfan Liu <piliu@redhat.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Dave Young <dyoung@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Philipp Rudo <prudo@redhat.com>
To: kexec@lists.infradead.org
---
 kernel/kexec_bpf_loader.c | 47 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

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
@@ -41,6 +41,8 @@ struct kexec_context {
 	bool parsed;
 	char *parsing_buf[MAX_PARSING_BUF_NUM];
 	unsigned long parsing_buf_sz[MAX_PARSING_BUF_NUM];
+	char *next_parsing_buf[MAX_PARSING_BUF_NUM];
+	unsigned long next_parsing_buf_sz[MAX_PARSING_BUF_NUM];
 
 	char *kernel;
 	unsigned long kernel_sz;
@@ -278,8 +280,9 @@ static int kexec_buff_parser(struct bpf_parser_context *parser)
 	struct bpf_parser_buf *pbuf = parser->buf;
 	struct kexec_context *ctx = (struct kexec_context *)parser->data;
 	struct cmd_hdr *cmd = (struct cmd_hdr *)pbuf->buf;
-	char *decompressed_buf, *buf, *p;
+	char *decompressed_buf, *buf, *p, *pn;
 	unsigned long decompressed_sz;
+	bool fill_pipeline = false;
 	int ret = 0;
 
 	buf = pbuf->buf + sizeof(struct cmd_hdr);
@@ -288,6 +291,7 @@ static int kexec_buff_parser(struct bpf_parser_context *parser)
 				cmd->payload_len, pbuf->size);
 		return -EINVAL;
 	}
+	fill_pipeline = cmd->pipeline_flag & KEXEC_BPF_PIPELINE_FILL;
 	switch (cmd->cmd) {
 	case KEXEC_BPF_CMD_DONE:
 		ctx->parsed = true;
@@ -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;
+					}
+					/* No enough parsing slot */
+					if (i == MAX_PARSING_BUF_NUM) {
+						ctx->kernel = NULL;
+						vfree(decompressed_buf);
+						return -ENOMEM;
+					}
+				}
 				break;
 			default:
 				vfree(decompressed_buf);
@@ -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;
+				}
+			}
+		}
+
 		switch (cmd->subcmd) {
 		case KEXEC_BPF_SUBCMD_KERNEL:
 			vfree(ctx->kernel);
@@ -637,6 +674,14 @@ static int process_bpf_parsers_container(const char *elf_buf, size_t elf_sz,
 			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];
+			context->parsing_buf_sz[i] = context->next_parsing_buf_sz[i];
+			context->next_parsing_buf[i] = NULL;
+			context->next_parsing_buf_sz[i] = 0;
+		}
 
 		put_bpf_parser_context(bpf);
 		/* If the bpf-prog success, it flags by KEXEC_BPF_CMD_DONE */
-- 
2.49.0
Re: [PATCHv7 06/13] kexec_file: Chain the stages into a pipeline
Posted by bot+bpf-ci@kernel.org 2 weeks, 1 day ago
> diff --git a/kernel/kexec_bpf_loader.c b/kernel/kexec_bpf_loader.c
> index af16f7b685d9..7f7884411e2c 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;
> +					}

After this, both ctx->kernel and ctx->next_parsing_buf[i] point to
the same decompressed_buf allocation. When next_parsing_buf is later
promoted to parsing_buf in process_bpf_parsers_container(), there
are two independent references to the same memory.

If the next BPF stage issues another KEXEC_BPF_CMD_DECOMPRESS for
KEXEC_BPF_SUBCMD_KERNEL, vfree(ctx->kernel) frees the buffer while
parsing_buf[i] still references it.

The KEXEC_BPF_CMD_COPY path avoids this by allocating a separate
copy with __vmalloc(). Should the DECOMPRESS path do the same to
prevent aliased ownership?

> +					/* 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;
> +				}
> +			}
> +		}

If all MAX_PARSING_BUF_NUM slots are already occupied, the loop
completes without storing pn anywhere, and the allocation is leaked.

The DECOMPRESS path handles this case with an explicit check:

    if (i == MAX_PARSING_BUF_NUM) {
        ctx->kernel = NULL;
        vfree(decompressed_buf);
        return -ENOMEM;
    }

Is a similar overflow check missing here for the COPY path?

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/23393379437