* [PATCH bpf-next v2 0/3] bpf: Prevent writing read-only memory
@ 2021-10-25 23:12 Hao Luo
2021-10-25 23:12 ` [PATCH bpf-next 1/3] bpf: Prevent write to ksym memory Hao Luo
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Hao Luo @ 2021-10-25 23:12 UTC (permalink / raw)
To: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann
Cc: KP Singh, bpf, Hao Luo
Currently there are two ways to modify a kernel memory in bpf programs:
1. declare a ksym of scalar type and directly modify its memory.
2. Pass a RDONLY_BUF into a helper function which will override
its arguments. For example, bpf_d_path, bpf_snprintf.
This patchset fixes these two problem. For the first, we introduce a
new reg type PTR_TO_RDONLY_MEM for the scalar typed ksym, which forbids
writing. Second, we introduce a new arg type ARG_PTR_TO_WRITABLE_MEM,
which is a proper subset of the ARG_PTR_TO_MEM and includes only those
reg types that are writable. For helper functions that may override its
argument, they should use ARG_PTR_TO_WRITABLE_MEM. For other helper
functions, they can continue using ARG_PTR_TO_MEM.
There is an alternative solution to the second problem, that is, an
ARG_PTR_TO_CONST_MEM, which represents the current ARG_PTR_TO_MEM, and
ARG_PTR_TO_MEM, which represents the ARG_PTR_TO_WRITABLE_MEM in this
patchset. But I find the naming here is too confusing. Most of the
helper functions should not override their arguments, therefore, using
ARG_PTR_TO_MEM sounds natural.
Hao Luo (3):
bpf: Prevent write to ksym memory
bpf: Introduce ARG_PTR_TO_WRITABLE_MEM
bpf/selftests: Test PTR_TO_RDONLY_MEM
include/linux/bpf.h | 15 +++++-
include/uapi/linux/bpf.h | 4 +-
kernel/bpf/cgroup.c | 2 +-
kernel/bpf/helpers.c | 6 +--
kernel/bpf/verifier.c | 54 ++++++++++++++++---
kernel/trace/bpf_trace.c | 6 +--
net/core/filter.c | 6 +--
tools/include/uapi/linux/bpf.h | 4 +-
.../selftests/bpf/prog_tests/ksyms_btf.c | 14 +++++
.../bpf/progs/test_ksyms_btf_write_check.c | 29 ++++++++++
10 files changed, 116 insertions(+), 24 deletions(-)
create mode 100644 tools/testing/selftests/bpf/progs/test_ksyms_btf_write_check.c
--
2.33.0.1079.g6e70778dc9-goog
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH bpf-next 1/3] bpf: Prevent write to ksym memory 2021-10-25 23:12 [PATCH bpf-next v2 0/3] bpf: Prevent writing read-only memory Hao Luo @ 2021-10-25 23:12 ` Hao Luo 2021-10-25 23:12 ` [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM Hao Luo 2021-10-25 23:12 ` [PATCH bpf-next v2 3/3] bpf/selftests: Test PTR_TO_RDONLY_MEM Hao Luo 2 siblings, 0 replies; 15+ messages in thread From: Hao Luo @ 2021-10-25 23:12 UTC (permalink / raw) To: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann Cc: KP Singh, bpf, Hao Luo A ksym could be used to refer a global variable in the kernel. Previously if this variable is of non-struct type, bpf verifier resolves its value type to be PTR_TO_MEM, which allows the program to write to the memory. This patch introduces PTR_TO_RDONLY_MEM, which is similar to PTR_TO_MEM, but forbids writing. This should prevent program from writing kernel memory through ksyms. Right now a PTR_TO_RDONLY_MEM can not be passed into any helper as argument. But a PTR_TO_RDONLY_MEM can be read into a PTR_TO_MEM (e.g. stack variable), which can be passed to helpers such as bpf_snprintf. The following patch will add checks to differentiate the read-write arguments and read-only arguments, and support for passing PTR_TO_RDONLY_MEM to helper's read-only arguments. Fixes: 63d9b80dcf2c ("bpf: Introducte bpf_this_cpu_ptr()") Fixes: eaa6bcb71ef6 ("bpf: Introduce bpf_per_cpu_ptr()") Fixes: 4976b718c355 ("bpf: Introduce pseudo_btf_id") Signed-off-by: Hao Luo <haoluo@google.com> --- Changes since v1: - Added Fixes tag. - Removed PTR_TO_RDONLY_MEM[_OR_NULL] from reg_type_may_be_refcounted. include/linux/bpf.h | 6 ++++-- include/uapi/linux/bpf.h | 4 ++-- kernel/bpf/helpers.c | 4 ++-- kernel/bpf/verifier.c | 36 ++++++++++++++++++++++++++-------- tools/include/uapi/linux/bpf.h | 4 ++-- 5 files changed, 38 insertions(+), 16 deletions(-) diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 31421c74ba08..7b47e8f344cb 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -355,8 +355,8 @@ enum bpf_return_type { RET_PTR_TO_SOCK_COMMON_OR_NULL, /* returns a pointer to a sock_common or NULL */ RET_PTR_TO_ALLOC_MEM_OR_NULL, /* returns a pointer to dynamically allocated memory or NULL */ RET_PTR_TO_BTF_ID_OR_NULL, /* returns a pointer to a btf_id or NULL */ - RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL, /* returns a pointer to a valid memory or a btf_id or NULL */ - RET_PTR_TO_MEM_OR_BTF_ID, /* returns a pointer to a valid memory or a btf_id */ + RET_PTR_TO_RDONLY_MEM_OR_BTF_ID_OR_NULL, /* returns a pointer to a readonly memory or a btf_id or NULL */ + RET_PTR_TO_RDONLY_MEM_OR_BTF_ID, /* returns a pointer to a readonly memory or a btf_id */ RET_PTR_TO_BTF_ID, /* returns a pointer to a btf_id */ }; @@ -459,6 +459,8 @@ enum bpf_reg_type { PTR_TO_PERCPU_BTF_ID, /* reg points to a percpu kernel variable */ PTR_TO_FUNC, /* reg points to a bpf program function */ PTR_TO_MAP_KEY, /* reg points to a map element key */ + PTR_TO_RDONLY_MEM, /* reg points to valid readonly memory region */ + PTR_TO_RDONLY_MEM_OR_NULL, /* reg points to valid readonly memory region or null */ __BPF_REG_TYPE_MAX, }; diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index c10820037883..9fb931de668a 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -1141,8 +1141,8 @@ enum bpf_link_type { * insn[0].off: 0 * insn[1].off: 0 * ldimm64 rewrite: address of the kernel variable - * verifier type: PTR_TO_BTF_ID or PTR_TO_MEM, depending on whether the var - * is struct/union. + * verifier type: PTR_TO_BTF_ID or PTR_TO_RDONLY_MEM, depending on whether + * the var is struct/union. */ #define BPF_PSEUDO_BTF_ID 3 /* insn[0].src_reg: BPF_PSEUDO_FUNC diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 1ffd469c217f..14531757087f 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -667,7 +667,7 @@ BPF_CALL_2(bpf_per_cpu_ptr, const void *, ptr, u32, cpu) const struct bpf_func_proto bpf_per_cpu_ptr_proto = { .func = bpf_per_cpu_ptr, .gpl_only = false, - .ret_type = RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL, + .ret_type = RET_PTR_TO_RDONLY_MEM_OR_BTF_ID_OR_NULL, .arg1_type = ARG_PTR_TO_PERCPU_BTF_ID, .arg2_type = ARG_ANYTHING, }; @@ -680,7 +680,7 @@ BPF_CALL_1(bpf_this_cpu_ptr, const void *, percpu_ptr) const struct bpf_func_proto bpf_this_cpu_ptr_proto = { .func = bpf_this_cpu_ptr, .gpl_only = false, - .ret_type = RET_PTR_TO_MEM_OR_BTF_ID, + .ret_type = RET_PTR_TO_RDONLY_MEM_OR_BTF_ID, .arg1_type = ARG_PTR_TO_PERCPU_BTF_ID, }; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index c6616e325803..ae3ff297240e 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -453,6 +453,7 @@ static bool reg_type_may_be_null(enum bpf_reg_type type) type == PTR_TO_TCP_SOCK_OR_NULL || type == PTR_TO_BTF_ID_OR_NULL || type == PTR_TO_MEM_OR_NULL || + type == PTR_TO_RDONLY_MEM_OR_NULL || type == PTR_TO_RDONLY_BUF_OR_NULL || type == PTR_TO_RDWR_BUF_OR_NULL; } @@ -571,6 +572,8 @@ static const char * const reg_type_str[] = { [PTR_TO_PERCPU_BTF_ID] = "percpu_ptr_", [PTR_TO_MEM] = "mem", [PTR_TO_MEM_OR_NULL] = "mem_or_null", + [PTR_TO_RDONLY_MEM] = "rdonly_mem", + [PTR_TO_RDONLY_MEM_OR_NULL] = "rdonly_mem_or_null", [PTR_TO_RDONLY_BUF] = "rdonly_buf", [PTR_TO_RDONLY_BUF_OR_NULL] = "rdonly_buf_or_null", [PTR_TO_RDWR_BUF] = "rdwr_buf", @@ -1183,6 +1186,9 @@ static void mark_ptr_not_null_reg(struct bpf_reg_state *reg) case PTR_TO_MEM_OR_NULL: reg->type = PTR_TO_MEM; break; + case PTR_TO_RDONLY_MEM_OR_NULL: + reg->type = PTR_TO_RDONLY_MEM; + break; case PTR_TO_RDONLY_BUF_OR_NULL: reg->type = PTR_TO_RDONLY_BUF; break; @@ -2741,6 +2747,8 @@ static bool is_spillable_regtype(enum bpf_reg_type type) case PTR_TO_PERCPU_BTF_ID: case PTR_TO_MEM: case PTR_TO_MEM_OR_NULL: + case PTR_TO_RDONLY_MEM: + case PTR_TO_RDONLY_MEM_OR_NULL: case PTR_TO_FUNC: case PTR_TO_MAP_KEY: return true; @@ -3367,6 +3375,7 @@ static int __check_mem_access(struct bpf_verifier_env *env, int regno, off, size, regno, reg->id, off, mem_size); break; case PTR_TO_MEM: + case PTR_TO_RDONLY_MEM: default: verbose(env, "invalid access to memory, mem_size=%u off=%d size=%d\n", mem_size, off, size); @@ -4377,6 +4386,16 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn reg->mem_size, false); if (!err && t == BPF_READ && value_regno >= 0) mark_reg_unknown(env, regs, value_regno); + } else if (reg->type == PTR_TO_RDONLY_MEM) { + if (t == BPF_WRITE) { + verbose(env, "R%d cannot write into %s\n", + regno, reg_type_str[reg->type]); + return -EACCES; + } + err = check_mem_region_access(env, regno, off, size, + reg->mem_size, false); + if (!err && value_regno >= 0) + mark_reg_unknown(env, regs, value_regno); } else if (reg->type == PTR_TO_CTX) { enum bpf_reg_type reg_type = SCALAR_VALUE; struct btf *btf = NULL; @@ -6532,8 +6551,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn mark_reg_known_zero(env, regs, BPF_REG_0); regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL; regs[BPF_REG_0].mem_size = meta.mem_size; - } else if (fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL || - fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID) { + } else if (fn->ret_type == RET_PTR_TO_RDONLY_MEM_OR_BTF_ID_OR_NULL || + fn->ret_type == RET_PTR_TO_RDONLY_MEM_OR_BTF_ID) { const struct btf_type *t; mark_reg_known_zero(env, regs, BPF_REG_0); @@ -6552,12 +6571,12 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn return -EINVAL; } regs[BPF_REG_0].type = - fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID ? - PTR_TO_MEM : PTR_TO_MEM_OR_NULL; + fn->ret_type == RET_PTR_TO_RDONLY_MEM_OR_BTF_ID ? + PTR_TO_RDONLY_MEM : PTR_TO_RDONLY_MEM_OR_NULL; regs[BPF_REG_0].mem_size = tsize; } else { regs[BPF_REG_0].type = - fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID ? + fn->ret_type == RET_PTR_TO_RDONLY_MEM_OR_BTF_ID ? PTR_TO_BTF_ID : PTR_TO_BTF_ID_OR_NULL; regs[BPF_REG_0].btf = meta.ret_btf; regs[BPF_REG_0].btf_id = meta.ret_btf_id; @@ -9363,7 +9382,7 @@ static int check_ld_imm(struct bpf_verifier_env *env, struct bpf_insn *insn) dst_reg->type = aux->btf_var.reg_type; switch (dst_reg->type) { - case PTR_TO_MEM: + case PTR_TO_RDONLY_MEM: dst_reg->mem_size = aux->btf_var.mem_size; break; case PTR_TO_BTF_ID: @@ -11510,7 +11529,7 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env, err = -EINVAL; goto err_put; } - aux->btf_var.reg_type = PTR_TO_MEM; + aux->btf_var.reg_type = PTR_TO_RDONLY_MEM; aux->btf_var.mem_size = tsize; } else { aux->btf_var.reg_type = PTR_TO_BTF_ID; @@ -13332,7 +13351,8 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog) mark_reg_known_zero(env, regs, i); else if (regs[i].type == SCALAR_VALUE) mark_reg_unknown(env, regs, i); - else if (regs[i].type == PTR_TO_MEM_OR_NULL) { + else if (regs[i].type == PTR_TO_MEM_OR_NULL || + regs[i].type == PTR_TO_RDONLY_MEM_OR_NULL) { const u32 mem_size = regs[i].mem_size; mark_reg_known_zero(env, regs, i); diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index c10820037883..9fb931de668a 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -1141,8 +1141,8 @@ enum bpf_link_type { * insn[0].off: 0 * insn[1].off: 0 * ldimm64 rewrite: address of the kernel variable - * verifier type: PTR_TO_BTF_ID or PTR_TO_MEM, depending on whether the var - * is struct/union. + * verifier type: PTR_TO_BTF_ID or PTR_TO_RDONLY_MEM, depending on whether + * the var is struct/union. */ #define BPF_PSEUDO_BTF_ID 3 /* insn[0].src_reg: BPF_PSEUDO_FUNC -- 2.33.0.1079.g6e70778dc9-goog ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM 2021-10-25 23:12 [PATCH bpf-next v2 0/3] bpf: Prevent writing read-only memory Hao Luo 2021-10-25 23:12 ` [PATCH bpf-next 1/3] bpf: Prevent write to ksym memory Hao Luo @ 2021-10-25 23:12 ` Hao Luo 2021-10-26 3:48 ` Alexei Starovoitov 2021-10-26 5:06 ` Andrii Nakryiko 2021-10-25 23:12 ` [PATCH bpf-next v2 3/3] bpf/selftests: Test PTR_TO_RDONLY_MEM Hao Luo 2 siblings, 2 replies; 15+ messages in thread From: Hao Luo @ 2021-10-25 23:12 UTC (permalink / raw) To: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann Cc: KP Singh, bpf, Hao Luo Some helper functions may modify its arguments, for example, bpf_d_path, bpf_get_stack etc. Previously, their argument types were marked as ARG_PTR_TO_MEM, which is compatible with read-only mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate to modify a read-only memory by passing it into one of such helper functions. This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to annotate the arguments that may be modified by the helpers. For arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type, while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not acceptable. In short, when a helper may modify its input parameter, use ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM. So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is only used in bpf_iter prog as the type of key, which hasn't been used in the affected helper functions. PTR_TO_RDONLY_MEM currently has no consumers. Signed-off-by: Hao Luo <haoluo@google.com> --- Changes since v1: - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate read-only and read-write mem arg types. include/linux/bpf.h | 9 +++++++++ kernel/bpf/cgroup.c | 2 +- kernel/bpf/helpers.c | 2 +- kernel/bpf/verifier.c | 18 ++++++++++++++++++ kernel/trace/bpf_trace.c | 6 +++--- net/core/filter.c | 6 +++--- 6 files changed, 35 insertions(+), 8 deletions(-) diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 7b47e8f344cb..586ce67d63a9 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -341,6 +341,15 @@ enum bpf_arg_type { ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */ ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */ ARG_PTR_TO_TIMER, /* pointer to bpf_timer */ + ARG_PTR_TO_WRITABLE_MEM, /* pointer to valid memory. Compared to + * ARG_PTR_TO_MEM, this arg_type is not + * compatible with RDONLY memory. If the + * argument may be updated by the helper, + * use this type. + */ + ARG_PTR_TO_WRITABLE_MEM_OR_NULL, /* pointer to memory or null, similar to + * ARG_PTR_TO_WRITABLE_MEM. + */ __BPF_ARG_TYPE_MAX, }; diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c index 03145d45e3d5..683269b7cd92 100644 --- a/kernel/bpf/cgroup.c +++ b/kernel/bpf/cgroup.c @@ -1666,7 +1666,7 @@ static const struct bpf_func_proto bpf_sysctl_get_name_proto = { .gpl_only = false, .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_CTX, - .arg2_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, .arg3_type = ARG_CONST_SIZE, .arg4_type = ARG_ANYTHING, }; diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 14531757087f..db98385ee7af 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -1008,7 +1008,7 @@ const struct bpf_func_proto bpf_snprintf_proto = { .func = bpf_snprintf, .gpl_only = true, .ret_type = RET_INTEGER, - .arg1_type = ARG_PTR_TO_MEM_OR_NULL, + .arg1_type = ARG_PTR_TO_WRITABLE_MEM_OR_NULL, .arg2_type = ARG_CONST_SIZE_OR_ZERO, .arg3_type = ARG_PTR_TO_CONST_STR, .arg4_type = ARG_PTR_TO_MEM_OR_NULL, diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index ae3ff297240e..d8817c3d990e 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -486,6 +486,7 @@ static bool arg_type_may_be_null(enum bpf_arg_type type) type == ARG_PTR_TO_CTX_OR_NULL || type == ARG_PTR_TO_SOCKET_OR_NULL || type == ARG_PTR_TO_ALLOC_MEM_OR_NULL || + type == ARG_PTR_TO_WRITABLE_MEM_OR_NULL || type == ARG_PTR_TO_STACK_OR_NULL; } @@ -4971,6 +4972,8 @@ static bool arg_type_is_mem_ptr(enum bpf_arg_type type) { return type == ARG_PTR_TO_MEM || type == ARG_PTR_TO_MEM_OR_NULL || + type == ARG_PTR_TO_WRITABLE_MEM || + type == ARG_PTR_TO_WRITABLE_MEM_OR_NULL || type == ARG_PTR_TO_UNINIT_MEM; } @@ -5075,6 +5078,19 @@ static const struct bpf_reg_types mem_types = { PTR_TO_MEM, PTR_TO_RDONLY_BUF, PTR_TO_RDWR_BUF, + PTR_TO_RDONLY_MEM, + }, +}; + +static const struct bpf_reg_types writable_mem_types = { + .types = { + PTR_TO_STACK, + PTR_TO_PACKET, + PTR_TO_PACKET_META, + PTR_TO_MAP_KEY, + PTR_TO_MAP_VALUE, + PTR_TO_MEM, + PTR_TO_RDWR_BUF, }, }; @@ -5125,6 +5141,8 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { [ARG_PTR_TO_UNINIT_MEM] = &mem_types, [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types, [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, + [ARG_PTR_TO_WRITABLE_MEM] = &writable_mem_types, + [ARG_PTR_TO_WRITABLE_MEM_OR_NULL] = &writable_mem_types, [ARG_PTR_TO_INT] = &int_ptr_types, [ARG_PTR_TO_LONG] = &int_ptr_types, [ARG_PTR_TO_PERCPU_BTF_ID] = &percpu_btf_ptr_types, diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index cbcd0d6fca7c..5815845222de 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -945,7 +945,7 @@ static const struct bpf_func_proto bpf_d_path_proto = { .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_BTF_ID, .arg1_btf_id = &bpf_d_path_btf_ids[0], - .arg2_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, .arg3_type = ARG_CONST_SIZE_OR_ZERO, .allowed = bpf_d_path_allowed, }; @@ -1002,7 +1002,7 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = { .func = bpf_snprintf_btf, .gpl_only = false, .ret_type = RET_INTEGER, - .arg1_type = ARG_PTR_TO_MEM, + .arg1_type = ARG_PTR_TO_WRITABLE_MEM, .arg2_type = ARG_CONST_SIZE, .arg3_type = ARG_PTR_TO_MEM, .arg4_type = ARG_CONST_SIZE, @@ -1433,7 +1433,7 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = { .gpl_only = true, .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_CTX, - .arg2_type = ARG_PTR_TO_MEM_OR_NULL, + .arg2_type = ARG_PTR_TO_WRITABLE_MEM_OR_NULL, .arg3_type = ARG_CONST_SIZE_OR_ZERO, .arg4_type = ARG_ANYTHING, }; diff --git a/net/core/filter.c b/net/core/filter.c index 8e8d3b49c297..7dadabe12ceb 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -5639,7 +5639,7 @@ static const struct bpf_func_proto bpf_xdp_fib_lookup_proto = { .gpl_only = true, .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_CTX, - .arg2_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, .arg3_type = ARG_CONST_SIZE, .arg4_type = ARG_ANYTHING, }; @@ -5694,7 +5694,7 @@ static const struct bpf_func_proto bpf_skb_fib_lookup_proto = { .gpl_only = true, .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_CTX, - .arg2_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, .arg3_type = ARG_CONST_SIZE, .arg4_type = ARG_ANYTHING, }; @@ -6977,7 +6977,7 @@ static const struct bpf_func_proto bpf_sock_ops_load_hdr_opt_proto = { .gpl_only = false, .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_CTX, - .arg2_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, .arg3_type = ARG_CONST_SIZE, .arg4_type = ARG_ANYTHING, }; -- 2.33.0.1079.g6e70778dc9-goog ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM 2021-10-25 23:12 ` [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM Hao Luo @ 2021-10-26 3:48 ` Alexei Starovoitov 2021-10-26 5:14 ` Andrii Nakryiko 2021-10-26 5:06 ` Andrii Nakryiko 1 sibling, 1 reply; 15+ messages in thread From: Alexei Starovoitov @ 2021-10-26 3:48 UTC (permalink / raw) To: Hao Luo Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, KP Singh, bpf On Mon, Oct 25, 2021 at 04:12:55PM -0700, Hao Luo wrote: > Some helper functions may modify its arguments, for example, > bpf_d_path, bpf_get_stack etc. Previously, their argument types > were marked as ARG_PTR_TO_MEM, which is compatible with read-only > mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate > to modify a read-only memory by passing it into one of such helper > functions. > > This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to > annotate the arguments that may be modified by the helpers. For > arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type, > while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not > acceptable. > > In short, when a helper may modify its input parameter, use > ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM. > > So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM > is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is > only used in bpf_iter prog as the type of key, which hasn't been > used in the affected helper functions. PTR_TO_RDONLY_MEM currently > has no consumers. > > Signed-off-by: Hao Luo <haoluo@google.com> > --- > Changes since v1: > - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate > read-only and read-write mem arg types. > > include/linux/bpf.h | 9 +++++++++ > kernel/bpf/cgroup.c | 2 +- > kernel/bpf/helpers.c | 2 +- > kernel/bpf/verifier.c | 18 ++++++++++++++++++ > kernel/trace/bpf_trace.c | 6 +++--- > net/core/filter.c | 6 +++--- > 6 files changed, 35 insertions(+), 8 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 7b47e8f344cb..586ce67d63a9 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -341,6 +341,15 @@ enum bpf_arg_type { > ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */ > ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */ > ARG_PTR_TO_TIMER, /* pointer to bpf_timer */ > + ARG_PTR_TO_WRITABLE_MEM, /* pointer to valid memory. Compared to > + * ARG_PTR_TO_MEM, this arg_type is not > + * compatible with RDONLY memory. If the > + * argument may be updated by the helper, > + * use this type. > + */ > + ARG_PTR_TO_WRITABLE_MEM_OR_NULL, /* pointer to memory or null, similar to > + * ARG_PTR_TO_WRITABLE_MEM. > + */ Instead of adding new types, can we do something like this instead: diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index c8a78e830fca..5dbd2541aa86 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -68,7 +68,8 @@ struct bpf_reg_state { u32 btf_id; }; - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ + u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ + u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ /* Max size from any of the above. */ struct { diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index c6616e325803..ad46169d422b 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4374,7 +4374,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn return -EACCES; } err = check_mem_region_access(env, regno, off, size, - reg->mem_size, false); + t == BPF_WRITE ? reg->wr_mem_size : reg->rd_mem_size, false); if (!err && t == BPF_READ && value_regno >= 0) mark_reg_unknown(env, regs, value_regno); } else if (reg->type == PTR_TO_CTX) { @@ -11511,7 +11511,8 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env, goto err_put; } aux->btf_var.reg_type = PTR_TO_MEM; - aux->btf_var.mem_size = tsize; + aux->btf_var.rd_mem_size = tsize; + aux->btf_var.wr_mem_size = 0; } else { aux->btf_var.reg_type = PTR_TO_BTF_ID; aux->btf_var.btf = btf; ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM 2021-10-26 3:48 ` Alexei Starovoitov @ 2021-10-26 5:14 ` Andrii Nakryiko 2021-10-26 17:59 ` Alexei Starovoitov 0 siblings, 1 reply; 15+ messages in thread From: Andrii Nakryiko @ 2021-10-26 5:14 UTC (permalink / raw) To: Alexei Starovoitov Cc: Hao Luo, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, KP Singh, bpf On Mon, Oct 25, 2021 at 8:48 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Oct 25, 2021 at 04:12:55PM -0700, Hao Luo wrote: > > Some helper functions may modify its arguments, for example, > > bpf_d_path, bpf_get_stack etc. Previously, their argument types > > were marked as ARG_PTR_TO_MEM, which is compatible with read-only > > mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate > > to modify a read-only memory by passing it into one of such helper > > functions. > > > > This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to > > annotate the arguments that may be modified by the helpers. For > > arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type, > > while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not > > acceptable. > > > > In short, when a helper may modify its input parameter, use > > ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM. > > > > So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM > > is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is > > only used in bpf_iter prog as the type of key, which hasn't been > > used in the affected helper functions. PTR_TO_RDONLY_MEM currently > > has no consumers. > > > > Signed-off-by: Hao Luo <haoluo@google.com> > > --- > > Changes since v1: > > - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate > > read-only and read-write mem arg types. > > > > include/linux/bpf.h | 9 +++++++++ > > kernel/bpf/cgroup.c | 2 +- > > kernel/bpf/helpers.c | 2 +- > > kernel/bpf/verifier.c | 18 ++++++++++++++++++ > > kernel/trace/bpf_trace.c | 6 +++--- > > net/core/filter.c | 6 +++--- > > 6 files changed, 35 insertions(+), 8 deletions(-) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index 7b47e8f344cb..586ce67d63a9 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -341,6 +341,15 @@ enum bpf_arg_type { > > ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */ > > ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */ > > ARG_PTR_TO_TIMER, /* pointer to bpf_timer */ > > + ARG_PTR_TO_WRITABLE_MEM, /* pointer to valid memory. Compared to > > + * ARG_PTR_TO_MEM, this arg_type is not > > + * compatible with RDONLY memory. If the > > + * argument may be updated by the helper, > > + * use this type. > > + */ > > + ARG_PTR_TO_WRITABLE_MEM_OR_NULL, /* pointer to memory or null, similar to > > + * ARG_PTR_TO_WRITABLE_MEM. > > + */ > > Instead of adding new types, > can we do something like this instead: > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > index c8a78e830fca..5dbd2541aa86 100644 > --- a/include/linux/bpf_verifier.h > +++ b/include/linux/bpf_verifier.h > @@ -68,7 +68,8 @@ struct bpf_reg_state { > u32 btf_id; > }; > > - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > + u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > + u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ This seems more confusing, it's technically possible to express a memory pointer from which you can read X bytes, but can write Y bytes. I actually liked the idea that helpers will be explicit about whether they can write into a memory or only read from it. Apart from a few more lines of code, are there any downsides to having PTR_TO_MEM vs PTR_TO_RDONLY_MEM? BTW, this made me think about read-only (from the BPF side) maps. Seems like we have some special handling around that right now, but if we had PTR_TO_RDONLY_MEM and PTR_TO_MEM, could we have used that as a more uniform way to enforce read-only access to memory? > > /* Max size from any of the above. */ > struct { > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index c6616e325803..ad46169d422b 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -4374,7 +4374,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn > return -EACCES; > } > err = check_mem_region_access(env, regno, off, size, > - reg->mem_size, false); > + t == BPF_WRITE ? reg->wr_mem_size : reg->rd_mem_size, false); > if (!err && t == BPF_READ && value_regno >= 0) > mark_reg_unknown(env, regs, value_regno); > } else if (reg->type == PTR_TO_CTX) { > @@ -11511,7 +11511,8 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env, > goto err_put; > } > aux->btf_var.reg_type = PTR_TO_MEM; > - aux->btf_var.mem_size = tsize; > + aux->btf_var.rd_mem_size = tsize; > + aux->btf_var.wr_mem_size = 0; > } else { > aux->btf_var.reg_type = PTR_TO_BTF_ID; > aux->btf_var.btf = btf; > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM 2021-10-26 5:14 ` Andrii Nakryiko @ 2021-10-26 17:59 ` Alexei Starovoitov 2021-10-26 18:13 ` Hao Luo 2021-10-26 18:44 ` Andrii Nakryiko 0 siblings, 2 replies; 15+ messages in thread From: Alexei Starovoitov @ 2021-10-26 17:59 UTC (permalink / raw) To: Andrii Nakryiko Cc: Hao Luo, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, KP Singh, bpf On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > Instead of adding new types, > > can we do something like this instead: > > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > > index c8a78e830fca..5dbd2541aa86 100644 > > --- a/include/linux/bpf_verifier.h > > +++ b/include/linux/bpf_verifier.h > > @@ -68,7 +68,8 @@ struct bpf_reg_state { > > u32 btf_id; > > }; > > > > - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > + u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > + u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > This seems more confusing, it's technically possible to express a > memory pointer from which you can read X bytes, but can write Y bytes. I'm fine it being a new flag instead of wr_mem_size. > I actually liked the idea that helpers will be explicit about whether > they can write into a memory or only read from it. > > Apart from a few more lines of code, are there any downsides to having > PTR_TO_MEM vs PTR_TO_RDONLY_MEM? because it's a churn and non scalable long term. It's not just PTR_TO_RDONLY_MEM. It's also ARG_PTR_TO_RDONLY_MEM, and RET_PTR_TO_RDONLY_MEM, and PTR_TO_RDONLY_MEM_OR_NULL and *_OR_BTF_ID, and *_OR_BTF_ID_OR_NULL. It felt that expressing readonly-ness as a flag in bpf_reg_state will make it easier to get right in the code and extend in the future. May be we will have a kernel vs user flag for PTR_TO_MEM in the future. If we use different name to express that we will have: PTR_TO_USER_RDONLY_MEM and PTR_TO_USER_MEM plus all variants of ARG_* and RET_* and *_OR_NULL. With a flag approach it will be just another flag in bpf_reg_state. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM 2021-10-26 17:59 ` Alexei Starovoitov @ 2021-10-26 18:13 ` Hao Luo 2021-10-26 18:44 ` Andrii Nakryiko 1 sibling, 0 replies; 15+ messages in thread From: Hao Luo @ 2021-10-26 18:13 UTC (permalink / raw) To: Alexei Starovoitov Cc: Andrii Nakryiko, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, KP Singh, bpf On Tue, Oct 26, 2021 at 11:00 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > > > Instead of adding new types, > > > can we do something like this instead: > > > > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > > > index c8a78e830fca..5dbd2541aa86 100644 > > > --- a/include/linux/bpf_verifier.h > > > +++ b/include/linux/bpf_verifier.h > > > @@ -68,7 +68,8 @@ struct bpf_reg_state { > > > u32 btf_id; > > > }; > > > > > > - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > + u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > + u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > This seems more confusing, it's technically possible to express a > > memory pointer from which you can read X bytes, but can write Y bytes. > > I'm fine it being a new flag instead of wr_mem_size. > > > I actually liked the idea that helpers will be explicit about whether > > they can write into a memory or only read from it. > > > > Apart from a few more lines of code, are there any downsides to having > > PTR_TO_MEM vs PTR_TO_RDONLY_MEM? > > because it's a churn and non scalable long term. > It's not just PTR_TO_RDONLY_MEM. > It's also ARG_PTR_TO_RDONLY_MEM, > and RET_PTR_TO_RDONLY_MEM, > and PTR_TO_RDONLY_MEM_OR_NULL > and *_OR_BTF_ID, > and *_OR_BTF_ID_OR_NULL. > It felt that expressing readonly-ness as a flag in bpf_reg_state > will make it easier to get right in the code and extend in the future. > May be we will have a kernel vs user flag for PTR_TO_MEM in the future. > If we use different name to express that we will have: > PTR_TO_USER_RDONLY_MEM and > PTR_TO_USER_MEM > plus all variants of ARG_* and RET_* and *_OR_NULL. > With a flag approach it will be just another flag in bpf_reg_state. Totally agree. Adding a variant incurs exponential cost. Introducing another dimension in future may need to go over all the MEM, RDONLY_MEM, MEM_OR_NULL, RDONLY_MEM_OR_NULL, multiplied by ARG_*, RET_*, etc. It's a pain. I have that in mind and start thinking more about how can we do a more scalable flag approach. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM 2021-10-26 17:59 ` Alexei Starovoitov 2021-10-26 18:13 ` Hao Luo @ 2021-10-26 18:44 ` Andrii Nakryiko 2021-10-26 19:22 ` Alexei Starovoitov 1 sibling, 1 reply; 15+ messages in thread From: Andrii Nakryiko @ 2021-10-26 18:44 UTC (permalink / raw) To: Alexei Starovoitov Cc: Hao Luo, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, KP Singh, bpf On Tue, Oct 26, 2021 at 10:59 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > > > Instead of adding new types, > > > can we do something like this instead: > > > > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > > > index c8a78e830fca..5dbd2541aa86 100644 > > > --- a/include/linux/bpf_verifier.h > > > +++ b/include/linux/bpf_verifier.h > > > @@ -68,7 +68,8 @@ struct bpf_reg_state { > > > u32 btf_id; > > > }; > > > > > > - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > + u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > + u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > This seems more confusing, it's technically possible to express a > > memory pointer from which you can read X bytes, but can write Y bytes. > > I'm fine it being a new flag instead of wr_mem_size. > > > I actually liked the idea that helpers will be explicit about whether > > they can write into a memory or only read from it. > > > > Apart from a few more lines of code, are there any downsides to having > > PTR_TO_MEM vs PTR_TO_RDONLY_MEM? > > because it's a churn and non scalable long term. > It's not just PTR_TO_RDONLY_MEM. > It's also ARG_PTR_TO_RDONLY_MEM, > and RET_PTR_TO_RDONLY_MEM, > and PTR_TO_RDONLY_MEM_OR_NULL > and *_OR_BTF_ID, > and *_OR_BTF_ID_OR_NULL. > It felt that expressing readonly-ness as a flag in bpf_reg_state > will make it easier to get right in the code and extend in the future. That's true, but while it's easy to add a flag to bpf_reg_state, it's not easy to do the same for BPF helper input (ARG_PTR_xxx) and output (RET_PTR_xxx) restrictions. So unless we extend ARG_PTR and RET_PTR with flags, it seems more consistent to keep the same pure enum approach for reg_state. > May be we will have a kernel vs user flag for PTR_TO_MEM in the future. > If we use different name to express that we will have: > PTR_TO_USER_RDONLY_MEM and > PTR_TO_USER_MEM > plus all variants of ARG_* and RET_* and *_OR_NULL. > With a flag approach it will be just another flag in bpf_reg_state. All true, but then maybe we should rethink how we do all those enums. And instead of having all the _OR_NULL variants, it should be ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE flag that can be or-ed with the basic set of register/input/output type enums? Same for ARG_RDONLY flag. Same could technically be done for USER vs KERNEL memory in the future. It's definitely a bunch of code changes, but if we are worried about an explosion of enum values, it might be the right move? On the other hand, if there are all those different variations and each is handled slightly differently, we'll have to have different logic for each of them. And whether it's an enum + flags, or a few more enumerators, doesn't change anything fundamentally. I feel like enums make code discovery a bit simpler in practice, but it's subjective. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM 2021-10-26 18:44 ` Andrii Nakryiko @ 2021-10-26 19:22 ` Alexei Starovoitov 2021-10-26 21:24 ` Andrii Nakryiko 0 siblings, 1 reply; 15+ messages in thread From: Alexei Starovoitov @ 2021-10-26 19:22 UTC (permalink / raw) To: Andrii Nakryiko Cc: Hao Luo, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, KP Singh, bpf On Tue, Oct 26, 2021 at 11:45 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Oct 26, 2021 at 10:59 AM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > Instead of adding new types, > > > > can we do something like this instead: > > > > > > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > > > > index c8a78e830fca..5dbd2541aa86 100644 > > > > --- a/include/linux/bpf_verifier.h > > > > +++ b/include/linux/bpf_verifier.h > > > > @@ -68,7 +68,8 @@ struct bpf_reg_state { > > > > u32 btf_id; > > > > }; > > > > > > > > - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > + u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > + u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > > > This seems more confusing, it's technically possible to express a > > > memory pointer from which you can read X bytes, but can write Y bytes. > > > > I'm fine it being a new flag instead of wr_mem_size. > > > > > I actually liked the idea that helpers will be explicit about whether > > > they can write into a memory or only read from it. > > > > > > Apart from a few more lines of code, are there any downsides to having > > > PTR_TO_MEM vs PTR_TO_RDONLY_MEM? > > > > because it's a churn and non scalable long term. > > It's not just PTR_TO_RDONLY_MEM. > > It's also ARG_PTR_TO_RDONLY_MEM, > > and RET_PTR_TO_RDONLY_MEM, > > and PTR_TO_RDONLY_MEM_OR_NULL > > and *_OR_BTF_ID, > > and *_OR_BTF_ID_OR_NULL. > > It felt that expressing readonly-ness as a flag in bpf_reg_state > > will make it easier to get right in the code and extend in the future. > > That's true, but while it's easy to add a flag to bpf_reg_state, it's > not easy to do the same for BPF helper input (ARG_PTR_xxx) and output > (RET_PTR_xxx) restrictions. So unless we extend ARG_PTR and RET_PTR > with flags, it seems more consistent to keep the same pure enum > approach for reg_state. > > > May be we will have a kernel vs user flag for PTR_TO_MEM in the future. > > If we use different name to express that we will have: > > PTR_TO_USER_RDONLY_MEM and > > PTR_TO_USER_MEM > > plus all variants of ARG_* and RET_* and *_OR_NULL. > > With a flag approach it will be just another flag in bpf_reg_state. > > All true, but then maybe we should rethink how we do all those enums. > And instead of having all the _OR_NULL variants, it should be > ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE flag that can be or-ed with the > basic set of register/input/output type enums? Same for ARG_RDONLY > flag. Same could technically be done for USER vs KERNEL memory in the > future. Exactly. OR_NULL is such a flag and we already struggled to differentiate that flag with truly_not_equal_to_NULL and may_be_NULL. That's why all bpf_skc* helpers have additional run-time !NULL check. ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE would make it cleaner. And ARG_RDONLY would fit that model well. > It's definitely a bunch of code changes, but if we are worried about > an explosion of enum values, it might be the right move? > > On the other hand, if there are all those different variations and > each is handled slightly differently, we'll have to have different > logic for each of them. And whether it's an enum + flags, or a few > more enumerators, doesn't change anything fundamentally. I feel like > enums make code discovery a bit simpler in practice, but it's > subjective. I think it's a bit of a mess already. ARG_PTR_TO_BTF_ID has may_be_NULL flag. Just like ARG_PTR_TO_BTF_ID_SOCK_COMMON. but RET_PTR_TO_BTF_ID doesn't. PTR_TO_BTF_ID doesn't have that may_be_NULL assumption either. imo cleaning up OR_NULL will be very nice. RDONLY would be an addition on top. We can probably fold UNINIT as a flag too. All that will be a big change, but I think it's worth it. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM 2021-10-26 19:22 ` Alexei Starovoitov @ 2021-10-26 21:24 ` Andrii Nakryiko 0 siblings, 0 replies; 15+ messages in thread From: Andrii Nakryiko @ 2021-10-26 21:24 UTC (permalink / raw) To: Alexei Starovoitov Cc: Hao Luo, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, KP Singh, bpf On Tue, Oct 26, 2021 at 12:23 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Oct 26, 2021 at 11:45 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Oct 26, 2021 at 10:59 AM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > > > Instead of adding new types, > > > > > can we do something like this instead: > > > > > > > > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > > > > > index c8a78e830fca..5dbd2541aa86 100644 > > > > > --- a/include/linux/bpf_verifier.h > > > > > +++ b/include/linux/bpf_verifier.h > > > > > @@ -68,7 +68,8 @@ struct bpf_reg_state { > > > > > u32 btf_id; > > > > > }; > > > > > > > > > > - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > > + u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > > + u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > > > > > This seems more confusing, it's technically possible to express a > > > > memory pointer from which you can read X bytes, but can write Y bytes. > > > > > > I'm fine it being a new flag instead of wr_mem_size. > > > > > > > I actually liked the idea that helpers will be explicit about whether > > > > they can write into a memory or only read from it. > > > > > > > > Apart from a few more lines of code, are there any downsides to having > > > > PTR_TO_MEM vs PTR_TO_RDONLY_MEM? > > > > > > because it's a churn and non scalable long term. > > > It's not just PTR_TO_RDONLY_MEM. > > > It's also ARG_PTR_TO_RDONLY_MEM, > > > and RET_PTR_TO_RDONLY_MEM, > > > and PTR_TO_RDONLY_MEM_OR_NULL > > > and *_OR_BTF_ID, > > > and *_OR_BTF_ID_OR_NULL. > > > It felt that expressing readonly-ness as a flag in bpf_reg_state > > > will make it easier to get right in the code and extend in the future. > > > > That's true, but while it's easy to add a flag to bpf_reg_state, it's > > not easy to do the same for BPF helper input (ARG_PTR_xxx) and output > > (RET_PTR_xxx) restrictions. So unless we extend ARG_PTR and RET_PTR > > with flags, it seems more consistent to keep the same pure enum > > approach for reg_state. > > > > > May be we will have a kernel vs user flag for PTR_TO_MEM in the future. > > > If we use different name to express that we will have: > > > PTR_TO_USER_RDONLY_MEM and > > > PTR_TO_USER_MEM > > > plus all variants of ARG_* and RET_* and *_OR_NULL. > > > With a flag approach it will be just another flag in bpf_reg_state. > > > > All true, but then maybe we should rethink how we do all those enums. > > And instead of having all the _OR_NULL variants, it should be > > ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE flag that can be or-ed with the > > basic set of register/input/output type enums? Same for ARG_RDONLY > > flag. Same could technically be done for USER vs KERNEL memory in the > > future. > > Exactly. OR_NULL is such a flag and we already struggled to > differentiate that flag with truly_not_equal_to_NULL and may_be_NULL. > That's why all bpf_skc* helpers have additional run-time !NULL check. > > ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE would make it cleaner. > And ARG_RDONLY would fit that model well. > > > It's definitely a bunch of code changes, but if we are worried about > > an explosion of enum values, it might be the right move? > > > > On the other hand, if there are all those different variations and > > each is handled slightly differently, we'll have to have different > > logic for each of them. And whether it's an enum + flags, or a few > > more enumerators, doesn't change anything fundamentally. I feel like > > enums make code discovery a bit simpler in practice, but it's > > subjective. > > I think it's a bit of a mess already. > ARG_PTR_TO_BTF_ID has may_be_NULL flag. > Just like ARG_PTR_TO_BTF_ID_SOCK_COMMON. > but RET_PTR_TO_BTF_ID doesn't. > PTR_TO_BTF_ID doesn't have that may_be_NULL assumption either. > > imo cleaning up OR_NULL will be very nice. > RDONLY would be an addition on top. > We can probably fold UNINIT as a flag too. > > All that will be a big change, but I think it's worth it. Yep, agree. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM 2021-10-25 23:12 ` [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM Hao Luo 2021-10-26 3:48 ` Alexei Starovoitov @ 2021-10-26 5:06 ` Andrii Nakryiko 2021-10-26 17:51 ` Hao Luo 1 sibling, 1 reply; 15+ messages in thread From: Andrii Nakryiko @ 2021-10-26 5:06 UTC (permalink / raw) To: Hao Luo Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, KP Singh, bpf On Mon, Oct 25, 2021 at 4:13 PM Hao Luo <haoluo@google.com> wrote: > > Some helper functions may modify its arguments, for example, > bpf_d_path, bpf_get_stack etc. Previously, their argument types > were marked as ARG_PTR_TO_MEM, which is compatible with read-only > mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate > to modify a read-only memory by passing it into one of such helper > functions. > > This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to > annotate the arguments that may be modified by the helpers. For > arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type, > while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not > acceptable. > > In short, when a helper may modify its input parameter, use > ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM. This is inconsistent with the other uses where we have something that's writable by default and mark it as RDONLY if it's read-only. Same here, why not keep ARG_PTR_TO_MEM to mean "writable memory", and add ARG_PTR_TO_RDONLY_MEM for helpers that are not writing into the memory? It will also be safer default: if helper defines ARG_PTR_TO_MEM but never writes to it, worst thing that can happen would be that you won't be able to pass REG_PTR_TO_RDONLY_MEM into it without fixing helper definition. The other way is more dangerous. If ARG_PTR_TO_MEM means read-only mem and helper forgot this distinction and actually writes into the memory, then we are in much bigger trouble. > > So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM > is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is > only used in bpf_iter prog as the type of key, which hasn't been > used in the affected helper functions. PTR_TO_RDONLY_MEM currently > has no consumers. > > Signed-off-by: Hao Luo <haoluo@google.com> > --- > Changes since v1: > - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate > read-only and read-write mem arg types. > > include/linux/bpf.h | 9 +++++++++ > kernel/bpf/cgroup.c | 2 +- > kernel/bpf/helpers.c | 2 +- > kernel/bpf/verifier.c | 18 ++++++++++++++++++ > kernel/trace/bpf_trace.c | 6 +++--- > net/core/filter.c | 6 +++--- > 6 files changed, 35 insertions(+), 8 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 7b47e8f344cb..586ce67d63a9 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -341,6 +341,15 @@ enum bpf_arg_type { > ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */ > ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */ > ARG_PTR_TO_TIMER, /* pointer to bpf_timer */ > + ARG_PTR_TO_WRITABLE_MEM, /* pointer to valid memory. Compared to > + * ARG_PTR_TO_MEM, this arg_type is not > + * compatible with RDONLY memory. If the > + * argument may be updated by the helper, > + * use this type. > + */ > + ARG_PTR_TO_WRITABLE_MEM_OR_NULL, /* pointer to memory or null, similar to > + * ARG_PTR_TO_WRITABLE_MEM. > + */ > __BPF_ARG_TYPE_MAX, > }; > > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c > index 03145d45e3d5..683269b7cd92 100644 > --- a/kernel/bpf/cgroup.c > +++ b/kernel/bpf/cgroup.c > @@ -1666,7 +1666,7 @@ static const struct bpf_func_proto bpf_sysctl_get_name_proto = { > .gpl_only = false, > .ret_type = RET_INTEGER, > .arg1_type = ARG_PTR_TO_CTX, > - .arg2_type = ARG_PTR_TO_MEM, > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > .arg3_type = ARG_CONST_SIZE, > .arg4_type = ARG_ANYTHING, > }; > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > index 14531757087f..db98385ee7af 100644 > --- a/kernel/bpf/helpers.c > +++ b/kernel/bpf/helpers.c > @@ -1008,7 +1008,7 @@ const struct bpf_func_proto bpf_snprintf_proto = { > .func = bpf_snprintf, > .gpl_only = true, > .ret_type = RET_INTEGER, > - .arg1_type = ARG_PTR_TO_MEM_OR_NULL, > + .arg1_type = ARG_PTR_TO_WRITABLE_MEM_OR_NULL, > .arg2_type = ARG_CONST_SIZE_OR_ZERO, > .arg3_type = ARG_PTR_TO_CONST_STR, > .arg4_type = ARG_PTR_TO_MEM_OR_NULL, > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index ae3ff297240e..d8817c3d990e 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -486,6 +486,7 @@ static bool arg_type_may_be_null(enum bpf_arg_type type) > type == ARG_PTR_TO_CTX_OR_NULL || > type == ARG_PTR_TO_SOCKET_OR_NULL || > type == ARG_PTR_TO_ALLOC_MEM_OR_NULL || > + type == ARG_PTR_TO_WRITABLE_MEM_OR_NULL || > type == ARG_PTR_TO_STACK_OR_NULL; > } > > @@ -4971,6 +4972,8 @@ static bool arg_type_is_mem_ptr(enum bpf_arg_type type) > { > return type == ARG_PTR_TO_MEM || > type == ARG_PTR_TO_MEM_OR_NULL || > + type == ARG_PTR_TO_WRITABLE_MEM || > + type == ARG_PTR_TO_WRITABLE_MEM_OR_NULL || > type == ARG_PTR_TO_UNINIT_MEM; > } > > @@ -5075,6 +5078,19 @@ static const struct bpf_reg_types mem_types = { > PTR_TO_MEM, > PTR_TO_RDONLY_BUF, > PTR_TO_RDWR_BUF, > + PTR_TO_RDONLY_MEM, > + }, > +}; > + > +static const struct bpf_reg_types writable_mem_types = { > + .types = { > + PTR_TO_STACK, > + PTR_TO_PACKET, > + PTR_TO_PACKET_META, > + PTR_TO_MAP_KEY, > + PTR_TO_MAP_VALUE, > + PTR_TO_MEM, > + PTR_TO_RDWR_BUF, > }, > }; > > @@ -5125,6 +5141,8 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { > [ARG_PTR_TO_UNINIT_MEM] = &mem_types, > [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types, > [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, > + [ARG_PTR_TO_WRITABLE_MEM] = &writable_mem_types, > + [ARG_PTR_TO_WRITABLE_MEM_OR_NULL] = &writable_mem_types, > [ARG_PTR_TO_INT] = &int_ptr_types, > [ARG_PTR_TO_LONG] = &int_ptr_types, > [ARG_PTR_TO_PERCPU_BTF_ID] = &percpu_btf_ptr_types, > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index cbcd0d6fca7c..5815845222de 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -945,7 +945,7 @@ static const struct bpf_func_proto bpf_d_path_proto = { > .ret_type = RET_INTEGER, > .arg1_type = ARG_PTR_TO_BTF_ID, > .arg1_btf_id = &bpf_d_path_btf_ids[0], > - .arg2_type = ARG_PTR_TO_MEM, > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > .arg3_type = ARG_CONST_SIZE_OR_ZERO, > .allowed = bpf_d_path_allowed, > }; > @@ -1002,7 +1002,7 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = { > .func = bpf_snprintf_btf, > .gpl_only = false, > .ret_type = RET_INTEGER, > - .arg1_type = ARG_PTR_TO_MEM, > + .arg1_type = ARG_PTR_TO_WRITABLE_MEM, > .arg2_type = ARG_CONST_SIZE, > .arg3_type = ARG_PTR_TO_MEM, > .arg4_type = ARG_CONST_SIZE, > @@ -1433,7 +1433,7 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = { > .gpl_only = true, > .ret_type = RET_INTEGER, > .arg1_type = ARG_PTR_TO_CTX, > - .arg2_type = ARG_PTR_TO_MEM_OR_NULL, > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM_OR_NULL, > .arg3_type = ARG_CONST_SIZE_OR_ZERO, > .arg4_type = ARG_ANYTHING, > }; > diff --git a/net/core/filter.c b/net/core/filter.c > index 8e8d3b49c297..7dadabe12ceb 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -5639,7 +5639,7 @@ static const struct bpf_func_proto bpf_xdp_fib_lookup_proto = { > .gpl_only = true, > .ret_type = RET_INTEGER, > .arg1_type = ARG_PTR_TO_CTX, > - .arg2_type = ARG_PTR_TO_MEM, > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > .arg3_type = ARG_CONST_SIZE, > .arg4_type = ARG_ANYTHING, > }; > @@ -5694,7 +5694,7 @@ static const struct bpf_func_proto bpf_skb_fib_lookup_proto = { > .gpl_only = true, > .ret_type = RET_INTEGER, > .arg1_type = ARG_PTR_TO_CTX, > - .arg2_type = ARG_PTR_TO_MEM, > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > .arg3_type = ARG_CONST_SIZE, > .arg4_type = ARG_ANYTHING, > }; > @@ -6977,7 +6977,7 @@ static const struct bpf_func_proto bpf_sock_ops_load_hdr_opt_proto = { > .gpl_only = false, > .ret_type = RET_INTEGER, > .arg1_type = ARG_PTR_TO_CTX, > - .arg2_type = ARG_PTR_TO_MEM, > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > .arg3_type = ARG_CONST_SIZE, > .arg4_type = ARG_ANYTHING, > }; > -- > 2.33.0.1079.g6e70778dc9-goog > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM 2021-10-26 5:06 ` Andrii Nakryiko @ 2021-10-26 17:51 ` Hao Luo 2021-10-26 18:53 ` Andrii Nakryiko 0 siblings, 1 reply; 15+ messages in thread From: Hao Luo @ 2021-10-26 17:51 UTC (permalink / raw) To: Andrii Nakryiko Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, KP Singh, bpf On Mon, Oct 25, 2021 at 10:06 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Oct 25, 2021 at 4:13 PM Hao Luo <haoluo@google.com> wrote: > > > > Some helper functions may modify its arguments, for example, > > bpf_d_path, bpf_get_stack etc. Previously, their argument types > > were marked as ARG_PTR_TO_MEM, which is compatible with read-only > > mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate > > to modify a read-only memory by passing it into one of such helper > > functions. > > > > This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to > > annotate the arguments that may be modified by the helpers. For > > arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type, > > while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not > > acceptable. > > > > In short, when a helper may modify its input parameter, use > > ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM. > > This is inconsistent with the other uses where we have something > that's writable by default and mark it as RDONLY if it's read-only. > Same here, why not keep ARG_PTR_TO_MEM to mean "writable memory", and > add ARG_PTR_TO_RDONLY_MEM for helpers that are not writing into the > memory? It will also be safer default: if helper defines > ARG_PTR_TO_MEM but never writes to it, worst thing that can happen > would be that you won't be able to pass REG_PTR_TO_RDONLY_MEM into it > without fixing helper definition. The other way is more dangerous. If > ARG_PTR_TO_MEM means read-only mem and helper forgot this distinction > and actually writes into the memory, then we are in much bigger > trouble. > My original implementation was adding ARG_PTR_TO_RDONLY_MEM. But I find it's not intuitive for developers when adding helpers. The majority of PTR_TO_MEM arguments are read-only. When adding a new helper, I would expect the default arg type (that is, ARG_PTR_TO_MEM) to match the default case (that is, read-only argument). Requiring explicitly adding RDONLY to most cases seems a little unintuitive to me. But other than that, I agree that narrowing ARG_PTR_TO_MEM down to writable memory fosters more strict checks and safer behavior. But when people add helpers, they are clearly aware which argument will be modified and which will not. IMHO I do trust that the developers and the reviewers can choose the right type at the review time. :) > > > > So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM > > is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is > > only used in bpf_iter prog as the type of key, which hasn't been > > used in the affected helper functions. PTR_TO_RDONLY_MEM currently > > has no consumers. > > > > Signed-off-by: Hao Luo <haoluo@google.com> > > --- > > Changes since v1: > > - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate > > read-only and read-write mem arg types. > > > > include/linux/bpf.h | 9 +++++++++ > > kernel/bpf/cgroup.c | 2 +- > > kernel/bpf/helpers.c | 2 +- > > kernel/bpf/verifier.c | 18 ++++++++++++++++++ > > kernel/trace/bpf_trace.c | 6 +++--- > > net/core/filter.c | 6 +++--- > > 6 files changed, 35 insertions(+), 8 deletions(-) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index 7b47e8f344cb..586ce67d63a9 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -341,6 +341,15 @@ enum bpf_arg_type { > > ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */ > > ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */ > > ARG_PTR_TO_TIMER, /* pointer to bpf_timer */ > > + ARG_PTR_TO_WRITABLE_MEM, /* pointer to valid memory. Compared to > > + * ARG_PTR_TO_MEM, this arg_type is not > > + * compatible with RDONLY memory. If the > > + * argument may be updated by the helper, > > + * use this type. > > + */ > > + ARG_PTR_TO_WRITABLE_MEM_OR_NULL, /* pointer to memory or null, similar to > > + * ARG_PTR_TO_WRITABLE_MEM. > > + */ > > __BPF_ARG_TYPE_MAX, > > }; > > > > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c > > index 03145d45e3d5..683269b7cd92 100644 > > --- a/kernel/bpf/cgroup.c > > +++ b/kernel/bpf/cgroup.c > > @@ -1666,7 +1666,7 @@ static const struct bpf_func_proto bpf_sysctl_get_name_proto = { > > .gpl_only = false, > > .ret_type = RET_INTEGER, > > .arg1_type = ARG_PTR_TO_CTX, > > - .arg2_type = ARG_PTR_TO_MEM, > > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > > .arg3_type = ARG_CONST_SIZE, > > .arg4_type = ARG_ANYTHING, > > }; > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > > index 14531757087f..db98385ee7af 100644 > > --- a/kernel/bpf/helpers.c > > +++ b/kernel/bpf/helpers.c > > @@ -1008,7 +1008,7 @@ const struct bpf_func_proto bpf_snprintf_proto = { > > .func = bpf_snprintf, > > .gpl_only = true, > > .ret_type = RET_INTEGER, > > - .arg1_type = ARG_PTR_TO_MEM_OR_NULL, > > + .arg1_type = ARG_PTR_TO_WRITABLE_MEM_OR_NULL, > > .arg2_type = ARG_CONST_SIZE_OR_ZERO, > > .arg3_type = ARG_PTR_TO_CONST_STR, > > .arg4_type = ARG_PTR_TO_MEM_OR_NULL, > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index ae3ff297240e..d8817c3d990e 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -486,6 +486,7 @@ static bool arg_type_may_be_null(enum bpf_arg_type type) > > type == ARG_PTR_TO_CTX_OR_NULL || > > type == ARG_PTR_TO_SOCKET_OR_NULL || > > type == ARG_PTR_TO_ALLOC_MEM_OR_NULL || > > + type == ARG_PTR_TO_WRITABLE_MEM_OR_NULL || > > type == ARG_PTR_TO_STACK_OR_NULL; > > } > > > > @@ -4971,6 +4972,8 @@ static bool arg_type_is_mem_ptr(enum bpf_arg_type type) > > { > > return type == ARG_PTR_TO_MEM || > > type == ARG_PTR_TO_MEM_OR_NULL || > > + type == ARG_PTR_TO_WRITABLE_MEM || > > + type == ARG_PTR_TO_WRITABLE_MEM_OR_NULL || > > type == ARG_PTR_TO_UNINIT_MEM; > > } > > > > @@ -5075,6 +5078,19 @@ static const struct bpf_reg_types mem_types = { > > PTR_TO_MEM, > > PTR_TO_RDONLY_BUF, > > PTR_TO_RDWR_BUF, > > + PTR_TO_RDONLY_MEM, > > + }, > > +}; > > + > > +static const struct bpf_reg_types writable_mem_types = { > > + .types = { > > + PTR_TO_STACK, > > + PTR_TO_PACKET, > > + PTR_TO_PACKET_META, > > + PTR_TO_MAP_KEY, > > + PTR_TO_MAP_VALUE, > > + PTR_TO_MEM, > > + PTR_TO_RDWR_BUF, > > }, > > }; > > > > @@ -5125,6 +5141,8 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { > > [ARG_PTR_TO_UNINIT_MEM] = &mem_types, > > [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types, > > [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, > > + [ARG_PTR_TO_WRITABLE_MEM] = &writable_mem_types, > > + [ARG_PTR_TO_WRITABLE_MEM_OR_NULL] = &writable_mem_types, > > [ARG_PTR_TO_INT] = &int_ptr_types, > > [ARG_PTR_TO_LONG] = &int_ptr_types, > > [ARG_PTR_TO_PERCPU_BTF_ID] = &percpu_btf_ptr_types, > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index cbcd0d6fca7c..5815845222de 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -945,7 +945,7 @@ static const struct bpf_func_proto bpf_d_path_proto = { > > .ret_type = RET_INTEGER, > > .arg1_type = ARG_PTR_TO_BTF_ID, > > .arg1_btf_id = &bpf_d_path_btf_ids[0], > > - .arg2_type = ARG_PTR_TO_MEM, > > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > > .arg3_type = ARG_CONST_SIZE_OR_ZERO, > > .allowed = bpf_d_path_allowed, > > }; > > @@ -1002,7 +1002,7 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = { > > .func = bpf_snprintf_btf, > > .gpl_only = false, > > .ret_type = RET_INTEGER, > > - .arg1_type = ARG_PTR_TO_MEM, > > + .arg1_type = ARG_PTR_TO_WRITABLE_MEM, > > .arg2_type = ARG_CONST_SIZE, > > .arg3_type = ARG_PTR_TO_MEM, > > .arg4_type = ARG_CONST_SIZE, > > @@ -1433,7 +1433,7 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = { > > .gpl_only = true, > > .ret_type = RET_INTEGER, > > .arg1_type = ARG_PTR_TO_CTX, > > - .arg2_type = ARG_PTR_TO_MEM_OR_NULL, > > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM_OR_NULL, > > .arg3_type = ARG_CONST_SIZE_OR_ZERO, > > .arg4_type = ARG_ANYTHING, > > }; > > diff --git a/net/core/filter.c b/net/core/filter.c > > index 8e8d3b49c297..7dadabe12ceb 100644 > > --- a/net/core/filter.c > > +++ b/net/core/filter.c > > @@ -5639,7 +5639,7 @@ static const struct bpf_func_proto bpf_xdp_fib_lookup_proto = { > > .gpl_only = true, > > .ret_type = RET_INTEGER, > > .arg1_type = ARG_PTR_TO_CTX, > > - .arg2_type = ARG_PTR_TO_MEM, > > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > > .arg3_type = ARG_CONST_SIZE, > > .arg4_type = ARG_ANYTHING, > > }; > > @@ -5694,7 +5694,7 @@ static const struct bpf_func_proto bpf_skb_fib_lookup_proto = { > > .gpl_only = true, > > .ret_type = RET_INTEGER, > > .arg1_type = ARG_PTR_TO_CTX, > > - .arg2_type = ARG_PTR_TO_MEM, > > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > > .arg3_type = ARG_CONST_SIZE, > > .arg4_type = ARG_ANYTHING, > > }; > > @@ -6977,7 +6977,7 @@ static const struct bpf_func_proto bpf_sock_ops_load_hdr_opt_proto = { > > .gpl_only = false, > > .ret_type = RET_INTEGER, > > .arg1_type = ARG_PTR_TO_CTX, > > - .arg2_type = ARG_PTR_TO_MEM, > > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > > .arg3_type = ARG_CONST_SIZE, > > .arg4_type = ARG_ANYTHING, > > }; > > -- > > 2.33.0.1079.g6e70778dc9-goog > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM 2021-10-26 17:51 ` Hao Luo @ 2021-10-26 18:53 ` Andrii Nakryiko 2021-10-26 20:00 ` Hao Luo 0 siblings, 1 reply; 15+ messages in thread From: Andrii Nakryiko @ 2021-10-26 18:53 UTC (permalink / raw) To: Hao Luo Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, KP Singh, bpf On Tue, Oct 26, 2021 at 10:51 AM Hao Luo <haoluo@google.com> wrote: > > On Mon, Oct 25, 2021 at 10:06 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Mon, Oct 25, 2021 at 4:13 PM Hao Luo <haoluo@google.com> wrote: > > > > > > Some helper functions may modify its arguments, for example, > > > bpf_d_path, bpf_get_stack etc. Previously, their argument types > > > were marked as ARG_PTR_TO_MEM, which is compatible with read-only > > > mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate > > > to modify a read-only memory by passing it into one of such helper > > > functions. > > > > > > This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to > > > annotate the arguments that may be modified by the helpers. For > > > arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type, > > > while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not > > > acceptable. > > > > > > In short, when a helper may modify its input parameter, use > > > ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM. > > > > This is inconsistent with the other uses where we have something > > that's writable by default and mark it as RDONLY if it's read-only. > > Same here, why not keep ARG_PTR_TO_MEM to mean "writable memory", and > > add ARG_PTR_TO_RDONLY_MEM for helpers that are not writing into the > > memory? It will also be safer default: if helper defines > > ARG_PTR_TO_MEM but never writes to it, worst thing that can happen > > would be that you won't be able to pass REG_PTR_TO_RDONLY_MEM into it > > without fixing helper definition. The other way is more dangerous. If > > ARG_PTR_TO_MEM means read-only mem and helper forgot this distinction > > and actually writes into the memory, then we are in much bigger > > trouble. > > > > My original implementation was adding ARG_PTR_TO_RDONLY_MEM. But I > find it's not intuitive for developers when adding helpers. The > majority of PTR_TO_MEM arguments are read-only. When adding a new > helper, I would expect the default arg type (that is, ARG_PTR_TO_MEM) > to match the default case (that is, read-only argument). Requiring > explicitly adding RDONLY to most cases seems a little unintuitive to > me. > > But other than that, I agree that narrowing ARG_PTR_TO_MEM down to > writable memory fosters more strict checks and safer behavior. But > when people add helpers, they are clearly aware which argument will be > modified and which will not. IMHO I do trust that the developers and > the reviewers can choose the right type at the review time. :) I don't trust myself, and neither should you :) See 5b029a32cfe4 ("bpf: Fix ringbuf helper function compatibility") as an example of the things that shouldn't have happened, but slipped through my own testing and code review anyway. And there were multiple cases where we accidentally enabled stuff that we shouldn't or didn't check something that should have been prevented. All that is to say that if we can have safer behavior by default (not as enforced by humans), then it's better. In this sense, ARG_PTR_TO_MEM meaning writable access is safer, because even if we accidentally forget to mark some input as ARG_PTR_TO_RDONLY_MEM, worst thing is that users won't be able to use helper in some situation and hopefully will report this and we'll fix it. The alternative is that a helper declares the argument as read-only memory (accidentally, because it's shorter enum and sort of default), but actually does the write to that memory. Now we have a much bigger issue. > > > > > > > So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM > > > is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is > > > only used in bpf_iter prog as the type of key, which hasn't been > > > used in the affected helper functions. PTR_TO_RDONLY_MEM currently > > > has no consumers. > > > > > > Signed-off-by: Hao Luo <haoluo@google.com> > > > --- > > > Changes since v1: > > > - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate > > > read-only and read-write mem arg types. > > > [...] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM 2021-10-26 18:53 ` Andrii Nakryiko @ 2021-10-26 20:00 ` Hao Luo 0 siblings, 0 replies; 15+ messages in thread From: Hao Luo @ 2021-10-26 20:00 UTC (permalink / raw) To: Andrii Nakryiko Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, KP Singh, bpf On Tue, Oct 26, 2021 at 11:53 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Oct 26, 2021 at 10:51 AM Hao Luo <haoluo@google.com> wrote: > > > > On Mon, Oct 25, 2021 at 10:06 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Mon, Oct 25, 2021 at 4:13 PM Hao Luo <haoluo@google.com> wrote: > > > > > > > > Some helper functions may modify its arguments, for example, > > > > bpf_d_path, bpf_get_stack etc. Previously, their argument types > > > > were marked as ARG_PTR_TO_MEM, which is compatible with read-only > > > > mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate > > > > to modify a read-only memory by passing it into one of such helper > > > > functions. > > > > > > > > This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to > > > > annotate the arguments that may be modified by the helpers. For > > > > arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type, > > > > while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not > > > > acceptable. > > > > > > > > In short, when a helper may modify its input parameter, use > > > > ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM. > > > > > > This is inconsistent with the other uses where we have something > > > that's writable by default and mark it as RDONLY if it's read-only. > > > Same here, why not keep ARG_PTR_TO_MEM to mean "writable memory", and > > > add ARG_PTR_TO_RDONLY_MEM for helpers that are not writing into the > > > memory? It will also be safer default: if helper defines > > > ARG_PTR_TO_MEM but never writes to it, worst thing that can happen > > > would be that you won't be able to pass REG_PTR_TO_RDONLY_MEM into it > > > without fixing helper definition. The other way is more dangerous. If > > > ARG_PTR_TO_MEM means read-only mem and helper forgot this distinction > > > and actually writes into the memory, then we are in much bigger > > > trouble. > > > > > > > My original implementation was adding ARG_PTR_TO_RDONLY_MEM. But I > > find it's not intuitive for developers when adding helpers. The > > majority of PTR_TO_MEM arguments are read-only. When adding a new > > helper, I would expect the default arg type (that is, ARG_PTR_TO_MEM) > > to match the default case (that is, read-only argument). Requiring > > explicitly adding RDONLY to most cases seems a little unintuitive to > > me. > > > > But other than that, I agree that narrowing ARG_PTR_TO_MEM down to > > writable memory fosters more strict checks and safer behavior. But > > when people add helpers, they are clearly aware which argument will be > > modified and which will not. IMHO I do trust that the developers and > > the reviewers can choose the right type at the review time. :) > > I don't trust myself, and neither should you :) See 5b029a32cfe4 > ("bpf: Fix ringbuf helper function compatibility") as an example of > the things that shouldn't have happened, but slipped through my own > testing and code review anyway. And there were multiple cases where we > accidentally enabled stuff that we shouldn't or didn't check something > that should have been prevented. > > All that is to say that if we can have safer behavior by default (not > as enforced by humans), then it's better. In this sense, > ARG_PTR_TO_MEM meaning writable access is safer, because even if we > accidentally forget to mark some input as ARG_PTR_TO_RDONLY_MEM, worst > thing is that users won't be able to use helper in some situation and > hopefully will report this and we'll fix it. The alternative is that a > helper declares the argument as read-only memory (accidentally, > because it's shorter enum and sort of default), but actually does the > write to that memory. Now we have a much bigger issue. > Acknowledged. Will make this change in this next iteration. > > > > > > > > > > So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM > > > > is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is > > > > only used in bpf_iter prog as the type of key, which hasn't been > > > > used in the affected helper functions. PTR_TO_RDONLY_MEM currently > > > > has no consumers. > > > > > > > > Signed-off-by: Hao Luo <haoluo@google.com> > > > > --- > > > > Changes since v1: > > > > - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate > > > > read-only and read-write mem arg types. > > > > > > [...] ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH bpf-next v2 3/3] bpf/selftests: Test PTR_TO_RDONLY_MEM 2021-10-25 23:12 [PATCH bpf-next v2 0/3] bpf: Prevent writing read-only memory Hao Luo 2021-10-25 23:12 ` [PATCH bpf-next 1/3] bpf: Prevent write to ksym memory Hao Luo 2021-10-25 23:12 ` [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM Hao Luo @ 2021-10-25 23:12 ` Hao Luo 2 siblings, 0 replies; 15+ messages in thread From: Hao Luo @ 2021-10-25 23:12 UTC (permalink / raw) To: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann Cc: KP Singh, bpf, Hao Luo This test verifies that a ksym of non-struct can not be directly updated. Signed-off-by: Hao Luo <haoluo@google.com> --- Changes since v1: - Replaced CHECK() with ASSERT_ERR_PTR() - Commented in the test for the reason of verification failure. .../selftests/bpf/prog_tests/ksyms_btf.c | 14 +++++++++ .../bpf/progs/test_ksyms_btf_write_check.c | 29 +++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 tools/testing/selftests/bpf/progs/test_ksyms_btf_write_check.c diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c b/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c index cf3acfa5a91d..69455fe90ac3 100644 --- a/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c +++ b/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c @@ -7,6 +7,7 @@ #include "test_ksyms_btf.skel.h" #include "test_ksyms_btf_null_check.skel.h" #include "test_ksyms_weak.skel.h" +#include "test_ksyms_btf_write_check.skel.h" static int duration; @@ -109,6 +110,16 @@ static void test_weak_syms(void) test_ksyms_weak__destroy(skel); } +static void test_write_check(void) +{ + struct test_ksyms_btf_write_check *skel; + + skel = test_ksyms_btf_write_check__open_and_load(); + ASSERT_ERR_PTR(skel, "unexpected load of a prog writing to ksym memory\n"); + + test_ksyms_btf_write_check__destroy(skel); +} + void test_ksyms_btf(void) { int percpu_datasec; @@ -136,4 +147,7 @@ void test_ksyms_btf(void) if (test__start_subtest("weak_ksyms")) test_weak_syms(); + + if (test__start_subtest("write_check")) + test_write_check(); } diff --git a/tools/testing/selftests/bpf/progs/test_ksyms_btf_write_check.c b/tools/testing/selftests/bpf/progs/test_ksyms_btf_write_check.c new file mode 100644 index 000000000000..2180c41cd890 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_ksyms_btf_write_check.c @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2021 Google */ + +#include "vmlinux.h" + +#include <bpf/bpf_helpers.h> + +extern const int bpf_prog_active __ksym; /* int type global var. */ + +SEC("raw_tp/sys_enter") +int handler(const void *ctx) +{ + int *active; + __u32 cpu; + + cpu = bpf_get_smp_processor_id(); + active = (int *)bpf_per_cpu_ptr(&bpf_prog_active, cpu); + if (active) { + /* Kernel memory obtained from bpf_{per,this}_cpu_ptr + * is read-only, should _not_ pass verification. + */ + /* WRITE_ONCE */ + *(volatile int *)active = -1; + } + + return 0; +} + +char _license[] SEC("license") = "GPL"; -- 2.33.0.1079.g6e70778dc9-goog ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-10-26 21:24 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-25 23:12 [PATCH bpf-next v2 0/3] bpf: Prevent writing read-only memory Hao Luo 2021-10-25 23:12 ` [PATCH bpf-next 1/3] bpf: Prevent write to ksym memory Hao Luo 2021-10-25 23:12 ` [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM Hao Luo 2021-10-26 3:48 ` Alexei Starovoitov 2021-10-26 5:14 ` Andrii Nakryiko 2021-10-26 17:59 ` Alexei Starovoitov 2021-10-26 18:13 ` Hao Luo 2021-10-26 18:44 ` Andrii Nakryiko 2021-10-26 19:22 ` Alexei Starovoitov 2021-10-26 21:24 ` Andrii Nakryiko 2021-10-26 5:06 ` Andrii Nakryiko 2021-10-26 17:51 ` Hao Luo 2021-10-26 18:53 ` Andrii Nakryiko 2021-10-26 20:00 ` Hao Luo 2021-10-25 23:12 ` [PATCH bpf-next v2 3/3] bpf/selftests: Test PTR_TO_RDONLY_MEM Hao Luo
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.