[PATCH bpf-next v2 7/7] bpf: Fix double-free RO header in bpf_jit_free

[Pu Lehui]

From: Pu Lehui <pulehui@xxxxxxxxxx>

When bpf_jit_binary_pack_finalize fails in bpf_jit_free, it implicitly
frees ro_header. However, JITs are unaware of this. They extract and
free it again, leading to double-free issue.

bpf_jit_free
  bpf_jit_binary_pack_finalize(ro_header, rw_header)
    ptr = bpf_arch_text_copy(ro_header, rw_header, rw_header->size);
    kvfree(rw_header);
    if (IS_ERR(ptr)) { <-- copy failed
      bpf_prog_pack_free(ro_header, ro_header->size); <-- first free ro_header
      return PTR_ERR(ptr);
    }
  hdr = bpf_jit_binary_pack_hdr(prog); <-- fetch ro_header
  bpf_jit_binary_pack_free(hdr, NULL); <-- double free ro_header

Fix this by dropping the implicit free in bpf_jit_binary_pack_finalize.
Accordingly, adapt the JITs to explicitly handle freeing ro_header in
their error paths. This also fixes the missing memory uncharge upon
failure, as bpf_prog_pack_free does not uncharge.

Additionally, initialize ro_header->size in bpf_jit_binary_pack_alloc to
ensures ro_header->size is always valid during free, and remove
unnecessary bpf_arch_text_copy in JITs.

Fixes: 1d5f82d9dd47 ("bpf, x86: fix freeing of not-finalized bpf_prog_pack")
Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
Signed-off-by: Pu Lehui <pulehui@xxxxxxxxxx>
---
 arch/arm64/net/bpf_jit_comp.c   |  7 ++-----
 arch/loongarch/net/bpf_jit.c    |  6 ++----
 arch/powerpc/net/bpf_jit_comp.c |  5 +++--
 arch/riscv/net/bpf_jit_core.c   |  7 ++-----
 arch/x86/net/bpf_jit_comp.c     |  7 ++-----
 kernel/bpf/core.c               | 17 +++++++++--------
 6 files changed, 20 insertions(+), 29 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index f4e4d4578e38..f5851e08e76e 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -2258,7 +2258,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 			goto out_free_hdr;
 		}
 		if (WARN_ON(bpf_jit_binary_pack_finalize(ro_header, header))) {
-			/* ro_header and header has been freed */
+			bpf_jit_binary_pack_free(ro_header, NULL);
 			ro_header = NULL;
 			header = NULL;
 			goto out_free_hdr;
@@ -2307,11 +2307,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 		prog->jited = 0;
 		prog->jited_len = 0;
 	}
-	if (header) {
-		bpf_arch_text_copy(&ro_header->size, &header->size,
-				   sizeof(header->size));
+	if (header)
 		bpf_jit_binary_pack_free(ro_header, header);
-	}
 	goto out_off;
 }
 
diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
index 2738b4db1165..48ad418df133 100644
--- a/arch/loongarch/net/bpf_jit.c
+++ b/arch/loongarch/net/bpf_jit.c
@@ -2298,7 +2298,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 			goto out_free;
 		}
 		if (WARN_ON(bpf_jit_binary_pack_finalize(ro_header, header))) {
-			/* ro_header and header have been freed */
+			bpf_jit_binary_pack_free(ro_header, NULL);
 			ro_header = NULL;
 			header = NULL;
 			goto out_free;
@@ -2341,10 +2341,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 		prog->jited_len = 0;
 	}
 
-	if (header) {
-		bpf_arch_text_copy(&ro_header->size, &header->size, sizeof(header->size));
+	if (header)
 		bpf_jit_binary_pack_free(ro_header, header);
-	}
 	goto out_offset;
 }
 
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index 7b07b43575f1..ff9bcd55165b 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -314,7 +314,6 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 		bpf_jit_build_prologue(code_base, &cgctx);
 		if (bpf_jit_build_body(fp, code_base, fcode_base, &cgctx, addrs, pass,
 				       extra_pass)) {
-			bpf_arch_text_copy(&fhdr->size, &hdr->size, sizeof(hdr->size));
 			bpf_jit_binary_pack_free(fhdr, hdr);
 			goto out_err;
 		}
