Kernel/linux-stable
1
0
Fork 0
mirror of https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git synced 2024-12-29 09:16:33 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Merge branch 'bpf-track-changes_pkt_data-property-for-global-functions'

Browse Source 
Eduard Zingerman says:

====================
bpf: track changes_pkt_data property for global functions

Nick Zavaritsky reported [0] a bug in verifier, where the following
unsafe program is not rejected:

    __attribute__((__noinline__))
    long skb_pull_data(struct __sk_buff *sk, __u32 len)
    {
        return bpf_skb_pull_data(sk, len);
    }

    SEC("tc")
    int test_invalidate_checks(struct __sk_buff *sk)
    {
        int *p = (void *)(long)sk->data;
        if ((void *)(p + 1) > (void *)(long)sk->data_end) return TCX_DROP;
        skb_pull_data(sk, 0);
        /* not safe, p is invalid after bpf_skb_pull_data call */
        *p = 42;
        return TCX_PASS;
    }

This happens because verifier does not track package invalidation
effect of global sub-programs.

This patch-set fixes the issue by modifying check_cfg() to compute
whether or not each sub-program calls (directly or indirectly)
helper invalidating packet pointers.

As global functions could be replaced with extension programs,
a new field 'changes_pkt_data' is added to struct bpf_prog_aux.
Verifier only allows replacing functions that do not change packet
data with functions that do not change packet data.

In case if there is a need to a have a global function that does not
change packet data, but allow replacing it with function that does,
the recommendation is to add a noop call to a helper, e.g.:
- for skb do 'bpf_skb_change_proto(skb, 0, 0)';
- for xdp do 'bpf_xdp_adjust_meta(xdp, 0)'.

Functions also can do tail calls. Effects of the tail call cannot be
analyzed before-hand, thus verifier assumes that tail calls always
change packet data.

Changes v1 [1] -> v2:
- added handling of extension programs and tail calls
  (thanks, Alexei, for all the input).

[0] https://lore.kernel.org/bpf/0498CA22-5779-4767-9C0C-A9515CEA711F@gmail.com/
[1] https://lore.kernel.org/bpf/20241206040307.568065-1-eddyz87@gmail.com/
====================

Link: https://patch.msgid.link/20241210041100.1898468-1-eddyz87@gmail.com
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
This commit is contained in:
Alexei Starovoitov 2024-12-10 10:24:58 -08:00
commit cf8b876363
11 changed files with 279 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
include/linux/bpf.h
View File

