All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf-next v3 1/2] bpf: Remove arch_unprotect_bpf_trampoline()
@ 2024-03-15 17:06 ` Christophe Leroy
  0 siblings, 0 replies; 14+ messages in thread
From: Christophe Leroy @ 2024-03-15 17:06 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,
	Zi Shen Lim, Catalin Marinas, Will Deacon, David S. Miller,
	David Ahern, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H. Peter Anvin, Kui-Feng Lee, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni
  Cc: Christophe Leroy, bpf, linux-arm-kernel, linux-kernel, netdev

Last user of arch_unprotect_bpf_trampoline() was removed by
commit 187e2af05abe ("bpf: struct_ops supports more than one page for
trampolines.")

Remove arch_unprotect_bpf_trampoline()

Reported-by: Daniel Borkmann <daniel@iogearbox.net>
Fixes: 187e2af05abe ("bpf: struct_ops supports more than one page for trampolines.")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v3: New
---
 arch/arm64/net/bpf_jit_comp.c | 4 ----
 arch/x86/net/bpf_jit_comp.c   | 4 ----
 include/linux/bpf.h           | 1 -
 kernel/bpf/trampoline.c       | 7 -------
 4 files changed, 16 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index c5b461dda438..132c8ffba109 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -2180,10 +2180,6 @@ void arch_protect_bpf_trampoline(void *image, unsigned int size)
 {
 }
 
-void arch_unprotect_bpf_trampoline(void *image, unsigned int size)
-{
-}
-
 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *ro_image,
 				void *ro_image_end, const struct btf_func_model *m,
 				u32 flags, struct bpf_tramp_links *tlinks,
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index a7ba8e178645..7a56d2d84512 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -3008,10 +3008,6 @@ void arch_protect_bpf_trampoline(void *image, unsigned int size)
 {
 }
 
-void arch_unprotect_bpf_trampoline(void *image, unsigned int size)
-{
-}
-
 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end,
 				const struct btf_func_model *m, u32 flags,
 				struct bpf_tramp_links *tlinks,
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 4f20f62f9d63..d89bdefb42e2 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1117,7 +1117,6 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 void *arch_alloc_bpf_trampoline(unsigned int size);
 void arch_free_bpf_trampoline(void *image, unsigned int size);
 void arch_protect_bpf_trampoline(void *image, unsigned int size);
-void arch_unprotect_bpf_trampoline(void *image, unsigned int size);
 int arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags,
 			     struct bpf_tramp_links *tlinks, void *func_addr);
 
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index db7599c59c78..04fd1abd3661 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -1078,13 +1078,6 @@ void __weak arch_protect_bpf_trampoline(void *image, unsigned int size)
 	set_memory_rox((long)image, 1);
 }
 
-void __weak arch_unprotect_bpf_trampoline(void *image, unsigned int size)
-{
-	WARN_ON_ONCE(size > PAGE_SIZE);
-	set_memory_nx((long)image, 1);
-	set_memory_rw((long)image, 1);
-}
-
 int __weak arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags,
 				    struct bpf_tramp_links *tlinks, void *func_addr)
 {
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH bpf-next v3 1/2] bpf: Remove arch_unprotect_bpf_trampoline()
@ 2024-03-15 17:06 ` Christophe Leroy
  0 siblings, 0 replies; 14+ messages in thread
From: Christophe Leroy @ 2024-03-15 17:06 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,
	Zi Shen Lim, Catalin Marinas, Will Deacon, David S. Miller,
	David Ahern, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H. Peter Anvin, Kui-Feng Lee, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni
  Cc: Christophe Leroy, bpf, linux-arm-kernel, linux-kernel, netdev

Last user of arch_unprotect_bpf_trampoline() was removed by
commit 187e2af05abe ("bpf: struct_ops supports more than one page for
trampolines.")

Remove arch_unprotect_bpf_trampoline()

Reported-by: Daniel Borkmann <daniel@iogearbox.net>
Fixes: 187e2af05abe ("bpf: struct_ops supports more than one page for trampolines.")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v3: New
---
 arch/arm64/net/bpf_jit_comp.c | 4 ----
 arch/x86/net/bpf_jit_comp.c   | 4 ----
 include/linux/bpf.h           | 1 -
 kernel/bpf/trampoline.c       | 7 -------
 4 files changed, 16 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index c5b461dda438..132c8ffba109 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -2180,10 +2180,6 @@ void arch_protect_bpf_trampoline(void *image, unsigned int size)
 {
 }
 
-void arch_unprotect_bpf_trampoline(void *image, unsigned int size)
-{
-}
-
 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *ro_image,
 				void *ro_image_end, const struct btf_func_model *m,
 				u32 flags, struct bpf_tramp_links *tlinks,
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index a7ba8e178645..7a56d2d84512 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -3008,10 +3008,6 @@ void arch_protect_bpf_trampoline(void *image, unsigned int size)
 {
 }
 
-void arch_unprotect_bpf_trampoline(void *image, unsigned int size)
-{
-}
-
 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end,
 				const struct btf_func_model *m, u32 flags,
 				struct bpf_tramp_links *tlinks,
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 4f20f62f9d63..d89bdefb42e2 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1117,7 +1117,6 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 void *arch_alloc_bpf_trampoline(unsigned int size);
 void arch_free_bpf_trampoline(void *image, unsigned int size);
 void arch_protect_bpf_trampoline(void *image, unsigned int size);
-void arch_unprotect_bpf_trampoline(void *image, unsigned int size);
 int arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags,
 			     struct bpf_tramp_links *tlinks, void *func_addr);
 
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index db7599c59c78..04fd1abd3661 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -1078,13 +1078,6 @@ void __weak arch_protect_bpf_trampoline(void *image, unsigned int size)
 	set_memory_rox((long)image, 1);
 }
 
