* [PATCH bpf-next v3 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro()
@ 2024-03-08 5:38 Christophe Leroy
2024-03-08 5:38 ` [PATCH bpf-next v3 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro() Christophe Leroy
2024-03-15 2:30 ` [PATCH bpf-next v3 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro() patchwork-bot+netdevbpf
0 siblings, 2 replies; 3+ messages in thread
From: Christophe Leroy @ 2024-03-08 5:38 UTC (permalink / raw
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa
Cc: Christophe Leroy, bpf, linux-arm-kernel, linux-kernel, loongarch,
linux-mips, linux-parisc, linux-s390, sparclinux, netdev,
linux-hardening @ vger . kernel . org, Kees Cook
set_memory_ro() can fail, leaving memory unprotected.
Check its return and take it into account as an error.
Link: https://github.com/KSPP/linux/issues/7
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: linux-hardening@vger.kernel.org <linux-hardening@vger.kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
Note: next patch is autonomous, it is sent as a follow-up of this one to minimize risk of conflict on filter.h because the two changes are too close to each other.
v3: Perform bpf_prog_kallsyms_add() after protection of all subprog have succeeded.
v2: No modification (Just added link in patch message), patchwork discarded this series due to failed test of s390 but it seems unrelated, see https://lore.kernel.org/bpf/wvd5gzde5ejc2rzsbrtwqyof56uw5ea3rxntfrxtkdabzcuwt6@w7iczzhmay2i/T/#m2e61446f42d5dc3d78f2e0e8b7a783f15cfb109d
---
include/linux/filter.h | 5 +++--
kernel/bpf/core.c | 4 +++-
kernel/bpf/verifier.c | 8 ++++++--
3 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 36cc29a2934c..7dd59bccaeec 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -884,14 +884,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 9ee4536d0a09..ae28bab2b138 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2393,7 +2393,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 ee86e4d7d5fc..c97108935c9a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19153,10 +19153,14 @@ 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]);
- bpf_prog_kallsyms_add(func[i]);
+ err = bpf_prog_lock_ro(func[i]);
+ if (err)
+ goto out_free;
}
+ for (i = 1; i < env->subprog_cnt; i++)
+ bpf_prog_kallsyms_add(func[i]);
+
/* Last step: make now unused interpreter insns from main
* prog consistent for later dump requests, so they can
* later look the same as if they were interpreted only.
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH bpf-next v3 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()
2024-03-08 5:38 [PATCH bpf-next v3 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro() Christophe Leroy
@ 2024-03-08 5:38 ` Christophe Leroy
2024-03-15 2:30 ` [PATCH bpf-next v3 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro() patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: Christophe Leroy @ 2024-03-08 5:38 UTC (permalink / raw
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Russell King, Puranjay Mohan, Tiezhu Yang, Hengqi Chen,
Huacai Chen, WANG Xuerui, Johan Almbladh, Paul Burton,
Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
Ilya Leoshkevich, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
David S. Miller, Andreas Larsson, Wang YanQing, David Ahern,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin
Cc: Christophe Leroy, bpf, linux-arm-kernel, linux-kernel, loongarch,
linux-mips, linux-parisc, linux-s390, sparclinux, netdev,
linux-hardening @ vger . kernel . org, Kees Cook
set_memory_rox() can fail, leaving memory unprotected.
Check return and bail out when bpf_jit_binary_lock_ro() returns
an error.
Link: https://github.com/KSPP/linux/issues/7
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: linux-hardening@vger.kernel.org <linux-hardening@vger.kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Puranjay Mohan <puranjay12@gmail.com>
Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com> # s390x
Acked-by: Tiezhu Yang <yangtiezhu@loongson.cn> # LoongArch
Reviewed-by: Johan Almbladh <johan.almbladh@anyfinetworks.com> # MIPS Part
---
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().
v3: No change.
v2:
- Dropped arm64 change following commit 1dad391daef1 ("bpf, arm64: use bpf_prog_pack for memory management")
- Fixed too long lines reported by checkpatch
---
arch/arm/net/bpf_jit_32.c | 25 ++++++++++++-------------
arch/loongarch/net/bpf_jit.c | 22 ++++++++++++++++------
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 | 5 +++--
8 files changed, 51 insertions(+), 27 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/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
index e73323d759d0..7dbefd4ba210 100644
--- a/arch/loongarch/net/bpf_jit.c
+++ b/arch/loongarch/net/bpf_jit.c
@@ -1294,16 +1294,19 @@ 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 +1337,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 7dd59bccaeec..fc42dcfdbd62 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -895,10 +895,11 @@ 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro()
2024-03-08 5:38 [PATCH bpf-next v3 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro() Christophe Leroy
2024-03-08 5:38 ` [PATCH bpf-next v3 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro() Christophe Leroy
@ 2024-03-15 2:30 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-03-15 2:30 UTC (permalink / raw
To: Christophe Leroy
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, bpf,
linux-arm-kernel, linux-kernel, loongarch, linux-mips,
linux-parisc, linux-s390, sparclinux, netdev, linux-hardening,
keescook
Hello:
This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Fri, 8 Mar 2024 06:38:07 +0100 you wrote:
> set_memory_ro() can fail, leaving memory unprotected.
>
> Check its return and take it into account as an error.
>
> Link: https://github.com/KSPP/linux/issues/7
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: linux-hardening@vger.kernel.org <linux-hardening@vger.kernel.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> [...]
Here is the summary with links:
- [bpf-next,v3,1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro()
https://git.kernel.org/bpf/bpf-next/c/7d2cc63eca0c
- [bpf-next,v3,2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()
https://git.kernel.org/bpf/bpf-next/c/e60adf513275
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-03-15 2:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-08 5:38 [PATCH bpf-next v3 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro() Christophe Leroy
2024-03-08 5:38 ` [PATCH bpf-next v3 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro() Christophe Leroy
2024-03-15 2:30 ` [PATCH bpf-next v3 1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro() patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).