[PATCH bpf-next 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro()

[PATCH bpf-next 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro()

[Christophe Leroy]

set_memory_ro() can fail, leaving memory unprotected.

Check its return and take it into account as an error.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 include/linux/filter.h | 5 +++--
 kernel/bpf/core.c      | 4 +++-
 kernel/bpf/verifier.c  | 4 +++-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index fee070b9826e..fc0994dc5c72 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -881,14 +881,15 @@ bpf_ctx_narrow_access_offset(u32 off, u32 size, u32 size_default)
 
 #define bpf_classic_proglen(fprog) (fprog->len * sizeof(fprog->filter[0]))
 
-static inline void bpf_prog_lock_ro(struct bpf_prog *fp)
+static inline int __must_check bpf_prog_lock_ro(struct bpf_prog *fp)
 {
 #ifndef CONFIG_BPF_JIT_ALWAYS_ON
 	if (!fp->jited) {
 		set_vm_flush_reset_perms(fp);
-		set_memory_ro((unsigned long)fp, fp->pages);
+		return set_memory_ro((unsigned long)fp, fp->pages);
 	}
 #endif
+	return 0;
 }
 
 static inline void bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr)
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 71c459a51d9e..c49619ef55d0 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2392,7 +2392,9 @@ struct bpf_prog *bpf_prog_select_runtime(struct bpf_prog *fp, int *err)
 	}
 
 finalize:
-	bpf_prog_lock_ro(fp);
+	*err = bpf_prog_lock_ro(fp);
+	if (*err)
+		return fp;
 
 	/* The tail call compatibility check can only be done at
 	 * this late stage as we need to determine, if we deal
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c5d68a9d8acc..1f831a6b4bbc 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19020,7 +19020,9 @@ static int jit_subprogs(struct bpf_verifier_env *env)
 	 * bpf_prog_load will add the kallsyms for the main program.
 	 */
 	for (i = 1; i < env->subprog_cnt; i++) {
-		bpf_prog_lock_ro(func[i]);
+		err = bpf_prog_lock_ro(func[i]);
+		if (err)
+			goto out_free;
 		bpf_prog_kallsyms_add(func[i]);
 	}
 
-- 
2.43.0

[PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()

[Christophe Leroy]

set_memory_rox() can fail, leaving memory unprotected.

Check return and bail out when bpf_jit_binary_lock_ro() returns
and error.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
Previous patch introduces a dependency on this patch because it modifies bpf_prog_lock_ro(), but they are independant.
It is possible to apply this patch as standalone by handling trivial conflict with unmodified bpf_prog_lock_ro().
---
 arch/arm/net/bpf_jit_32.c        | 25 ++++++++++++-------------
 arch/arm64/net/bpf_jit_comp.c    | 21 +++++++++++++++------
 arch/loongarch/net/bpf_jit.c     | 21 +++++++++++++++------
 arch/mips/net/bpf_jit_comp.c     |  3 ++-
 arch/parisc/net/bpf_jit_core.c   |  8 +++++++-
 arch/s390/net/bpf_jit_comp.c     |  6 +++++-
 arch/sparc/net/bpf_jit_comp_64.c |  6 +++++-
 arch/x86/net/bpf_jit_comp32.c    |  3 +--
 include/linux/filter.h           |  4 ++--
 9 files changed, 64 insertions(+), 33 deletions(-)

diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index 1d672457d02f..01516f83a95a 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -2222,28 +2222,21 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 	/* If building the body of the JITed code fails somehow,
 	 * we fall back to the interpretation.
 	 */
-	if (build_body(&ctx) < 0) {
-		image_ptr = NULL;
-		bpf_jit_binary_free(header);
-		prog = orig_prog;
-		goto out_imms;
-	}
+	if (build_body(&ctx) < 0)
+		goto out_free;
 	build_epilogue(&ctx);
 
 	/* 3.) Extra pass to validate JITed Code */
-	if (validate_code(&ctx)) {
-		image_ptr = NULL;
-		bpf_jit_binary_free(header);
-		prog = orig_prog;
-		goto out_imms;
-	}
+	if (validate_code(&ctx))
+		goto out_free;
 	flush_icache_range((u32)header, (u32)(ctx.target + ctx.idx));
 
 	if (bpf_jit_enable > 1)
 		/* there are 2 passes here */
 		bpf_jit_dump(prog->len, image_size, 2, ctx.target);
 
-	bpf_jit_binary_lock_ro(header);
+	if (bpf_jit_binary_lock_ro(header))
+		goto out_free;
 	prog->bpf_func = (void *)ctx.target;
 	prog->jited = 1;
 	prog->jited_len = image_size;
@@ -2260,5 +2253,11 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 		bpf_jit_prog_release_other(prog, prog == orig_prog ?
 					   tmp : orig_prog);
 	return prog;
+
+out_free:
+	image_ptr = NULL;
+	bpf_jit_binary_free(header);
+	prog = orig_prog;
+	goto out_imms;
 }
 
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index cfd5434de483..21a901d61aa1 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -1639,16 +1639,18 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 	bpf_flush_icache(header, ctx.image + ctx.idx);
 
 	if (!prog->is_func || extra_pass) {
+		int err;
+
 		if (extra_pass && ctx.idx != jit_data->ctx.idx) {
 			pr_err_once("multi-func JIT bug %d != %d\n",
 				    ctx.idx, jit_data->ctx.idx);
-			bpf_jit_binary_free(header);
-			prog->bpf_func = NULL;
-			prog->jited = 0;
-			prog->jited_len = 0;
-			goto out_off;
+			goto out_free;
+		}
+		err = bpf_jit_binary_lock_ro(header);
+		if (err) {
+			pr_err_once("bpf_jit_binary_lock_ro() returned %d\n", err);
+			goto out_free;
 		}
-		bpf_jit_binary_lock_ro(header);
 	} else {
 		jit_data->ctx = ctx;
 		jit_data->image = image_ptr;
@@ -1675,6 +1677,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 		bpf_jit_prog_release_other(prog, prog == orig_prog ?
 					   tmp : orig_prog);
 	return prog;
+
+out_free:
+	bpf_jit_binary_free(header);
+	prog->bpf_func = NULL;
+	prog->jited = 0;
+	prog->jited_len = 0;
+	goto out_off;
 }
 
 bool bpf_jit_supports_kfunc_call(void)
diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
index e73323d759d0..aafc5037fd2b 100644
--- a/arch/loongarch/net/bpf_jit.c
+++ b/arch/loongarch/net/bpf_jit.c
@@ -1294,16 +1294,18 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 	flush_icache_range((unsigned long)header, (unsigned long)(ctx.image + ctx.idx));
 
 	if (!prog->is_func || extra_pass) {
+		int err;
+
 		if (extra_pass && ctx.idx != jit_data->ctx.idx) {
 			pr_err_once("multi-func JIT bug %d != %d\n",
 				    ctx.idx, jit_data->ctx.idx);
-			bpf_jit_binary_free(header);
-			prog->bpf_func = NULL;
-			prog->jited = 0;
-			prog->jited_len = 0;
-			goto out_offset;
+			goto out_free;
+		}
+		err = bpf_jit_binary_lock_ro(header);
+		if (err) {
+			pr_err_once("bpf_jit_binary_lock_ro() returned %d\n", err);
+			goto out_free;
 		}
-		bpf_jit_binary_lock_ro(header);
 	} else {
 		jit_data->ctx = ctx;
 		jit_data->image = image_ptr;
@@ -1334,6 +1336,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 	out_offset = -1;
 
 	return prog;
+
+out_free:
+	bpf_jit_binary_free(header);
+	prog->bpf_func = NULL;
+	prog->jited = 0;
+	prog->jited_len = 0;
+	goto out_offset;
 }
 
 /* Indicate the JIT backend supports mixing bpf2bpf and tailcalls. */
diff --git a/arch/mips/net/bpf_jit_comp.c b/arch/mips/net/bpf_jit_comp.c
index a40d926b6513..e355dfca4400 100644
--- a/arch/mips/net/bpf_jit_comp.c
+++ b/arch/mips/net/bpf_jit_comp.c
@@ -1012,7 +1012,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 	bpf_prog_fill_jited_linfo(prog, &ctx.descriptors[1]);
 
 	/* Set as read-only exec and flush instruction cache */