-void __weak arch_unprotect_bpf_trampoline(void *image, unsigned int size)
-{
-	WARN_ON_ONCE(size > PAGE_SIZE);
-	set_memory_nx((long)image, 1);
-	set_memory_rw((long)image, 1);
-}
-
 int __weak arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags,
 				    struct bpf_tramp_links *tlinks, void *func_addr)
 {
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH bpf-next v3 2/2] bpf: Check return from set_memory_rox()
  2024-03-15 17:06 ` Christophe Leroy
@ 2024-03-15 17:06   ` Christophe Leroy
  -1 siblings, 0 replies; 14+ messages in thread
From: Christophe Leroy @ 2024-03-15 17:06 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,
	Zi Shen Lim, Catalin Marinas, Will Deacon, David S. Miller,
	David Ahern, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H. Peter Anvin, Kui-Feng Lee, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni
  Cc: Christophe Leroy, bpf, linux-arm-kernel, linux-kernel, netdev, Kees Cook

arch_protect_bpf_trampoline() and alloc_new_pack() call
set_memory_rox() which can fail, leading to unprotected memory.

Take into account return from set_memory_rox() function and add
__must_check flag to arch_protect_bpf_trampoline().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
v3:
- Rebased and handled conflict in kernel/bpf/bpf_struct_ops.c

v2:
- Move list_add_tail(&pack->list, &pack_list) at the end of alloc_new_pack()
- Split 2 lines that are reported longer than 80 chars by BPF patchwork's checkpatch report.
---
 arch/arm64/net/bpf_jit_comp.c  |  3 ++-
 arch/x86/net/bpf_jit_comp.c    |  3 ++-
 include/linux/bpf.h            |  2 +-
 kernel/bpf/bpf_struct_ops.c    |  7 +++++--
 kernel/bpf/core.c              | 29 ++++++++++++++++++++++-------
 kernel/bpf/trampoline.c        |  8 +++++---
 net/bpf/bpf_dummy_struct_ops.c |  4 +++-
 7 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 132c8ffba109..bc16eb694657 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -2176,8 +2176,9 @@ void arch_free_bpf_trampoline(void *image, unsigned int size)
 	bpf_prog_pack_free(image, size);
 }
 
-void arch_protect_bpf_trampoline(void *image, unsigned int size)
+int arch_protect_bpf_trampoline(void *image, unsigned int size)
 {
+	return 0;
 }
 
 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *ro_image,
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 7a56d2d84512..4900b1ee019f 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -3004,8 +3004,9 @@ void arch_free_bpf_trampoline(void *image, unsigned int size)
 	bpf_prog_pack_free(image, size);
 }
 
-void arch_protect_bpf_trampoline(void *image, unsigned int size)
+int arch_protect_bpf_trampoline(void *image, unsigned int size)
 {
+	return 0;
 }
 
 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end,
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d89bdefb42e2..17843e66a1d3 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1116,7 +1116,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 				void *func_addr);
 void *arch_alloc_bpf_trampoline(unsigned int size);
 void arch_free_bpf_trampoline(void *image, unsigned int size);
-void arch_protect_bpf_trampoline(void *image, unsigned int size);
+int __must_check arch_protect_bpf_trampoline(void *image, unsigned int size);
 int arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags,
 			     struct bpf_tramp_links *tlinks, void *func_addr);
 
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 43356faaa057..ca1d9b87c475 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 		if (err)
 			goto reset_unlock;
 	}
-	for (i = 0; i < st_map->image_pages_cnt; i++)
-		arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
+	for (i = 0; i < st_map->image_pages_cnt && !err; i++)
+		err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
+
+	if (err)
+		goto reset_unlock;
 
 	if (st_map->map.map_flags & BPF_F_LINK) {
 		err = 0;
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 63f100def31b..1e761c3c66db 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -908,23 +908,31 @@ static LIST_HEAD(pack_list);
 static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_insns)
 {
 	struct bpf_prog_pack *pack;
+	int err;
 
 	pack = kzalloc(struct_size(pack, bitmap, BITS_TO_LONGS(BPF_PROG_CHUNK_COUNT)),
 		       GFP_KERNEL);
 	if (!pack)
 		return NULL;
 	pack->ptr = bpf_jit_alloc_exec(BPF_PROG_PACK_SIZE);
-	if (!pack->ptr) {
-		kfree(pack);
-		return NULL;
-	}
+	if (!pack->ptr)
+		goto out;
 	bpf_fill_ill_insns(pack->ptr, BPF_PROG_PACK_SIZE);
 	bitmap_zero(pack->bitmap, BPF_PROG_PACK_SIZE / BPF_PROG_CHUNK_SIZE);
-	list_add_tail(&pack->list, &pack_list);
 
 	set_vm_flush_reset_perms(pack->ptr);
-	set_memory_rox((unsigned long)pack->ptr, BPF_PROG_PACK_SIZE / PAGE_SIZE);
+	err = set_memory_rox((unsigned long)pack->ptr,
+			     BPF_PROG_PACK_SIZE / PAGE_SIZE);
+	if (err)
+		goto out_free;
+	list_add_tail(&pack->list, &pack_list);
 	return pack;
+
+out_free:
+	bpf_jit_free_exec(pack->ptr);
+out:
+	kfree(pack);
+	return NULL;
 }
 
 void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
@@ -939,9 +947,16 @@ void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
 		size = round_up(size, PAGE_SIZE);
 		ptr = bpf_jit_alloc_exec(size);
 		if (ptr) {
+			int err;
+
 			bpf_fill_ill_insns(ptr, size);
 			set_vm_flush_reset_perms(ptr);
-			set_memory_rox((unsigned long)ptr, size / PAGE_SIZE);
+			err = set_memory_rox((unsigned long)ptr,
+					     size / PAGE_SIZE);
+			if (err) {
+				bpf_jit_free_exec(ptr);
+				ptr = NULL;
+			}
 		}
 		goto out;
 	}
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 04fd1abd3661..cc50607f8d8c 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -456,7 +456,9 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 	if (err < 0)
 		goto out_free;
 
-	arch_protect_bpf_trampoline(im->image, im->size);
+	err = arch_protect_bpf_trampoline(im->image, im->size);
+	if (err)
+		goto out_free;
 
 	WARN_ON(tr->cur_image && total == 0);
 	if (tr->cur_image)
