* [PATCH AUTOSEL 6.12 19/29] ARC: bpf: Correct conditional check in 'check_jmp_32'
[not found] <20241220171130.511389-1-sashal@kernel.org>
@ 2024-12-20 17:11 ` Sasha Levin
2024-12-20 17:11 ` [PATCH AUTOSEL 6.12 20/29] bpf: fix potential error return Sasha Levin
` (2 subsequent siblings)
3 siblings, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2024-12-20 17:11 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Hardevsinh Palaniya, Vadim Fedorenko, Shahab Vahedi, Vineet Gupta,
Sasha Levin, ast, daniel, andrii, bpf, linux-snps-arc
From: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
[ Upstream commit 7dd9eb6ba88964b091b89855ce7d2a12405013af ]
The original code checks 'if (ARC_CC_AL)', which is always true since
ARC_CC_AL is a constant. This makes the check redundant and likely
obscures the intention of verifying whether the jump is conditional.
Updates the code to check cond == ARC_CC_AL instead, reflecting the intent
to differentiate conditional from unconditional jumps.
Suggested-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
Acked-by: Shahab Vahedi <list+bpf@vahedi.org>
Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
Signed-off-by: Vineet Gupta <vgupta@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
arch/arc/net/bpf_jit_arcv2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arc/net/bpf_jit_arcv2.c b/arch/arc/net/bpf_jit_arcv2.c
index 4458e409ca0a..6d989b6d88c6 100644
--- a/arch/arc/net/bpf_jit_arcv2.c
+++ b/arch/arc/net/bpf_jit_arcv2.c
@@ -2916,7 +2916,7 @@ bool check_jmp_32(u32 curr_off, u32 targ_off, u8 cond)
addendum = (cond == ARC_CC_AL) ? 0 : INSN_len_normal;
disp = get_displacement(curr_off + addendum, targ_off);
- if (ARC_CC_AL)
+ if (cond == ARC_CC_AL)
return is_valid_far_disp(disp);
else
return is_valid_near_disp(disp);
--
2.39.5
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH AUTOSEL 6.12 20/29] bpf: fix potential error return
[not found] <20241220171130.511389-1-sashal@kernel.org>
2024-12-20 17:11 ` [PATCH AUTOSEL 6.12 19/29] ARC: bpf: Correct conditional check in 'check_jmp_32' Sasha Levin
@ 2024-12-20 17:11 ` Sasha Levin
2024-12-20 17:11 ` [PATCH AUTOSEL 6.12 28/29] bpf: refactor bpf_helper_changes_pkt_data to use helper number Sasha Levin
2024-12-20 17:11 ` [PATCH AUTOSEL 6.12 29/29] bpf: consider that tail calls invalidate packet pointers Sasha Levin
3 siblings, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2024-12-20 17:11 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Anton Protopopov, Jiri Olsa, Andrii Nakryiko, Alexei Starovoitov,
Sasha Levin, daniel, bpf
From: Anton Protopopov <aspsk@isovalent.com>
[ Upstream commit c4441ca86afe4814039ee1b32c39d833c1a16bbc ]
The bpf_remove_insns() function returns WARN_ON_ONCE(error), where
error is a result of bpf_adj_branches(), and thus should be always 0
However, if for any reason it is not 0, then it will be converted to
boolean by WARN_ON_ONCE and returned to user space as 1, not an actual
error value. Fix this by returning the original err after the WARN check.
Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Link: https://lore.kernel.org/r/20241210114245.836164-1-aspsk@isovalent.com
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
kernel/bpf/core.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 233ea78f8f1b..3af5f42ea791 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -539,6 +539,8 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
{
+ int err;
+
/* Branch offsets can't overflow when program is shrinking, no need
* to call bpf_adj_branches(..., true) here
*/
@@ -546,7 +548,9 @@ int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
sizeof(struct bpf_insn) * (prog->len - off - cnt));
prog->len -= cnt;
- return WARN_ON_ONCE(bpf_adj_branches(prog, off, off + cnt, off, false));
+ err = bpf_adj_branches(prog, off, off + cnt, off, false);
+ WARN_ON_ONCE(err);
+ return err;
}
static void bpf_prog_kallsyms_del_subprogs(struct bpf_prog *fp)
--
2.39.5
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH AUTOSEL 6.12 28/29] bpf: refactor bpf_helper_changes_pkt_data to use helper number
[not found] <20241220171130.511389-1-sashal@kernel.org>
2024-12-20 17:11 ` [PATCH AUTOSEL 6.12 19/29] ARC: bpf: Correct conditional check in 'check_jmp_32' Sasha Levin
2024-12-20 17:11 ` [PATCH AUTOSEL 6.12 20/29] bpf: fix potential error return Sasha Levin
@ 2024-12-20 17:11 ` Sasha Levin
2024-12-20 17:11 ` [PATCH AUTOSEL 6.12 29/29] bpf: consider that tail calls invalidate packet pointers Sasha Levin
3 siblings, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2024-12-20 17:11 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Eduard Zingerman, Alexei Starovoitov, Sasha Levin, daniel, andrii,
martin.lau, davem, edumazet, kuba, pabeni, hawk, john.fastabend,
bpf, netdev
From: Eduard Zingerman <eddyz87@gmail.com>
[ Upstream commit b238e187b4a2d3b54d80aec05a9cab6466b79dde ]
Use BPF helper number instead of function pointer in
bpf_helper_changes_pkt_data(). This would simplify usage of this
function in verifier.c:check_cfg() (in a follow-up patch),
where only helper number is easily available and there is no real need
to lookup helper proto.
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
Link: https://lore.kernel.org/r/20241210041100.1898468-3-eddyz87@gmail.com
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Stable-dep-of: 1a4607ffba35 ("bpf: consider that tail calls invalidate packet pointers")
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
include/linux/filter.h | 2 +-
kernel/bpf/core.c | 2 +-
kernel/bpf/verifier.c | 2 +-
net/core/filter.c | 63 +++++++++++++++++++-----------------------
4 files changed, 31 insertions(+), 38 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 7d7578a8eac1..5118caf8aa1c 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1121,7 +1121,7 @@ bool bpf_jit_supports_arena(void);
bool bpf_jit_supports_insn(struct bpf_insn *insn, bool in_arena);
u64 bpf_arch_uaddress_limit(void);
void arch_bpf_stack_walk(bool (*consume_fn)(void *cookie, u64 ip, u64 sp, u64 bp), void *cookie);
-bool bpf_helper_changes_pkt_data(void *func);
+bool bpf_helper_changes_pkt_data(enum bpf_func_id func_id);
static inline bool bpf_dump_raw_ok(const struct cred *cred)
{
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 3af5f42ea791..2b9c8c168a0b 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2940,7 +2940,7 @@ void __weak bpf_jit_compile(struct bpf_prog *prog)
{
}
-bool __weak bpf_helper_changes_pkt_data(void *func)
+bool __weak bpf_helper_changes_pkt_data(enum bpf_func_id func_id)
{
return false;
}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index b2008076df9c..71575f83860b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10519,7 +10519,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
}
/* With LD_ABS/IND some JITs save/restore skb from r1. */
- changes_data = bpf_helper_changes_pkt_data(fn->func);
+ changes_data = bpf_helper_changes_pkt_data(func_id);
if (changes_data && fn->arg1_type != ARG_PTR_TO_CTX) {
verbose(env, "kernel subsystem misconfigured func %s#%d: r1 != ctx\n",
func_id_name(func_id), func_id);
diff --git a/net/core/filter.c b/net/core/filter.c
index 9a459213d283..33125317994e 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7909,42 +7909,35 @@ static const struct bpf_func_proto bpf_tcp_raw_check_syncookie_ipv6_proto = {
#endif /* CONFIG_INET */
-bool bpf_helper_changes_pkt_data(void *func)
-{
- if (func == bpf_skb_vlan_push ||
- func == bpf_skb_vlan_pop ||
- func == bpf_skb_store_bytes ||
- func == bpf_skb_change_proto ||
- func == bpf_skb_change_head ||
- func == sk_skb_change_head ||
- func == bpf_skb_change_tail ||
- func == sk_skb_change_tail ||
- func == bpf_skb_adjust_room ||
- func == sk_skb_adjust_room ||
- func == bpf_skb_pull_data ||
- func == sk_skb_pull_data ||
- func == bpf_clone_redirect ||
- func == bpf_l3_csum_replace ||
- func == bpf_l4_csum_replace ||
- func == bpf_xdp_adjust_head ||
- func == bpf_xdp_adjust_meta ||
- func == bpf_msg_pull_data ||
- func == bpf_msg_push_data ||
- func == bpf_msg_pop_data ||
- func == bpf_xdp_adjust_tail ||
-#if IS_ENABLED(CONFIG_IPV6_SEG6_BPF)
- func == bpf_lwt_seg6_store_bytes ||
- func == bpf_lwt_seg6_adjust_srh ||
- func == bpf_lwt_seg6_action ||
-#endif
-#ifdef CONFIG_INET
- func == bpf_sock_ops_store_hdr_opt ||
-#endif
- func == bpf_lwt_in_push_encap ||
- func == bpf_lwt_xmit_push_encap)
+bool bpf_helper_changes_pkt_data(enum bpf_func_id func_id)
+{
+ switch (func_id) {
+ case BPF_FUNC_clone_redirect:
+ case BPF_FUNC_l3_csum_replace:
+ case BPF_FUNC_l4_csum_replace:
+ case BPF_FUNC_lwt_push_encap:
+ case BPF_FUNC_lwt_seg6_action:
+ case BPF_FUNC_lwt_seg6_adjust_srh:
+ case BPF_FUNC_lwt_seg6_store_bytes:
+ case BPF_FUNC_msg_pop_data:
+ case BPF_FUNC_msg_pull_data:
+ case BPF_FUNC_msg_push_data:
+ case BPF_FUNC_skb_adjust_room:
+ case BPF_FUNC_skb_change_head:
+ case BPF_FUNC_skb_change_proto:
+ case BPF_FUNC_skb_change_tail:
+ case BPF_FUNC_skb_pull_data:
+ case BPF_FUNC_skb_store_bytes:
+ case BPF_FUNC_skb_vlan_pop:
+ case BPF_FUNC_skb_vlan_push:
+ case BPF_FUNC_store_hdr_opt:
+ case BPF_FUNC_xdp_adjust_head:
+ case BPF_FUNC_xdp_adjust_meta:
+ case BPF_FUNC_xdp_adjust_tail:
return true;
-
- return false;
+ default:
+ return false;
+ }
}
const struct bpf_func_proto bpf_event_output_data_proto __weak;
--
2.39.5
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH AUTOSEL 6.12 29/29] bpf: consider that tail calls invalidate packet pointers
[not found] <20241220171130.511389-1-sashal@kernel.org>
` (2 preceding siblings ...)
2024-12-20 17:11 ` [PATCH AUTOSEL 6.12 28/29] bpf: refactor bpf_helper_changes_pkt_data to use helper number Sasha Levin
@ 2024-12-20 17:11 ` Sasha Levin
3 siblings, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2024-12-20 17:11 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Eduard Zingerman, Alexei Starovoitov, Sasha Levin, daniel, andrii,
martin.lau, davem, edumazet, kuba, pabeni, shuah, leon.hwang,
yonghong.song, bpf, netdev, linux-kselftest
From: Eduard Zingerman <eddyz87@gmail.com>
[ Upstream commit 1a4607ffba35bf2a630aab299e34dd3f6e658d70 ]
Tail-called programs could execute any of the helpers that invalidate
packet pointers. Hence, conservatively assume that each tail call
invalidates packet pointers.
Making the change in bpf_helper_changes_pkt_data() automatically makes
use of check_cfg() logic that computes 'changes_pkt_data' effect for
global sub-programs, such that the following program could be
rejected:
int tail_call(struct __sk_buff *sk)
{
bpf_tail_call_static(sk, &jmp_table, 0);
return 0;
}
SEC("tc")
int not_safe(struct __sk_buff *sk)
{
int *p = (void *)(long)sk->data;
... make p valid ...
tail_call(sk);
*p = 42; /* this is unsafe */
...
}
The tc_bpf2bpf.c:subprog_tc() needs change: mark it as a function that
can invalidate packet pointers. Otherwise, it can't be freplaced with
tailcall_freplace.c:entry_freplace() that does a tail call.
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
Link: https://lore.kernel.org/r/20241210041100.1898468-8-eddyz87@gmail.com
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
net/core/filter.c | 2 ++
tools/testing/selftests/bpf/progs/tc_bpf2bpf.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/net/core/filter.c b/net/core/filter.c
index 33125317994e..bbd0c08072cb 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7934,6 +7934,8 @@ bool bpf_helper_changes_pkt_data(enum bpf_func_id func_id)
case BPF_FUNC_xdp_adjust_head:
case BPF_FUNC_xdp_adjust_meta:
case BPF_FUNC_xdp_adjust_tail:
+ /* tail-called program could call any of the above */
+ case BPF_FUNC_tail_call:
return true;
default:
return false;
diff --git a/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
index 8a0632c37839..79f5087dade2 100644
--- a/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
@@ -10,6 +10,8 @@ int subprog(struct __sk_buff *skb)
int ret = 1;
__sink(ret);
+ /* let verifier know that 'subprog_tc' can change pointers to skb->data */
+ bpf_skb_change_proto(skb, 0, 0);
return ret;
}
--
2.39.5
^ permalink raw reply related [flat|nested] 4+ messages in thread