-	bpf_jit_binary_lock_ro(header);
+	if (bpf_jit_binary_lock_ro(header))
+		goto out_err;
 	flush_icache_range((unsigned long)header,
 			   (unsigned long)&ctx.target[ctx.jit_index]);
 
diff --git a/arch/parisc/net/bpf_jit_core.c b/arch/parisc/net/bpf_jit_core.c
index d6ee2fd45550..979f45d4d1fb 100644
--- a/arch/parisc/net/bpf_jit_core.c
+++ b/arch/parisc/net/bpf_jit_core.c
@@ -167,7 +167,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 	bpf_flush_icache(jit_data->header, ctx->insns + ctx->ninsns);
 
 	if (!prog->is_func || extra_pass) {
-		bpf_jit_binary_lock_ro(jit_data->header);
+		if (bpf_jit_binary_lock_ro(jit_data->header)) {
+			bpf_jit_binary_free(jit_data->header);
+			prog->bpf_func = NULL;
+			prog->jited = 0;
+			prog->jited_len = 0;
+			goto out_offset;
+		}
 		prologue_len = ctx->epilogue_offset - ctx->body_len;
 		for (i = 0; i < prog->len; i++)
 			ctx->offset[i] += prologue_len;
diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index b418333bb086..e613eebfd349 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -2111,7 +2111,11 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
 		print_fn_code(jit.prg_buf, jit.size_prg);
 	}
 	if (!fp->is_func || extra_pass) {
-		bpf_jit_binary_lock_ro(header);
+		if (bpf_jit_binary_lock_ro(header)) {
+			bpf_jit_binary_free(header);
+			fp = orig_fp;
+			goto free_addrs;
+		}
 	} else {
 		jit_data->header = header;
 		jit_data->ctx = jit;
diff --git a/arch/sparc/net/bpf_jit_comp_64.c b/arch/sparc/net/bpf_jit_comp_64.c
index fa0759bfe498..73bf0aea8baf 100644
--- a/arch/sparc/net/bpf_jit_comp_64.c
+++ b/arch/sparc/net/bpf_jit_comp_64.c
@@ -1602,7 +1602,11 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 	bpf_flush_icache(header, (u8 *)header + header->size);
 
 	if (!prog->is_func || extra_pass) {
-		bpf_jit_binary_lock_ro(header);
+		if (bpf_jit_binary_lock_ro(header)) {
+			bpf_jit_binary_free(header);
+			prog = orig_prog;
+			goto out_off;
+		}
 	} else {
 		jit_data->ctx = ctx;
 		jit_data->image = image_ptr;
diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c
index b18ce19981ec..f2be1dcf3b24 100644
--- a/arch/x86/net/bpf_jit_comp32.c
+++ b/arch/x86/net/bpf_jit_comp32.c
@@ -2600,8 +2600,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 	if (bpf_jit_enable > 1)
 		bpf_jit_dump(prog->len, proglen, pass + 1, image);
 
-	if (image) {
-		bpf_jit_binary_lock_ro(header);
+	if (image && !bpf_jit_binary_lock_ro(header)) {
 		prog->bpf_func = (void *)image;
 		prog->jited = 1;
 		prog->jited_len = proglen;
diff --git a/include/linux/filter.h b/include/linux/filter.h
index fc0994dc5c72..314414fa6d70 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -892,10 +892,10 @@ static inline int __must_check bpf_prog_lock_ro(struct bpf_prog *fp)
 	return 0;
 }
 
-static inline void bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr)
+static inline int __must_check bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr)
 {
 	set_vm_flush_reset_perms(hdr);
-	set_memory_rox((unsigned long)hdr, hdr->size >> PAGE_SHIFT);
+	return set_memory_rox((unsigned long)hdr, hdr->size >> PAGE_SHIFT);
 }
 
 int sk_filter_trim_cap(struct sock *sk, struct sk_buff *skb, unsigned int cap);
-- 
2.43.0

Re: [PATCH bpf-next 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro()

[Kees Cook]

On Sun, Feb 18, 2024 at 11:55:01AM +0100, Christophe Leroy wrote:
> set_memory_ro() can fail, leaving memory unprotected.
> 
> Check its return and take it into account as an error.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  include/linux/filter.h | 5 +++--
>  kernel/bpf/core.c      | 4 +++-
>  kernel/bpf/verifier.c  | 4 +++-
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index fee070b9826e..fc0994dc5c72 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -881,14 +881,15 @@ bpf_ctx_narrow_access_offset(u32 off, u32 size, u32 size_default)
>  
>  #define bpf_classic_proglen(fprog) (fprog->len * sizeof(fprog->filter[0]))
>  
> -static inline void bpf_prog_lock_ro(struct bpf_prog *fp)
> +static inline int __must_check bpf_prog_lock_ro(struct bpf_prog *fp)
>  {
>  #ifndef CONFIG_BPF_JIT_ALWAYS_ON
>  	if (!fp->jited) {
>  		set_vm_flush_reset_perms(fp);
> -		set_memory_ro((unsigned long)fp, fp->pages);
> +		return set_memory_ro((unsigned long)fp, fp->pages);
>  	}
>  #endif
> +	return 0;
>  }
>  
>  static inline void bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr)
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 71c459a51d9e..c49619ef55d0 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2392,7 +2392,9 @@ struct bpf_prog *bpf_prog_select_runtime(struct bpf_prog *fp, int *err)
>  	}
>  
>  finalize:
> -	bpf_prog_lock_ro(fp);
> +	*err = bpf_prog_lock_ro(fp);
> +	if (*err)
> +		return fp;