@@ -1072,10 +1074,10 @@ void __weak arch_free_bpf_trampoline(void *image, unsigned int size)
 	bpf_jit_free_exec(image);
 }
 
-void __weak arch_protect_bpf_trampoline(void *image, unsigned int size)
+int __weak arch_protect_bpf_trampoline(void *image, unsigned int size)
 {
 	WARN_ON_ONCE(size > PAGE_SIZE);
-	set_memory_rox((long)image, 1);
+	return set_memory_rox((long)image, 1);
 }
 
 int __weak arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags,
diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index de33dc1b0daa..25b75844891a 100644
--- a/net/bpf/bpf_dummy_struct_ops.c
+++ b/net/bpf/bpf_dummy_struct_ops.c
@@ -133,7 +133,9 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
 	if (err < 0)
 		goto out;
 
-	arch_protect_bpf_trampoline(image, PAGE_SIZE);
+	err = arch_protect_bpf_trampoline(image, PAGE_SIZE);
+	if (err)
+		goto out;
 	prog_ret = dummy_ops_call_op(image, args);
 
 	err = dummy_ops_copy_args(args);
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH bpf-next v3 2/2] bpf: Check return from set_memory_rox()
@ 2024-03-15 17:06   ` Christophe Leroy
  0 siblings, 0 replies; 14+ messages in thread
From: Christophe Leroy @ 2024-03-15 17:06 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,
	Zi Shen Lim, Catalin Marinas, Will Deacon, David S. Miller,
	David Ahern, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H. Peter Anvin, Kui-Feng Lee, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni
  Cc: Christophe Leroy, bpf, linux-arm-kernel, linux-kernel, netdev, Kees Cook

arch_protect_bpf_trampoline() and alloc_new_pack() call
set_memory_rox() which can fail, leading to unprotected memory.

Take into account return from set_memory_rox() function and add
__must_check flag to arch_protect_bpf_trampoline().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
v3:
- Rebased and handled conflict in kernel/bpf/bpf_struct_ops.c

v2:
- Move list_add_tail(&pack->list, &pack_list) at the end of alloc_new_pack()
- Split 2 lines that are reported longer than 80 chars by BPF patchwork's checkpatch report.
---
 arch/arm64/net/bpf_jit_comp.c  |  3 ++-
 arch/x86/net/bpf_jit_comp.c    |  3 ++-
 include/linux/bpf.h            |  2 +-
 kernel/bpf/bpf_struct_ops.c    |  7 +++++--
 kernel/bpf/core.c              | 29 ++++++++++++++++++++++-------
 kernel/bpf/trampoline.c        |  8 +++++---
 net/bpf/bpf_dummy_struct_ops.c |  4 +++-
 7 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 132c8ffba109..bc16eb694657 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -2176,8 +2176,9 @@ void arch_free_bpf_trampoline(void *image, unsigned int size)
 	bpf_prog_pack_free(image, size);
 }
 
-void arch_protect_bpf_trampoline(void *image, unsigned int size)
+int arch_protect_bpf_trampoline(void *image, unsigned int size)
 {
+	return 0;
 }
 
 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *ro_image,
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 7a56d2d84512..4900b1ee019f 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -3004,8 +3004,9 @@ void arch_free_bpf_trampoline(void *image, unsigned int size)
 	bpf_prog_pack_free(image, size);
 }
 
-void arch_protect_bpf_trampoline(void *image, unsigned int size)
+int arch_protect_bpf_trampoline(void *image, unsigned int size)
 {
+	return 0;
 }
 
 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end,
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d89bdefb42e2..17843e66a1d3 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1116,7 +1116,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 				void *func_addr);
 void *arch_alloc_bpf_trampoline(unsigned int size);
 void arch_free_bpf_trampoline(void *image, unsigned int size);
-void arch_protect_bpf_trampoline(void *image, unsigned int size);
+int __must_check arch_protect_bpf_trampoline(void *image, unsigned int size);
 int arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags,
 			     struct bpf_tramp_links *tlinks, void *func_addr);
 
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 43356faaa057..ca1d9b87c475 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 		if (err)
 			goto reset_unlock;
 	}
-	for (i = 0; i < st_map->image_pages_cnt; i++)
-		arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
+	for (i = 0; i < st_map->image_pages_cnt && !err; i++)
+		err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
+
+	if (err)
+		goto reset_unlock;
 
 	if (st_map->map.map_flags & BPF_F_LINK) {
 		err = 0;
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 63f100def31b..1e761c3c66db 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -908,23 +908,31 @@ static LIST_HEAD(pack_list);
 static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_insns)
 {
 	struct bpf_prog_pack *pack;
+	int err;
 
 	pack = kzalloc(struct_size(pack, bitmap, BITS_TO_LONGS(BPF_PROG_CHUNK_COUNT)),
 		       GFP_KERNEL);
 	if (!pack)
 		return NULL;
 	pack->ptr = bpf_jit_alloc_exec(BPF_PROG_PACK_SIZE);
-	if (!pack->ptr) {
-		kfree(pack);
-		return NULL;
-	}
+	if (!pack->ptr)
+		goto out;
 	bpf_fill_ill_insns(pack->ptr, BPF_PROG_PACK_SIZE);
 	bitmap_zero(pack->bitmap, BPF_PROG_PACK_SIZE / BPF_PROG_CHUNK_SIZE);
-	list_add_tail(&pack->list, &pack_list);
 
 	set_vm_flush_reset_perms(pack->ptr);
-	set_memory_rox((unsigned long)pack->ptr, BPF_PROG_PACK_SIZE / PAGE_SIZE);
+	err = set_memory_rox((unsigned long)pack->ptr,
+			     BPF_PROG_PACK_SIZE / PAGE_SIZE);
+	if (err)
+		goto out_free;
+	list_add_tail(&pack->list, &pack_list);
 	return pack;
+
+out_free:
+	bpf_jit_free_exec(pack->ptr);
+out:
+	kfree(pack);
+	return NULL;
 }
 
 void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
@@ -939,9 +947,16 @@ void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
 		size = round_up(size, PAGE_SIZE);
 		ptr = bpf_jit_alloc_exec(size);
 		if (ptr) {
+			int err;
+
 			bpf_fill_ill_insns(ptr, size);
 			set_vm_flush_reset_perms(ptr);
-			set_memory_rox((unsigned long)ptr, size / PAGE_SIZE);
+			err = set_memory_rox((unsigned long)ptr,
+					     size / PAGE_SIZE);
+			if (err) {
+				bpf_jit_free_exec(ptr);
+				ptr = NULL;
+			}
 		}
 		goto out;
 	}
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 04fd1abd3661..cc50607f8d8c 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -456,7 +456,9 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 	if (err < 0)
 		goto out_free;
 
-	arch_protect_bpf_trampoline(im->image, im->size);
+	err = arch_protect_bpf_trampoline(im->image, im->size);
+	if (err)
+		goto out_free;
 
 	WARN_ON(tr->cur_image && total == 0);
 	if (tr->cur_image)
@@ -1072,10 +1074,10 @@ void __weak arch_free_bpf_trampoline(void *image, unsigned int size)
 	bpf_jit_free_exec(image);
 }
 
-void __weak arch_protect_bpf_trampoline(void *image, unsigned int size)
+int __weak arch_protect_bpf_trampoline(void *image, unsigned int size)
 {
 	WARN_ON_ONCE(size > PAGE_SIZE);
-	set_memory_rox((long)image, 1);
+	return set_memory_rox((long)image, 1);
 }
 
 int __weak arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags,
diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index de33dc1b0daa..25b75844891a 100644
--- a/net/bpf/bpf_dummy_struct_ops.c
+++ b/net/bpf/bpf_dummy_struct_ops.c
@@ -133,7 +133,9 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
 	if (err < 0)
 		goto out;
 
-	arch_protect_bpf_trampoline(image, PAGE_SIZE);
+	err = arch_protect_bpf_trampoline(image, PAGE_SIZE);
+	if (err)
+		goto out;
 	prog_ret = dummy_ops_call_op(image, args);
 
 	err = dummy_ops_copy_args(args);
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next v3 2/2] bpf: Check return from set_memory_rox()
  2024-03-15 17:06   ` Christophe Leroy