@ -1527,6 +1527,7 @@ struct bpf_prog_aux {
bool is_extended; /* true if extended by freplace program */
bool jits_use_priv_stack;
bool priv_stack_requested;
bool changes_pkt_data;
u64 prog_array_member_cnt; /* counts how many times as member of prog_array */
struct mutex ext_mutex; /* mutex for is_extended and prog_array_member_cnt */
struct bpf_arena *arena;

1
include/linux/bpf_verifier.h
View File

@ -659,6 +659,7 @@ struct bpf_subprog_info {
bool args_cached: 1;
/* true if bpf_fastcall stack region is used by functions that can't be inlined */
bool keep_fastcall_stack: 1;
bool changes_pkt_data: 1;
enum priv_stack_mode priv_stack_mode;
u8 arg_cnt;

2
include/linux/filter.h
View File

@ -1122,7 +1122,7 @@ bool bpf_jit_supports_insn(struct bpf_insn *insn, bool in_arena);
bool bpf_jit_supports_private_stack(void);
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)
{

2
kernel/bpf/core.c
View File

@ -2936,7 +2936,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;
}

78
kernel/bpf/verifier.c
View File

@ -2597,16 +2597,36 @@ static int cmp_subprogs(const void *a, const void *b)
((struct bpf_subprog_info *)b)->start;
}
/* Find subprogram that contains instruction at 'off' */
static struct bpf_subprog_info *find_containing_subprog(struct bpf_verifier_env *env, int off)
{
struct bpf_subprog_info *vals = env->subprog_info;
int l, r, m;
if (off >= env->prog->len || off < 0 || env->subprog_cnt == 0)
return NULL;
l = 0;
r = env->subprog_cnt - 1;
while (l < r) {
m = l + (r - l + 1) / 2;
if (vals[m].start <= off)
l = m;
else
r = m - 1;
}
return &vals[l];
}
/* Find subprogram that starts exactly at 'off' */
static int find_subprog(struct bpf_verifier_env *env, int off)
{
struct bpf_subprog_info *p;
p = bsearch(&off, env->subprog_info, env->subprog_cnt,
sizeof(env->subprog_info[0]), cmp_subprogs);
if (!p)
p = find_containing_subprog(env, off);
if (!p || p->start != off)
return -ENOENT;
return p - env->subprog_info;
}
static int add_subprog(struct bpf_verifier_env *env, int off)
@ -10022,6 +10042,8 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
verbose(env, "Func#%d ('%s') is global and assumed valid.\n",
subprog, sub_name);
if (env->subprog_info[subprog].changes_pkt_data)
clear_all_pkt_pointers(env);
/* mark global subprog for verifying after main prog */
subprog_aux(env, subprog)->called = true;
clear_caller_saved_regs(env, caller->regs);
@ -10708,7 +10730,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);
@ -16226,6 +16248,29 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
return 0;
}
static void mark_subprog_changes_pkt_data(struct bpf_verifier_env *env, int off)
{
struct bpf_subprog_info *subprog;
subprog = find_containing_subprog(env, off);
subprog->changes_pkt_data = true;
}
/* 't' is an index of a call-site.
* 'w' is a callee entry point.
* Eventually this function would be called when env->cfg.insn_state[w] == EXPLORED.
* Rely on DFS traversal order and absence of recursive calls to guarantee that
* callee's change_pkt_data marks would be correct at that moment.
*/
static void merge_callee_effects(struct bpf_verifier_env *env, int t, int w)
{
struct bpf_subprog_info *caller, *callee;
caller = find_containing_subprog(env, t);
callee = find_containing_subprog(env, w);
caller->changes_pkt_data |= callee->changes_pkt_data;
}
/* non-recursive DFS pseudo code
* 1 procedure DFS-iterative(G,v):
* 2 label v as discovered
@ -16359,6 +16404,7 @@ static int visit_func_call_insn(int t, struct bpf_insn *insns,
bool visit_callee)
{
int ret, insn_sz;
int w;
insn_sz = bpf_is_ldimm64(&insns[t]) ? 2 : 1;
ret = push_insn(t, t + insn_sz, FALLTHROUGH, env);
@ -16370,8 +16416,10 @@ static int visit_func_call_insn(int t, struct bpf_insn *insns,
mark_jmp_point(env, t + insn_sz);
if (visit_callee) {
w = t + insns[t].imm + 1;
mark_prune_point(env, t);
ret = push_insn(t, t + insns[t].imm + 1, BRANCH, env);
merge_callee_effects(env, t, w);
ret = push_insn(t, w, BRANCH, env);
}
return ret;
}
@ -16688,6 +16736,8 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
mark_prune_point(env, t);
mark_jmp_point(env, t);
}
if (bpf_helper_call(insn) && bpf_helper_changes_pkt_data(insn->imm))
mark_subprog_changes_pkt_data(env, t);
if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
struct bpf_kfunc_call_arg_meta meta;
@ -16822,6 +16872,7 @@ static int check_cfg(struct bpf_verifier_env *env)
}
}
ret = 0; /* cfg looks good */
env->prog->aux->changes_pkt_data = env->subprog_info[0].changes_pkt_data;
err_free:
kvfree(insn_state);
@ -20311,6 +20362,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
func[i]->aux->num_exentries = num_exentries;
func[i]->aux->tail_call_reachable = env->subprog_info[i].tail_call_reachable;
func[i]->aux->exception_cb = env->subprog_info[i].is_exception_cb;
func[i]->aux->changes_pkt_data = env->subprog_info[i].changes_pkt_data;
if (!i)
func[i]->aux->exception_boundary = env->seen_exception;
func[i] = bpf_int_jit_compile(func[i]);
@ -22175,6 +22227,12 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
"Extension programs should be JITed\n");
return -EINVAL;
}
if (prog->aux->changes_pkt_data &&
!aux->func[subprog]->aux->changes_pkt_data) {
bpf_log(log,
"Extension program changes packet data, while original does not\n");
return -EINVAL;
}
}
if (!tgt_prog->jited) {
bpf_log(log, "Can attach to only JITed progs\n");
@ -22640,10 +22698,6 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
if (ret < 0)
goto skip_full_check;
ret = check_attach_btf_id(env);
if (ret)
goto skip_full_check;
ret = resolve_pseudo_ldimm64(env);
if (ret < 0)
goto skip_full_check;
@ -22658,6 +22712,10 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
if (ret < 0)
goto skip_full_check;
ret = check_attach_btf_id(env);
if (ret)
goto skip_full_check;
ret = mark_fastcall_patterns(env);
if (ret < 0)
goto skip_full_check;

63
net/core/filter.c
View File

@ -7899,42 +7899,37 @@ static const struct bpf_func_proto bpf_tcp_raw_check_syncookie_ipv6_proto = {
#endif /* CONFIG_INET */
bool bpf_helper_changes_pkt_data(void *func)
bool bpf_helper_changes_pkt_data(enum bpf_func_id func_id)
{
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)
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:
/* tail-called program could call any of the above */
case BPF_FUNC_tail_call:
return true;
return false;
default:
return false;
}
}
const struct bpf_func_proto bpf_event_output_data_proto __weak;