Weird error path, but yes.

>  
>  	/* The tail call compatibility check can only be done at
>  	 * this late stage as we need to determine, if we deal
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c5d68a9d8acc..1f831a6b4bbc 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19020,7 +19020,9 @@ static int jit_subprogs(struct bpf_verifier_env *env)
>  	 * bpf_prog_load will add the kallsyms for the main program.
>  	 */
>  	for (i = 1; i < env->subprog_cnt; i++) {
> -		bpf_prog_lock_ro(func[i]);
> +		err = bpf_prog_lock_ro(func[i]);
> +		if (err)
> +			goto out_free;
>  		bpf_prog_kallsyms_add(func[i]);
>  	}

Just to double-check if memory permissions being correctly restored on
this error path, I walked back through it and see that it ultimately
lands on vfree(), which appears to just throw the entire mapping away,
so I think that's safe. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()

[Kees Cook]

On Sun, Feb 18, 2024 at 11:55:02AM +0100, Christophe Leroy wrote:
> set_memory_rox() can fail, leaving memory unprotected.
> 
> Check return and bail out when bpf_jit_binary_lock_ro() returns
> and error.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> Previous patch introduces a dependency on this patch because it modifies bpf_prog_lock_ro(), but they are independant.
> It is possible to apply this patch as standalone by handling trivial conflict with unmodified bpf_prog_lock_ro().
> ---
>  arch/arm/net/bpf_jit_32.c        | 25 ++++++++++++-------------
>  arch/arm64/net/bpf_jit_comp.c    | 21 +++++++++++++++------
>  arch/loongarch/net/bpf_jit.c     | 21 +++++++++++++++------
>  arch/mips/net/bpf_jit_comp.c     |  3 ++-
>  arch/parisc/net/bpf_jit_core.c   |  8 +++++++-
>  arch/s390/net/bpf_jit_comp.c     |  6 +++++-
>  arch/sparc/net/bpf_jit_comp_64.c |  6 +++++-
>  arch/x86/net/bpf_jit_comp32.c    |  3 +--
>  include/linux/filter.h           |  4 ++--
>  9 files changed, 64 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> index 1d672457d02f..01516f83a95a 100644
> --- a/arch/arm/net/bpf_jit_32.c
> +++ b/arch/arm/net/bpf_jit_32.c
> @@ -2222,28 +2222,21 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>  	/* If building the body of the JITed code fails somehow,
>  	 * we fall back to the interpretation.
>  	 */
> -	if (build_body(&ctx) < 0) {
> -		image_ptr = NULL;
> -		bpf_jit_binary_free(header);
> -		prog = orig_prog;
> -		goto out_imms;
> -	}
> +	if (build_body(&ctx) < 0)
> +		goto out_free;
>  	build_epilogue(&ctx);
>  
>  	/* 3.) Extra pass to validate JITed Code */
> -	if (validate_code(&ctx)) {
> -		image_ptr = NULL;
> -		bpf_jit_binary_free(header);
> -		prog = orig_prog;
> -		goto out_imms;
> -	}
> +	if (validate_code(&ctx))
> +		goto out_free;
>  	flush_icache_range((u32)header, (u32)(ctx.target + ctx.idx));
>  
>  	if (bpf_jit_enable > 1)
>  		/* there are 2 passes here */
>  		bpf_jit_dump(prog->len, image_size, 2, ctx.target);
>  
> -	bpf_jit_binary_lock_ro(header);
> +	if (bpf_jit_binary_lock_ro(header))
> +		goto out_free;
>  	prog->bpf_func = (void *)ctx.target;
>  	prog->jited = 1;
>  	prog->jited_len = image_size;
> @@ -2260,5 +2253,11 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>  		bpf_jit_prog_release_other(prog, prog == orig_prog ?
>  					   tmp : orig_prog);
>  	return prog;
> +
> +out_free:
> +	image_ptr = NULL;
> +	bpf_jit_binary_free(header);
> +	prog = orig_prog;
> +	goto out_imms;