@ 2024-03-15 18:11     ` Kui-Feng Lee
  -1 siblings, 0 replies; 14+ messages in thread
From: Kui-Feng Lee @ 2024-03-15 18:11 UTC (permalink / raw)
  To: Christophe Leroy, 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, Zi Shen Lim, Catalin Marinas, Will Deacon,
	David S. Miller, David Ahern, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Kui-Feng Lee,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni
  Cc: bpf, linux-arm-kernel, linux-kernel, netdev, Kees Cook



On 3/15/24 10:06, Christophe Leroy wrote:
> arch_protect_bpf_trampoline() and alloc_new_pack() call
> set_memory_rox() which can fail, leading to unprotected memory.
> 
> Take into account return from set_memory_rox() function and add
> __must_check flag to arch_protect_bpf_trampoline().
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> ---
> v3:
> - Rebased and handled conflict in kernel/bpf/bpf_struct_ops.c
> 
> v2:
> - Move list_add_tail(&pack->list, &pack_list) at the end of alloc_new_pack()
> - Split 2 lines that are reported longer than 80 chars by BPF patchwork's checkpatch report.
> ---
>   arch/arm64/net/bpf_jit_comp.c  |  3 ++-
>   arch/x86/net/bpf_jit_comp.c    |  3 ++-
>   include/linux/bpf.h            |  2 +-
>   kernel/bpf/bpf_struct_ops.c    |  7 +++++--
>   kernel/bpf/core.c              | 29 ++++++++++++++++++++++-------
>   kernel/bpf/trampoline.c        |  8 +++++---
>   net/bpf/bpf_dummy_struct_ops.c |  4 +++-
>   7 files changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 132c8ffba109..bc16eb694657 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -2176,8 +2176,9 @@ void arch_free_bpf_trampoline(void *image, unsigned int size)
>   	bpf_prog_pack_free(image, size);
>   }
>   
> -void arch_protect_bpf_trampoline(void *image, unsigned int size)
> +int arch_protect_bpf_trampoline(void *image, unsigned int size)
>   {
> +	return 0;
>   }
>   
>   int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *ro_image,
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 7a56d2d84512..4900b1ee019f 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -3004,8 +3004,9 @@ void arch_free_bpf_trampoline(void *image, unsigned int size)
>   	bpf_prog_pack_free(image, size);
>   }
>   
> -void arch_protect_bpf_trampoline(void *image, unsigned int size)
> +int arch_protect_bpf_trampoline(void *image, unsigned int size)
>   {
> +	return 0;
>   }
>   
>   int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end,
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index d89bdefb42e2..17843e66a1d3 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1116,7 +1116,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>   				void *func_addr);
>   void *arch_alloc_bpf_trampoline(unsigned int size);
>   void arch_free_bpf_trampoline(void *image, unsigned int size);
> -void arch_protect_bpf_trampoline(void *image, unsigned int size);
> +int __must_check arch_protect_bpf_trampoline(void *image, unsigned int size);
>   int arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags,
>   			     struct bpf_tramp_links *tlinks, void *func_addr);
>   
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 43356faaa057..ca1d9b87c475 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
>   		if (err)
>   			goto reset_unlock;
>   	}
> -	for (i = 0; i < st_map->image_pages_cnt; i++)
> -		arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
> +	for (i = 0; i < st_map->image_pages_cnt && !err; i++)
> +		err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
> +
> +	if (err)

nit: Can it be more specific? I mean to check err < 0, so we can reason
that this function never returns a positive value other than 0.

> +		goto reset_unlock;
>   
>   	if (st_map->map.map_flags & BPF_F_LINK) {
>   		err = 0;
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 63f100def31b..1e761c3c66db 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -908,23 +908,31 @@ static LIST_HEAD(pack_list);
>   static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_insns)
>   {
>   	struct bpf_prog_pack *pack;
> +	int err;
>   
>   	pack = kzalloc(struct_size(pack, bitmap, BITS_TO_LONGS(BPF_PROG_CHUNK_COUNT)),
>   		       GFP_KERNEL);
>   	if (!pack)
>   		return NULL;
>   	pack->ptr = bpf_jit_alloc_exec(BPF_PROG_PACK_SIZE);
> -	if (!pack->ptr) {
> -		kfree(pack);
> -		return NULL;
> -	}
> +	if (!pack->ptr)
> +		goto out;
>   	bpf_fill_ill_insns(pack->ptr, BPF_PROG_PACK_SIZE);
>   	bitmap_zero(pack->bitmap, BPF_PROG_PACK_SIZE / BPF_PROG_CHUNK_SIZE);
> -	list_add_tail(&pack->list, &pack_list);
>   
>   	set_vm_flush_reset_perms(pack->ptr);
> -	set_memory_rox((unsigned long)pack->ptr, BPF_PROG_PACK_SIZE / PAGE_SIZE);
> +	err = set_memory_rox((unsigned long)pack->ptr,
> +			     BPF_PROG_PACK_SIZE / PAGE_SIZE);
> +	if (err)
> +		goto out_free;
> +	list_add_tail(&pack->list, &pack_list);
>   	return pack;
> +
> +out_free:
> +	bpf_jit_free_exec(pack->ptr);
> +out:
> +	kfree(pack);
> +	return NULL;
>   }
>   
>   void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
> @@ -939,9 +947,16 @@ void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
>   		size = round_up(size, PAGE_SIZE);
>   		ptr = bpf_jit_alloc_exec(size);
>   		if (ptr) {
> +			int err;
> +
>   			bpf_fill_ill_insns(ptr, size);
>   			set_vm_flush_reset_perms(ptr);
> -			set_memory_rox((unsigned long)ptr, size / PAGE_SIZE);
> +			err = set_memory_rox((unsigned long)ptr,
> +					     size / PAGE_SIZE);
> +			if (err) {
> +				bpf_jit_free_exec(ptr);
> +				ptr = NULL;
> +			}
>   		}
>   		goto out;
>   	}
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 04fd1abd3661..cc50607f8d8c 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -456,7 +456,9 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
>   	if (err < 0)
>   		goto out_free;

Same here

>   
> -	arch_protect_bpf_trampoline(im->image, im->size);
> +	err = arch_protect_bpf_trampoline(im->image, im->size);
> +	if (err)
> +		goto out_free;
>   
>   	WARN_ON(tr->cur_image && total == 0);
>   	if (tr->cur_image)
> @@ -1072,10 +1074,10 @@ void __weak arch_free_bpf_trampoline(void *image, unsigned int size)
>   	bpf_jit_free_exec(image);
>   }
>   
> -void __weak arch_protect_bpf_trampoline(void *image, unsigned int size)
> +int __weak arch_protect_bpf_trampoline(void *image, unsigned int size)
>   {
>   	WARN_ON_ONCE(size > PAGE_SIZE);
> -	set_memory_rox((long)image, 1);
> +	return set_memory_rox((long)image, 1);
>   }
>   
>   int __weak arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags,
> diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
> index de33dc1b0daa..25b75844891a 100644
> --- a/net/bpf/bpf_dummy_struct_ops.c
> +++ b/net/bpf/bpf_dummy_struct_ops.c
> @@ -133,7 +133,9 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
>   	if (err < 0)
>   		goto out;
>   
> -	arch_protect_bpf_trampoline(image, PAGE_SIZE);
> +	err = arch_protect_bpf_trampoline(image, PAGE_SIZE);
> +	if (err)

Same here.

> +		goto out;
>   	prog_ret = dummy_ops_call_op(image, args);
>   
>   	err = dummy_ops_copy_args(args);

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next v3 2/2] bpf: Check return from set_memory_rox()
@ 2024-03-15 18:11     ` Kui-Feng Lee
  0 siblings, 0 replies; 14+ messages in thread