76
tools/testing/selftests/bpf/prog_tests/changes_pkt_data.c Normal file
View File

@ -0,0 +1,76 @@
// SPDX-License-Identifier: GPL-2.0
#include "bpf/libbpf.h"
#include "changes_pkt_data_freplace.skel.h"
#include "changes_pkt_data.skel.h"
#include <test_progs.h>
static void print_verifier_log(const char *log)
{
if (env.verbosity >= VERBOSE_VERY)
fprintf(stdout, "VERIFIER LOG:\n=============\n%s=============\n", log);
}
static void test_aux(const char *main_prog_name, const char *freplace_prog_name, bool expect_load)
{
struct changes_pkt_data_freplace *freplace = NULL;
struct bpf_program *freplace_prog = NULL;
LIBBPF_OPTS(bpf_object_open_opts, opts);
struct changes_pkt_data *main = NULL;
char log[16*1024];
int err;
opts.kernel_log_buf = log;
opts.kernel_log_size = sizeof(log);
if (env.verbosity >= VERBOSE_SUPER)
opts.kernel_log_level = 1 | 2 | 4;
main = changes_pkt_data__open_opts(&opts);
if (!ASSERT_OK_PTR(main, "changes_pkt_data__open"))
goto out;
err = changes_pkt_data__load(main);
print_verifier_log(log);
if (!ASSERT_OK(err, "changes_pkt_data__load"))
goto out;
freplace = changes_pkt_data_freplace__open_opts(&opts);
if (!ASSERT_OK_PTR(freplace, "changes_pkt_data_freplace__open"))
goto out;
freplace_prog = bpf_object__find_program_by_name(freplace->obj, freplace_prog_name);
if (!ASSERT_OK_PTR(freplace_prog, "freplace_prog"))
goto out;
bpf_program__set_autoload(freplace_prog, true);
bpf_program__set_autoattach(freplace_prog, true);
bpf_program__set_attach_target(freplace_prog,
bpf_program__fd(main->progs.dummy),
main_prog_name);
err = changes_pkt_data_freplace__load(freplace);
print_verifier_log(log);
if (expect_load) {
ASSERT_OK(err, "changes_pkt_data_freplace__load");
} else {
ASSERT_ERR(err, "changes_pkt_data_freplace__load");
ASSERT_HAS_SUBSTR(log, "Extension program changes packet data", "error log");
}
out:
changes_pkt_data_freplace__destroy(freplace);
changes_pkt_data__destroy(main);
}
/* There are two global subprograms in both changes_pkt_data.skel.h:
* - one changes packet data;
* - another does not.
* It is ok to freplace subprograms that change packet data with those
* that either do or do not. It is only ok to freplace subprograms
* that do not change packet data with those that do not as well.
* The below tests check outcomes for each combination of such freplace.
*/
void test_changes_pkt_data_freplace(void)
{
if (test__start_subtest("changes_with_changes"))
test_aux("changes_pkt_data", "changes_pkt_data", true);
if (test__start_subtest("changes_with_doesnt_change"))
test_aux("changes_pkt_data", "does_not_change_pkt_data", true);
if (test__start_subtest("doesnt_change_with_changes"))
test_aux("does_not_change_pkt_data", "changes_pkt_data", false);
if (test__start_subtest("doesnt_change_with_doesnt_change"))
test_aux("does_not_change_pkt_data", "does_not_change_pkt_data", true);
}