These gotos give me the creeps, but yes, it does appear to be in the
style of the existing error handling.

> [...]
> diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c
> index b18ce19981ec..f2be1dcf3b24 100644
> --- a/arch/x86/net/bpf_jit_comp32.c
> +++ b/arch/x86/net/bpf_jit_comp32.c
> @@ -2600,8 +2600,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>  	if (bpf_jit_enable > 1)
>  		bpf_jit_dump(prog->len, proglen, pass + 1, image);
>  
> -	if (image) {
> -		bpf_jit_binary_lock_ro(header);
> +	if (image && !bpf_jit_binary_lock_ro(header)) {

I find the "!" kind of hard to read the "inverted" logic (0 is success),
so if this gets a revision, maybe do "== 0"?:

	if (image && bpf_jit_binary_lock_ro(header) == 0) {

But that's just me. So, regardless:

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH bpf-next 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro()

[Hengqi Chen]

Hello Christophe,

On Sun, Feb 18, 2024 at 6:55 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
> set_memory_ro() can fail, leaving memory unprotected.
>
> Check its return and take it into account as an error.
>

I don't see a cover letter for this series, could you describe how
set_memory_ro() could fail.
(Most callsites of set_memory_ro() didn't check the return values)

And maybe craft a selftest to check the expected return values.

> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  include/linux/filter.h | 5 +++--
>  kernel/bpf/core.c      | 4 +++-
>  kernel/bpf/verifier.c  | 4 +++-
>  3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index fee070b9826e..fc0994dc5c72 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -881,14 +881,15 @@ bpf_ctx_narrow_access_offset(u32 off, u32 size, u32 size_default)
>
>  #define bpf_classic_proglen(fprog) (fprog->len * sizeof(fprog->filter[0]))
>
> -static inline void bpf_prog_lock_ro(struct bpf_prog *fp)
> +static inline int __must_check bpf_prog_lock_ro(struct bpf_prog *fp)
>  {
>  #ifndef CONFIG_BPF_JIT_ALWAYS_ON
>         if (!fp->jited) {
>                 set_vm_flush_reset_perms(fp);
> -               set_memory_ro((unsigned long)fp, fp->pages);
> +               return set_memory_ro((unsigned long)fp, fp->pages);
>         }
>  #endif
> +       return 0;
>  }
>
>  static inline void bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr)
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 71c459a51d9e..c49619ef55d0 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2392,7 +2392,9 @@ struct bpf_prog *bpf_prog_select_runtime(struct bpf_prog *fp, int *err)
>         }
>
>  finalize:
> -       bpf_prog_lock_ro(fp);
> +       *err = bpf_prog_lock_ro(fp);
> +       if (*err)
> +               return fp;
>
>         /* The tail call compatibility check can only be done at
>          * this late stage as we need to determine, if we deal
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c5d68a9d8acc..1f831a6b4bbc 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19020,7 +19020,9 @@ static int jit_subprogs(struct bpf_verifier_env *env)
>          * bpf_prog_load will add the kallsyms for the main program.
>          */
>         for (i = 1; i < env->subprog_cnt; i++) {
> -               bpf_prog_lock_ro(func[i]);
> +               err = bpf_prog_lock_ro(func[i]);
> +               if (err)
> +                       goto out_free;
>                 bpf_prog_kallsyms_add(func[i]);
>         }
>
> --
> 2.43.0
>
>

Re: [PATCH bpf-next 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro()

[Christophe Leroy]

Le 19/02/2024 à 02:40, Hengqi Chen a écrit :
> [Vous ne recevez pas souvent de courriers de hengqi.chen@gmail.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hello Christophe,
> 
> On Sun, Feb 18, 2024 at 6:55 PM Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
>>
>> set_memory_ro() can fail, leaving memory unprotected.
>>
>> Check its return and take it into account as an error.
>>
> 
> I don't see a cover letter for this series, could you describe how
> set_memory_ro() could fail.
> (Most callsites of set_memory_ro() didn't check the return values)

Yeah, there is no cover letter because as explained in patch 2 the two 
patches are autonomous. The only reason why I sent it as a series is 
because the patches both modify include/linux/filter.h in two places 
that are too close to each other.

I should have added a link to https://github.com/KSPP/linux/issues/7
See that link for detailed explanation.

If we take powerpc as an exemple, set_memory_ro() is a frontend to 
change_memory_attr(). When you look at change_memory_attr() you see it 
can return -EINVAL in two cases. Then it calls 
apply_to_existing_page_range(). When you go down the road you see you 
can get -EINVAL or -ENOMEM from that function or its callees.

Christophe

Re: [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()

[Puranjay Mohan]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:

> set_memory_rox() can fail, leaving memory unprotected.
>
> Check return and bail out when bpf_jit_binary_lock_ro() returns
> and error.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> Previous patch introduces a dependency on this patch because it modifies bpf_prog_lock_ro(), but they are independant.
> It is possible to apply this patch as standalone by handling trivial conflict with unmodified bpf_prog_lock_ro().
> ---
>  arch/arm/net/bpf_jit_32.c        | 25 ++++++++++++-------------
>  arch/arm64/net/bpf_jit_comp.c    | 21 +++++++++++++++------
>  arch/loongarch/net/bpf_jit.c     | 21 +++++++++++++++------
>  arch/mips/net/bpf_jit_comp.c     |  3 ++-
>  arch/parisc/net/bpf_jit_core.c   |  8 +++++++-
>  arch/s390/net/bpf_jit_comp.c     |  6 +++++-
>  arch/sparc/net/bpf_jit_comp_64.c |  6 +++++-
>  arch/x86/net/bpf_jit_comp32.c    |  3 +--
>  include/linux/filter.h           |  4 ++--
>  9 files changed, 64 insertions(+), 33 deletions(-)
>

Reviewed-by: Puranjay Mohan <puranjay12@gmail.com>

Thanks,
Puranjay Mohan

Re: [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()

[Ilya Leoshkevich]

On Sun, 2024-02-18 at 11:55 +0100, Christophe Leroy wrote:
> set_memory_rox() can fail, leaving memory unprotected.
> 
> Check return and bail out when bpf_jit_binary_lock_ro() returns
> and error.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> Previous patch introduces a dependency on this patch because it
> modifies bpf_prog_lock_ro(), but they are independant.
> It is possible to apply this patch as standalone by handling trivial
> conflict with unmodified bpf_prog_lock_ro().
> ---
>  arch/arm/net/bpf_jit_32.c        | 25 ++++++++++++-------------
>  arch/arm64/net/bpf_jit_comp.c    | 21 +++++++++++++++------
>  arch/loongarch/net/bpf_jit.c     | 21 +++++++++++++++------
>  arch/mips/net/bpf_jit_comp.c     |  3 ++-
>  arch/parisc/net/bpf_jit_core.c   |  8 +++++++-
>  arch/s390/net/bpf_jit_comp.c     |  6 +++++-
>  arch/sparc/net/bpf_jit_comp_64.c |  6 +++++-
>  arch/x86/net/bpf_jit_comp32.c    |  3 +--
>  include/linux/filter.h           |  4 ++--
>  9 files changed, 64 insertions(+), 33 deletions(-)

Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>  # s390x

Re: [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()

[Tiezhu Yang]

On 02/18/2024 06:55 PM, Christophe Leroy wrote:
> set_memory_rox() can fail, leaving memory unprotected.
>
> Check return and bail out when bpf_jit_binary_lock_ro() returns
> and error.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---

...

> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index e73323d759d0..aafc5037fd2b 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c
> @@ -1294,16 +1294,18 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>  	flush_icache_range((unsigned long)header, (unsigned long)(ctx.image + ctx.idx));
>
>  	if (!prog->is_func || extra_pass) {
> +		int err;
> +
>  		if (extra_pass && ctx.idx != jit_data->ctx.idx) {
>  			pr_err_once("multi-func JIT bug %d != %d\n",
>  				    ctx.idx, jit_data->ctx.idx);
> -			bpf_jit_binary_free(header);
> -			prog->bpf_func = NULL;
> -			prog->jited = 0;
> -			prog->jited_len = 0;
> -			goto out_offset;
> +			goto out_free;
> +		}
> +		err = bpf_jit_binary_lock_ro(header);
> +		if (err) {
> +			pr_err_once("bpf_jit_binary_lock_ro() returned %d\n", err);
> +			goto out_free;
>  		}
> -		bpf_jit_binary_lock_ro(header);
>  	} else {
>  		jit_data->ctx = ctx;
>  		jit_data->image = image_ptr;
> @@ -1334,6 +1336,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>  	out_offset = -1;
>
>  	return prog;
> +
> +out_free:
> +	bpf_jit_binary_free(header);
> +	prog->bpf_func = NULL;
> +	prog->jited = 0;
> +	prog->jited_len = 0;
> +	goto out_offset;
>  }
>
>  /* Indicate the JIT backend supports mixing bpf2bpf and tailcalls. */

...

> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index fc0994dc5c72..314414fa6d70 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -892,10 +892,10 @@ static inline int __must_check bpf_prog_lock_ro(struct bpf_prog *fp)
>  	return 0;
>  }
>
> -static inline void bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr)
> +static inline int __must_check bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr)
>  {
>  	set_vm_flush_reset_perms(hdr);
> -	set_memory_rox((unsigned long)hdr, hdr->size >> PAGE_SHIFT);
> +	return set_memory_rox((unsigned long)hdr, hdr->size >> PAGE_SHIFT);
>  }
>
>  int sk_filter_trim_cap(struct sock *sk, struct sk_buff *skb, unsigned int cap);

LoongArch does not select CONFIG_ARCH_HAS_SET_MEMORY, set_memory_ro()
and set_memory_x() always return 0, then set_memory_rox() also returns
0, that is to say, bpf_jit_binary_lock_ro() will return 0, it seems
that there is no obvious effect for LoongArch with this patch.

But once CONFIG_ARCH_HAS_SET_MEMORY is selected and the arch-specified
set_memory_*() functions are implemented in the future, it is necessary
to handle the error cases. At least, in order to keep consistent with
the other archs, the code itself looks good to me.

Acked-by: Tiezhu Yang <yangtiezhu@loongson.cn>  # LoongArch

Thanks,
Tiezhu

Re: [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()

[Johan Almbladh]

On Sun, Feb 18, 2024 at 11:55 AM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
> set_memory_rox() can fail, leaving memory unprotected.
>
> Check return and bail out when bpf_jit_binary_lock_ro() returns
> and error.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> Previous patch introduces a dependency on this patch because it modifies bpf_prog_lock_ro(), but they are independant.
> It is possible to apply this patch as standalone by handling trivial conflict with unmodified bpf_prog_lock_ro().
> ---
>  arch/arm/net/bpf_jit_32.c        | 25 ++++++++++++-------------
>  arch/arm64/net/bpf_jit_comp.c    | 21 +++++++++++++++------
>  arch/loongarch/net/bpf_jit.c     | 21 +++++++++++++++------
>  arch/mips/net/bpf_jit_comp.c     |  3 ++-
>  arch/parisc/net/bpf_jit_core.c   |  8 +++++++-
>  arch/s390/net/bpf_jit_comp.c     |  6 +++++-
>  arch/sparc/net/bpf_jit_comp_64.c |  6 +++++-
>  arch/x86/net/bpf_jit_comp32.c    |  3 +--
>  include/linux/filter.h           |  4 ++--
>  9 files changed, 64 insertions(+), 33 deletions(-)

For the MIPS part:
Reviewed-by: Johan Almbladh <johan.almbladh@anyfinetworks.com>

Thanks,
Johan

Re: [PATCH bpf-next 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro()

[Daniel Borkmann]

On 2/19/24 7:39 AM, Christophe Leroy wrote:
> 
> 
> Le 19/02/2024 à 02:40, Hengqi Chen a écrit :
>> [Vous ne recevez pas souvent de courriers de hengqi.chen@gmail.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>>
>> Hello Christophe,
>>
>> On Sun, Feb 18, 2024 at 6:55 PM Christophe Leroy
>> <christophe.leroy@csgroup.eu> wrote:
>>>
>>> set_memory_ro() can fail, leaving memory unprotected.
>>>
>>> Check its return and take it into account as an error.
>>>
>>
>> I don't see a cover letter for this series, could you describe how
>> set_memory_ro() could fail.
>> (Most callsites of set_memory_ro() didn't check the return values)
> 
> Yeah, there is no cover letter because as explained in patch 2 the two
> patches are autonomous. The only reason why I sent it as a series is
> because the patches both modify include/linux/filter.h in two places
> that are too close to each other.
> 
> I should have added a link to https://github.com/KSPP/linux/issues/7
> See that link for detailed explanation.
> 
> If we take powerpc as an exemple, set_memory_ro() is a frontend to
> change_memory_attr(). When you look at change_memory_attr() you see it
> can return -EINVAL in two cases. Then it calls
> apply_to_existing_page_range(). When you go down the road you see you
> can get -EINVAL or -ENOMEM from that function or its callees.

By that logic, don't you have the same issue when undoing all of this?
E.g. take arch_protect_bpf_trampoline() / arch_unprotect_bpf_trampoline()
which is not covered in here, but what happens if you set it first to ro
and later the setting back to rw fails? How would the error path there
look like? It's something you cannot recover.

Thanks,
Daniel

Re: [PATCH bpf-next 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro()

[Christophe Leroy]

Le 21/02/2024 à 18:30, Daniel Borkmann a écrit :
> On 2/19/24 7:39 AM, Christophe Leroy wrote:
>>
>>
>> Le 19/02/2024 à 02:40, Hengqi Chen a écrit :
>>> [Vous ne recevez pas souvent de courriers de hengqi.chen@gmail.com. 
>>> Découvrez pourquoi ceci est important à 
>>> https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> Hello Christophe,
>>>
>>> On Sun, Feb 18, 2024 at 6:55 PM Christophe Leroy
>>> <christophe.leroy@csgroup.eu> wrote:
>>>>
>>>> set_memory_ro() can fail, leaving memory unprotected.
>>>>
>>>> Check its return and take it into account as an error.
>>>>
>>>
>>> I don't see a cover letter for this series, could you describe how
>>> set_memory_ro() could fail.
>>> (Most callsites of set_memory_ro() didn't check the return values)
>>
>> Yeah, there is no cover letter because as explained in patch 2 the two
>> patches are autonomous. The only reason why I sent it as a series is
>> because the patches both modify include/linux/filter.h in two places
>> that are too close to each other.
>>
>> I should have added a link to https://github.com/KSPP/linux/issues/7
>> See that link for detailed explanation.
>>
>> If we take powerpc as an exemple, set_memory_ro() is a frontend to
>> change_memory_attr(). When you look at change_memory_attr() you see it
>> can return -EINVAL in two cases. Then it calls
>> apply_to_existing_page_range(). When you go down the road you see you
>> can get -EINVAL or -ENOMEM from that function or its callees.
> 
> By that logic, don't you have the same issue when undoing all of this?
> E.g. take arch_protect_bpf_trampoline() / arch_unprotect_bpf_trampoline()
> which is not covered in here, but what happens if you set it first to ro
> and later the setting back to rw fails? How would the error path there
> look like? It's something you cannot recover.
> 

arch_protect_bpf_trampoline() is handled there 
https://patchwork.kernel.org/project/netdevbpf/patch/883c5a268483a89ab13ed630210328a926f16e5b.1708526584.git.christophe.leroy@csgroup.eu/

In case setting back to RW fails there is not security issue, the things 
will likely blow up later with a write access to write protected memory 
but in terms of security that's not a problem.