From: Kui-Feng Lee @ 2024-03-15 18:11 UTC (permalink / raw)
  To: Christophe Leroy, 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, Zi Shen Lim, Catalin Marinas, Will Deacon,
	David S. Miller, David Ahern, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Kui-Feng Lee,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni
  Cc: bpf, linux-arm-kernel, linux-kernel, netdev, Kees Cook



On 3/15/24 10:06, Christophe Leroy wrote:
> arch_protect_bpf_trampoline() and alloc_new_pack() call
> set_memory_rox() which can fail, leading to unprotected memory.
> 
> Take into account return from set_memory_rox() function and add
> __must_check flag to arch_protect_bpf_trampoline().
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> ---
> v3:
> - Rebased and handled conflict in kernel/bpf/bpf_struct_ops.c
> 
> v2:
> - Move list_add_tail(&pack->list, &pack_list) at the end of alloc_new_pack()
> - Split 2 lines that are reported longer than 80 chars by BPF patchwork's checkpatch report.
> ---
>   arch/arm64/net/bpf_jit_comp.c  |  3 ++-
>   arch/x86/net/bpf_jit_comp.c    |  3 ++-
>   include/linux/bpf.h            |  2 +-
>   kernel/bpf/bpf_struct_ops.c    |  7 +++++--
>   kernel/bpf/core.c              | 29 ++++++++++++++++++++++-------
>   kernel/bpf/trampoline.c        |  8 +++++---
>   net/bpf/bpf_dummy_struct_ops.c |  4 +++-
>   7 files changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 132c8ffba109..bc16eb694657 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -2176,8 +2176,9 @@ void arch_free_bpf_trampoline(void *image, unsigned int size)
>   	bpf_prog_pack_free(image, size);
>   }
>   
> -void arch_protect_bpf_trampoline(void *image, unsigned int size)
> +int arch_protect_bpf_trampoline(void *image, unsigned int size)
>   {
> +	return 0;
>   }
>   
>   int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *ro_image,
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 7a56d2d84512..4900b1ee019f 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -3004,8 +3004,9 @@ void arch_free_bpf_trampoline(void *image, unsigned int size)
>   	bpf_prog_pack_free(image, size);
>   }
>   
> -void arch_protect_bpf_trampoline(void *image, unsigned int size)
> +int arch_protect_bpf_trampoline(void *image, unsigned int size)
>   {
> +	return 0;
>   }
>   
>   int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end,
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index d89bdefb42e2..17843e66a1d3 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1116,7 +1116,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>   				void *func_addr);
>   void *arch_alloc_bpf_trampoline(unsigned int size);
>   void arch_free_bpf_trampoline(void *image, unsigned int size);
> -void arch_protect_bpf_trampoline(void *image, unsigned int size);
> +int __must_check arch_protect_bpf_trampoline(void *image, unsigned int size);
>   int arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags,
>   			     struct bpf_tramp_links *tlinks, void *func_addr);
>   
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 43356faaa057..ca1d9b87c475 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
>   		if (err)
>   			goto reset_unlock;
>   	}
> -	for (i = 0; i < st_map->image_pages_cnt; i++)
> -		arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
> +	for (i = 0; i < st_map->image_pages_cnt && !err; i++)
> +		err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
> +
> +	if (err)