26
tools/testing/selftests/bpf/progs/changes_pkt_data.c Normal file
View File

@ -0,0 +1,26 @@
// SPDX-License-Identifier: GPL-2.0
#include <linux/bpf.h>
#include <bpf/bpf_helpers.h>
__noinline
long changes_pkt_data(struct __sk_buff *sk, __u32 len)
{
return bpf_skb_pull_data(sk, len);
}
__noinline __weak
long does_not_change_pkt_data(struct __sk_buff *sk, __u32 len)
{
return 0;
}
SEC("tc")
int dummy(struct __sk_buff *sk)
{
changes_pkt_data(sk, 0);
does_not_change_pkt_data(sk, 0);
return 0;
}
char _license[] SEC("license") = "GPL";

18
tools/testing/selftests/bpf/progs/changes_pkt_data_freplace.c Normal file
View File

@ -0,0 +1,18 @@
// SPDX-License-Identifier: GPL-2.0
#include <linux/bpf.h>
#include <bpf/bpf_helpers.h>
SEC("?freplace")
long changes_pkt_data(struct __sk_buff *sk, __u32 len)
{
return bpf_skb_pull_data(sk, len);
}
SEC("?freplace")
long does_not_change_pkt_data(struct __sk_buff *sk, __u32 len)
{
return 0;
}
char _license[] SEC("license") = "GPL";

2
tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
View File

@ -11,6 +11,8 @@ int subprog_tc(struct __sk_buff *skb)
__sink(skb);
__sink(ret);
/* let verifier know that 'subprog_tc' can change pointers to skb->data */
bpf_skb_change_proto(skb, 0, 0);
return ret;
}

56
tools/testing/selftests/bpf/progs/verifier_sock.c
View File

@ -50,6 +50,13 @@ struct {
__uint(map_flags, BPF_F_NO_PREALLOC);
} sk_storage_map SEC(".maps");
struct {
__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
__uint(max_entries, 1);
__uint(key_size, sizeof(__u32));
__uint(value_size, sizeof(__u32));
} jmp_table SEC(".maps");
SEC("cgroup/skb")
__description("skb->sk: no NULL check")
__failure __msg("invalid mem access 'sock_common_or_null'")
@ -1037,4 +1044,53 @@ __naked void sock_create_read_src_port(void)
: __clobber_all);
}
__noinline
long skb_pull_data2(struct __sk_buff *sk, __u32 len)
{
return bpf_skb_pull_data(sk, len);
}
__noinline
long skb_pull_data1(struct __sk_buff *sk, __u32 len)
{
return skb_pull_data2(sk, len);
}
/* global function calls bpf_skb_pull_data(), which invalidates packet
* pointers established before global function call.
*/
SEC("tc")
__failure __msg("invalid mem access")
int invalidate_pkt_pointers_from_global_func(struct __sk_buff *sk)
{
int *p = (void *)(long)sk->data;
if ((void *)(p + 1) > (void *)(long)sk->data_end)
return TCX_DROP;
skb_pull_data1(sk, 0);
*p = 42; /* this is unsafe */
return TCX_PASS;
}
__noinline
int tail_call(struct __sk_buff *sk)
{
bpf_tail_call_static(sk, &jmp_table, 0);
return 0;
}
/* Tail calls invalidate packet pointers. */
SEC("tc")
__failure __msg("invalid mem access")
int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk)
{
int *p = (void *)(long)sk->data;
if ((void *)(p + 1) > (void *)(long)sk->data_end)
return TCX_DROP;
tail_call(sk);
*p = 42; /* this is unsafe */
return TCX_PASS;
}
char _license[] SEC("license") = "GPL";