@@ -339,8 +338,10 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 #endif
 
 	if (!fp->is_func || extra_pass) {
-		if (bpf_jit_binary_pack_finalize(fhdr, hdr))
+		if (bpf_jit_binary_pack_finalize(fhdr, hdr)) {
+			bpf_jit_binary_pack_free(fhdr, NULL);
 			goto out_err;
+		}
 	}
 
 	fp->bpf_func = (void *)fimage;
diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c
index 059db1adeaf8..55530e80df2e 100644
--- a/arch/riscv/net/bpf_jit_core.c
+++ b/arch/riscv/net/bpf_jit_core.c
@@ -154,7 +154,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 
 	if (!prog->is_func || extra_pass) {
 		if (WARN_ON(bpf_jit_binary_pack_finalize(jit_data->ro_header, jit_data->header))) {
-			/* ro_header has been freed */
+			bpf_jit_binary_pack_free(jit_data->ro_header, NULL);
 			jit_data->ro_header = NULL;
 			jit_data->header = NULL;
 			goto out_free_hdr;
@@ -183,11 +183,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 		prog->jited = 0;
 		prog->jited_len = 0;
 	}
-	if (jit_data->header) {
-		bpf_arch_text_copy(&jit_data->ro_header->size, &jit_data->header->size,
-				   sizeof(jit_data->header->size));
+	if (jit_data->header)
 		bpf_jit_binary_pack_free(jit_data->ro_header, jit_data->header);
-	}
 	goto out_offset;
 }
 
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index b2feec81e231..281e89764d98 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -3930,11 +3930,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 		if (proglen <= 0) {
 out_image:
 			image = NULL;
-			if (header) {
-				bpf_arch_text_copy(&header->size, &rw_header->size,
-						   sizeof(rw_header->size));
+			if (header)
 				bpf_jit_binary_pack_free(header, rw_header);
-			}
 			if (extra_pass) {
 				prog->bpf_func = NULL;
 				prog->jited = 0;
@@ -3987,7 +3984,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 			 * Both cases are serious bugs and justify WARN_ON.
 			 */
 			if (WARN_ON(bpf_jit_binary_pack_finalize(header, rw_header))) {
-				/* header has been freed */
+				bpf_jit_binary_pack_free(header, NULL);
 				header = NULL;
 				goto out_image;
 			}
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index e2076667b245..00c32539f074 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1218,6 +1218,13 @@ bpf_jit_binary_pack_alloc(unsigned int proglen, u8 **image_ptr,
 		return NULL;
 	}
 
+	/* Initialize ro_header->size to ensure it is valid during free */
+	if (IS_ERR(bpf_arch_text_copy(&ro_header->size, &size, sizeof(size)))) {
+		bpf_prog_pack_free(ro_header, size);
+		bpf_jit_uncharge_modmem(size);
+		return NULL;
+	}
+
 	*rw_header = kvmalloc(size, GFP_KERNEL);
 	if (!*rw_header) {
 		bpf_prog_pack_free(ro_header, size);
@@ -1249,10 +1256,9 @@ int bpf_jit_binary_pack_finalize(struct bpf_binary_header *ro_header,
 
 	kvfree(rw_header);
 
-	if (IS_ERR(ptr)) {
-		bpf_prog_pack_free(ro_header, ro_header->size);
+	if (IS_ERR(ptr))
 		return PTR_ERR(ptr);
-	}
+
 	return 0;
 }
 
@@ -1260,11 +1266,6 @@ int bpf_jit_binary_pack_finalize(struct bpf_binary_header *ro_header,
  *   1) when the program is freed after;
  *   2) when the JIT engine fails (before bpf_jit_binary_pack_finalize).
  * For case 2), we need to free both the RO memory and the RW buffer.
- *
- * bpf_jit_binary_pack_free requires proper ro_header->size. However,
- * bpf_jit_binary_pack_alloc does not set it. Therefore, ro_header->size
- * must be set with either bpf_jit_binary_pack_finalize (normal path) or
- * bpf_arch_text_copy (when jit fails).
  */
 void bpf_jit_binary_pack_free(struct bpf_binary_header *ro_header,
 			      struct bpf_binary_header *rw_header)
-- 
2.34.1