nit: Can it be more specific? I mean to check err < 0, so we can reason
that this function never returns a positive value other than 0.

> +		goto reset_unlock;
>   
>   	if (st_map->map.map_flags & BPF_F_LINK) {
>   		err = 0;
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 63f100def31b..1e761c3c66db 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -908,23 +908,31 @@ static LIST_HEAD(pack_list);
>   static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_insns)
>   {
>   	struct bpf_prog_pack *pack;
> +	int err;
>   
>   	pack = kzalloc(struct_size(pack, bitmap, BITS_TO_LONGS(BPF_PROG_CHUNK_COUNT)),
>   		       GFP_KERNEL);
>   	if (!pack)
>   		return NULL;
>   	pack->ptr = bpf_jit_alloc_exec(BPF_PROG_PACK_SIZE);
> -	if (!pack->ptr) {
> -		kfree(pack);
> -		return NULL;
> -	}
> +	if (!pack->ptr)
> +		goto out;
>   	bpf_fill_ill_insns(pack->ptr, BPF_PROG_PACK_SIZE);
>   	bitmap_zero(pack->bitmap, BPF_PROG_PACK_SIZE / BPF_PROG_CHUNK_SIZE);
> -	list_add_tail(&pack->list, &pack_list);
>   
>   	set_vm_flush_reset_perms(pack->ptr);
> -	set_memory_rox((unsigned long)pack->ptr, BPF_PROG_PACK_SIZE / PAGE_SIZE);
> +	err = set_memory_rox((unsigned long)pack->ptr,
> +			     BPF_PROG_PACK_SIZE / PAGE_SIZE);
> +	if (err)
> +		goto out_free;
> +	list_add_tail(&pack->list, &pack_list);
>   	return pack;
> +
> +out_free:
> +	bpf_jit_free_exec(pack->ptr);
> +out:
> +	kfree(pack);
> +	return NULL;
>   }
>   
>   void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
> @@ -939,9 +947,16 @@ void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
>   		size = round_up(size, PAGE_SIZE);
>   		ptr = bpf_jit_alloc_exec(size);
>   		if (ptr) {
> +			int err;
> +
>   			bpf_fill_ill_insns(ptr, size);
>   			set_vm_flush_reset_perms(ptr);
> -			set_memory_rox((unsigned long)ptr, size / PAGE_SIZE);
> +			err = set_memory_rox((unsigned long)ptr,
> +					     size / PAGE_SIZE);
> +			if (err) {
> +				bpf_jit_free_exec(ptr);
> +				ptr = NULL;
> +			}
>   		}
>   		goto out;
>   	}
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 04fd1abd3661..cc50607f8d8c 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -456,7 +456,9 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
>   	if (err < 0)
>   		goto out_free;

Same here

>   
> -	arch_protect_bpf_trampoline(im->image, im->size);
> +	err = arch_protect_bpf_trampoline(im->image, im->size);
> +	if (err)
> +		goto out_free;
>   
>   	WARN_ON(tr->cur_image && total == 0);
>   	if (tr->cur_image)
> @@ -1072,10 +1074,10 @@ void __weak arch_free_bpf_trampoline(void *image, unsigned int size)
>   	bpf_jit_free_exec(image);
>   }
>   
> -void __weak arch_protect_bpf_trampoline(void *image, unsigned int size)
> +int __weak arch_protect_bpf_trampoline(void *image, unsigned int size)
>   {
>   	WARN_ON_ONCE(size > PAGE_SIZE);
> -	set_memory_rox((long)image, 1);
> +	return set_memory_rox((long)image, 1);
>   }
>   
>   int __weak arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags,
> diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
> index de33dc1b0daa..25b75844891a 100644
> --- a/net/bpf/bpf_dummy_struct_ops.c
> +++ b/net/bpf/bpf_dummy_struct_ops.c
> @@ -133,7 +133,9 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
>   	if (err < 0)
>   		goto out;
>   
> -	arch_protect_bpf_trampoline(image, PAGE_SIZE);
> +	err = arch_protect_bpf_trampoline(image, PAGE_SIZE);
> +	if (err)

Same here.

> +		goto out;
>   	prog_ret = dummy_ops_call_op(image, args);
>   
>   	err = dummy_ops_copy_args(args);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next v3 2/2] bpf: Check return from set_memory_rox()
  2024-03-15 18:11     ` Kui-Feng Lee
@ 2024-03-15 18:34       ` Martin KaFai Lau
  -1 siblings, 0 replies; 14+ messages in thread
From: Martin KaFai Lau @ 2024-03-15 18:34 UTC (permalink / raw)
  To: Kui-Feng Lee
  Cc: bpf, linux-arm-kernel, linux-kernel, netdev, Kees Cook,
	Christophe Leroy, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Eduard Zingerman, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Zi Shen Lim, Catalin Marinas, Will Deacon, David S. Miller,
	David Ahern, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H. Peter Anvin, Kui-Feng Lee, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni

On 3/15/24 11:11 AM, Kui-Feng Lee wrote:
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map 
>> *map, void *key,
>>           if (err)
>>               goto reset_unlock;
>>       }
>> -    for (i = 0; i < st_map->image_pages_cnt; i++)
>> -        arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
>> +    for (i = 0; i < st_map->image_pages_cnt && !err; i++)
>> +        err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
>> +
>> +    if (err)
> 
> nit: Can it be more specific? I mean to check err < 0, so we can reason
> that this function never returns a positive value other than 0.

I think "if (err)" is fine. It is pretty common in other places of the kernel.

Checking "(err < 0)" may actually mean the return value could be positive. At 
least it is how bpf_struct_ops.c is using "(err < 0)".

[ An unrelated side note is another (err < 0) check in bpf_struct_ops.c could 
have been changed after the recent changes in bpf_struct_ops_prepare_trampoline 
which no longer return +val ].



^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next v3 2/2] bpf: Check return from set_memory_rox()
@ 2024-03-15 18:34       ` Martin KaFai Lau
  0 siblings, 0 replies; 14+ messages in thread
From: Martin KaFai Lau @ 2024-03-15 18:34 UTC (permalink / raw)
  To: Kui-Feng Lee
  Cc: bpf, linux-arm-kernel, linux-kernel, netdev, Kees Cook,
	Christophe Leroy, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Eduard Zingerman, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Zi Shen Lim, Catalin Marinas, Will Deacon, David S. Miller,
	David Ahern, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H. Peter Anvin, Kui-Feng Lee, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni

On 3/15/24 11:11 AM, Kui-Feng Lee wrote:
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map 
>> *map, void *key,
>>           if (err)
>>               goto reset_unlock;
>>       }
>> -    for (i = 0; i < st_map->image_pages_cnt; i++)
>> -        arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
>> +    for (i = 0; i < st_map->image_pages_cnt && !err; i++)
>> +        err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
>> +
>> +    if (err)
> 
> nit: Can it be more specific? I mean to check err < 0, so we can reason
> that this function never returns a positive value other than 0.

I think "if (err)" is fine. It is pretty common in other places of the kernel.

Checking "(err < 0)" may actually mean the return value could be positive. At 
least it is how bpf_struct_ops.c is using "(err < 0)".

[ An unrelated side note is another (err < 0) check in bpf_struct_ops.c could 
have been changed after the recent changes in bpf_struct_ops_prepare_trampoline 
which no longer return +val ].



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next v3 2/2] bpf: Check return from set_memory_rox()
  2024-03-15 17:06   ` Christophe Leroy
@ 2024-03-15 20:55     ` Martin KaFai Lau
  -1 siblings, 0 replies; 14+ messages in thread
From: Martin KaFai Lau @ 2024-03-15 20:55 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Zi Shen Lim,
	Catalin Marinas, Will Deacon, David S. Miller, David Ahern,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H. Peter Anvin, Kui-Feng Lee, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, bpf, linux-arm-kernel, linux-kernel, netdev,
	Kees Cook

On 3/15/24 10:06 AM, Christophe Leroy wrote:
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 43356faaa057..ca1d9b87c475 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
>   		if (err)
>   			goto reset_unlock;
>   	}
> -	for (i = 0; i < st_map->image_pages_cnt; i++)
> -		arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
> +	for (i = 0; i < st_map->image_pages_cnt && !err; i++)
> +		err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
> +
> +	if (err)
> +		goto reset_unlock;

This part does not look right. The "if (err)" check should be inside the for loop.

pw-bot: cr

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next v3 2/2] bpf: Check return from set_memory_rox()
@ 2024-03-15 20:55     ` Martin KaFai Lau
  0 siblings, 0 replies; 14+ messages in thread
From: Martin KaFai Lau @ 2024-03-15 20:55 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Zi Shen Lim,
	Catalin Marinas, Will Deacon, David S. Miller, David Ahern,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H. Peter Anvin, Kui-Feng Lee, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, bpf, linux-arm-kernel, linux-kernel, netdev,
	Kees Cook

On 3/15/24 10:06 AM, Christophe Leroy wrote:
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 43356faaa057..ca1d9b87c475 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
>   		if (err)
>   			goto reset_unlock;
>   	}
> -	for (i = 0; i < st_map->image_pages_cnt; i++)
> -		arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
> +	for (i = 0; i < st_map->image_pages_cnt && !err; i++)
> +		err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
> +
> +	if (err)
> +		goto reset_unlock;

This part does not look right. The "if (err)" check should be inside the for loop.

pw-bot: cr

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next v3 2/2] bpf: Check return from set_memory_rox()
  2024-03-15 20:55     ` Martin KaFai Lau
@ 2024-03-15 21:11       ` Martin KaFai Lau
  -1 siblings, 0 replies; 14+ messages in thread
From: Martin KaFai Lau @ 2024-03-15 21:11 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Zi Shen Lim,
	Catalin Marinas, Will Deacon, David S. Miller, David Ahern,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H. Peter Anvin, Kui-Feng Lee, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, bpf, linux-arm-kernel, linux-kernel, netdev,
	Kees Cook

On 3/15/24 1:55 PM, Martin KaFai Lau wrote:
> On 3/15/24 10:06 AM, Christophe Leroy wrote:
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 43356faaa057..ca1d9b87c475 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map 
>> *map, void *key,
>>           if (err)
>>               goto reset_unlock;
>>       }
>> -    for (i = 0; i < st_map->image_pages_cnt; i++)
>> -        arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
>> +    for (i = 0; i < st_map->image_pages_cnt && !err; i++)
>> +        err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
>> +
>> +    if (err)
>> +        goto reset_unlock;
> 
> This part does not look right. The "if (err)" check should be inside the for loop.

ah. Please ignore. missed the "!err" in the for loop.

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next v3 2/2] bpf: Check return from set_memory_rox()
@ 2024-03-15 21:11       ` Martin KaFai Lau
  0 siblings, 0 replies; 14+ messages in thread
From: Martin KaFai Lau @ 2024-03-15 21:11 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Zi Shen Lim,
	Catalin Marinas, Will Deacon, David S. Miller, David Ahern,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H. Peter Anvin, Kui-Feng Lee, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, bpf, linux-arm-kernel, linux-kernel, netdev,
	Kees Cook

On 3/15/24 1:55 PM, Martin KaFai Lau wrote:
> On 3/15/24 10:06 AM, Christophe Leroy wrote:
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 43356faaa057..ca1d9b87c475 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map 
>> *map, void *key,
>>           if (err)
>>               goto reset_unlock;
>>       }
>> -    for (i = 0; i < st_map->image_pages_cnt; i++)
>> -        arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
>> +    for (i = 0; i < st_map->image_pages_cnt && !err; i++)
>> +        err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
>> +
>> +    if (err)
>> +        goto reset_unlock;
> 
> This part does not look right. The "if (err)" check should be inside the for loop.

ah. Please ignore. missed the "!err" in the for loop.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next v3 2/2] bpf: Check return from set_memory_rox()
  2024-03-15 21:11       ` Martin KaFai Lau
@ 2024-03-16  0:56         ` Martin KaFai Lau
  -1 siblings, 0 replies; 14+ messages in thread
From: Martin KaFai Lau @ 2024-03-16  0:56 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Zi Shen Lim,
	Catalin Marinas, Will Deacon, David S. Miller, David Ahern,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H. Peter Anvin, Kui-Feng Lee, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, bpf, linux-arm-kernel, linux-kernel, netdev,
	Kees Cook

On 3/15/24 2:11 PM, Martin KaFai Lau wrote:
> On 3/15/24 1:55 PM, Martin KaFai Lau wrote:
>> On 3/15/24 10:06 AM, Christophe Leroy wrote:
>>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>>> index 43356faaa057..ca1d9b87c475 100644
>>> --- a/kernel/bpf/bpf_struct_ops.c
>>> +++ b/kernel/bpf/bpf_struct_ops.c
>>> @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct 
>>> bpf_map *map, void *key,
>>>           if (err)
>>>               goto reset_unlock;
>>>       }
>>> -    for (i = 0; i < st_map->image_pages_cnt; i++)
>>> -        arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
>>> +    for (i = 0; i < st_map->image_pages_cnt && !err; i++)

I was about to apply but I still think checking "&& !err" is not right given how 
"err" is used in the earlier code of this function.

The err may not be 0 in the first iteration of this for loop. Take a look at the 
"if (err > 0)" check in the "for_each_member(i, t, member)" loop above.

>>> +        err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
>>> +
>>> +    if (err)
>>> +        goto reset_unlock;
>>
>> This part does not look right. The "if (err)" check should be inside the for 
>> loop.

Instead of adding an extra "err = 0;" before the for loop. It is better to move 
this "if (err) goto reset_unlock;" into the for loop and remove the "&& !err" 
test above.

> 
> ah. Please ignore. missed the "!err" in the for loop.




^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next v3 2/2] bpf: Check return from set_memory_rox()
@ 2024-03-16  0:56         ` Martin KaFai Lau
  0 siblings, 0 replies; 14+ messages in thread
From: Martin KaFai Lau @ 2024-03-16  0:56 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Zi Shen Lim,
	Catalin Marinas, Will Deacon, David S. Miller, David Ahern,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H. Peter Anvin, Kui-Feng Lee, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, bpf, linux-arm-kernel, linux-kernel, netdev,
	Kees Cook

On 3/15/24 2:11 PM, Martin KaFai Lau wrote:
> On 3/15/24 1:55 PM, Martin KaFai Lau wrote:
>> On 3/15/24 10:06 AM, Christophe Leroy wrote:
>>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>>> index 43356faaa057..ca1d9b87c475 100644
>>> --- a/kernel/bpf/bpf_struct_ops.c
>>> +++ b/kernel/bpf/bpf_struct_ops.c
>>> @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct 
>>> bpf_map *map, void *key,
>>>           if (err)
>>>               goto reset_unlock;
>>>       }
>>> -    for (i = 0; i < st_map->image_pages_cnt; i++)
>>> -        arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
>>> +    for (i = 0; i < st_map->image_pages_cnt && !err; i++)

I was about to apply but I still think checking "&& !err" is not right given how 
"err" is used in the earlier code of this function.

The err may not be 0 in the first iteration of this for loop. Take a look at the 
"if (err > 0)" check in the "for_each_member(i, t, member)" loop above.

>>> +        err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
>>> +
>>> +    if (err)
>>> +        goto reset_unlock;
>>
>> This part does not look right. The "if (err)" check should be inside the for 
>> loop.

Instead of adding an extra "err = 0;" before the for loop. It is better to move 
this "if (err) goto reset_unlock;" into the for loop and remove the "&& !err" 
test above.

> 
> ah. Please ignore. missed the "!err" in the for loop.




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-03-16  0:57 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-15 17:06 [PATCH bpf-next v3 1/2] bpf: Remove arch_unprotect_bpf_trampoline() Christophe Leroy
2024-03-15 17:06 ` Christophe Leroy
2024-03-15 17:06 ` [PATCH bpf-next v3 2/2] bpf: Check return from set_memory_rox() Christophe Leroy
2024-03-15 17:06   ` Christophe Leroy
2024-03-15 18:11   ` Kui-Feng Lee
2024-03-15 18:11     ` Kui-Feng Lee
2024-03-15 18:34     ` Martin KaFai Lau
2024-03-15 18:34       ` Martin KaFai Lau
2024-03-15 20:55   ` Martin KaFai Lau
2024-03-15 20:55     ` Martin KaFai Lau
2024-03-15 21:11     ` Martin KaFai Lau
2024-03-15 21:11       ` Martin KaFai Lau
2024-03-16  0:56       ` Martin KaFai Lau
2024-03-16  0:56         ` Martin KaFai Lau

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.