* [PATCH bpf-next 1/8] selftests/bpf: fix test_loader check message
2024-01-05 0:09 [PATCH bpf-next 0/8] PTR_TO_BTF_ID arguments in global subprogs Andrii Nakryiko
@ 2024-01-05 0:09 ` Andrii Nakryiko
2024-01-05 0:09 ` [PATCH bpf-next 2/8] bpf: make sure scalar args don't accept __arg_nonnull tag Andrii Nakryiko
` (8 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2024-01-05 0:09 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Dave Marchevsky
Seeing:
process_subtest:PASS:Can't alloc specs array 0 nsec
... in verbose successful test log is very confusing. Use smaller
identifier-like test tag to denote that we are asserting specs array
allocation success.
Now it's much less distracting:
process_subtest:PASS:specs_alloc 0 nsec
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/testing/selftests/bpf/test_loader.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index f01391021218..72906d27babf 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -688,7 +688,7 @@ static void process_subtest(struct test_loader *tester,
++nr_progs;
specs = calloc(nr_progs, sizeof(struct test_spec));
- if (!ASSERT_OK_PTR(specs, "Can't alloc specs array"))
+ if (!ASSERT_OK_PTR(specs, "specs_alloc"))
return;
i = 0;
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH bpf-next 2/8] bpf: make sure scalar args don't accept __arg_nonnull tag
2024-01-05 0:09 [PATCH bpf-next 0/8] PTR_TO_BTF_ID arguments in global subprogs Andrii Nakryiko
2024-01-05 0:09 ` [PATCH bpf-next 1/8] selftests/bpf: fix test_loader check message Andrii Nakryiko
@ 2024-01-05 0:09 ` Andrii Nakryiko
2024-01-05 0:09 ` [PATCH bpf-next 3/8] bpf: prepare btf_prepare_func_args() for multiple tags per argument Andrii Nakryiko
` (7 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2024-01-05 0:09 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Dave Marchevsky
Move scalar arg processing in btf_prepare_func_args() after all pointer
arg processing is done. This makes it easier to do validation. One
example of unintended behavior right now is ability to specify
__arg_nonnull for integer/enum arguments. This patch fixes this.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
kernel/bpf/btf.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 51e8b4bee0c8..47163cb28b83 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6896,10 +6896,6 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
while (btf_type_is_modifier(t))
t = btf_type_by_id(btf, t->type);
- if (btf_type_is_int(t) || btf_is_any_enum(t)) {
- sub->args[i].arg_type = ARG_ANYTHING;
- continue;
- }
if (btf_type_is_ptr(t) && btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
sub->args[i].arg_type = ARG_PTR_TO_CTX;
continue;
@@ -6929,6 +6925,10 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
bpf_log(log, "arg#%d marked as non-null, but is not a pointer type\n", i);
return -EINVAL;
}
+ if (btf_type_is_int(t) || btf_is_any_enum(t)) {
+ sub->args[i].arg_type = ARG_ANYTHING;
+ continue;
+ }
bpf_log(log, "Arg#%d type %s in %s() is not supported yet.\n",
i, btf_type_str(t), tname);
return -EINVAL;
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH bpf-next 3/8] bpf: prepare btf_prepare_func_args() for multiple tags per argument
2024-01-05 0:09 [PATCH bpf-next 0/8] PTR_TO_BTF_ID arguments in global subprogs Andrii Nakryiko
2024-01-05 0:09 ` [PATCH bpf-next 1/8] selftests/bpf: fix test_loader check message Andrii Nakryiko
2024-01-05 0:09 ` [PATCH bpf-next 2/8] bpf: make sure scalar args don't accept __arg_nonnull tag Andrii Nakryiko
@ 2024-01-05 0:09 ` Andrii Nakryiko
2024-01-05 0:09 ` [PATCH bpf-next 4/8] bpf: support " Andrii Nakryiko
` (6 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2024-01-05 0:09 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Dave Marchevsky
Add btf_arg_tag flags enum to be able to record multiple tags per
argument. Also streamline pointer argument processing some more.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
kernel/bpf/btf.c | 53 ++++++++++++++++++++++++++++++-------------
kernel/bpf/verifier.c | 1 -
2 files changed, 37 insertions(+), 17 deletions(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 47163cb28b83..ccaf57e755fc 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6784,6 +6784,11 @@ static bool btf_is_dynptr_ptr(const struct btf *btf, const struct btf_type *t)
return false;
}
+enum btf_arg_tag {
+ ARG_TAG_CTX = 0x1,
+ ARG_TAG_NONNULL = 0x2,
+};
+
/* Process BTF of a function to produce high-level expectation of function
* arguments (like ARG_PTR_TO_CTX, or ARG_PTR_TO_MEM, etc). This information
* is cached in subprog info for reuse.
@@ -6865,10 +6870,8 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
* Only PTR_TO_CTX and SCALAR are supported atm.
*/
for (i = 0; i < nargs; i++) {
- bool is_nonnull = false;
const char *tag;
-
- t = btf_type_by_id(btf, args[i].type);
+ u32 tags = 0;
tag = btf_find_decl_tag_value(btf, fn_t, i, "arg:");
if (IS_ERR(tag) && PTR_ERR(tag) == -ENOENT) {
@@ -6886,43 +6889,61 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
bpf_log(log, "arg#%d type tag is not supported in static functions\n", i);
return -EOPNOTSUPP;
}
+
if (strcmp(tag, "ctx") == 0) {
- sub->args[i].arg_type = ARG_PTR_TO_CTX;
- continue;
+ tags |= ARG_TAG_CTX;
+ } else if (strcmp(tag, "nonnull") == 0) {
+ tags |= ARG_TAG_NONNULL;
+ } else {
+ bpf_log(log, "arg#%d has unsupported set of tags\n", i);
+ return -EOPNOTSUPP;
}
- if (strcmp(tag, "nonnull") == 0)
- is_nonnull = true;
}
+ t = btf_type_by_id(btf, args[i].type);
while (btf_type_is_modifier(t))
t = btf_type_by_id(btf, t->type);
- if (btf_type_is_ptr(t) && btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
+ if (!btf_type_is_ptr(t))
+ goto skip_pointer;
+
+ if ((tags & ARG_TAG_CTX) || btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
+ if (tags & ~ARG_TAG_CTX) {
+ bpf_log(log, "arg#%d has invalid combination of tags\n", i);
+ return -EINVAL;
+ }
sub->args[i].arg_type = ARG_PTR_TO_CTX;
continue;
}
- if (btf_type_is_ptr(t) && btf_is_dynptr_ptr(btf, t)) {
+ if (btf_is_dynptr_ptr(btf, t)) {
+ if (tags) {
+ bpf_log(log, "arg#%d has invalid combination of tags\n", i);
+ return -EINVAL;
+ }
sub->args[i].arg_type = ARG_PTR_TO_DYNPTR | MEM_RDONLY;
continue;
}
- if (is_global && btf_type_is_ptr(t)) {
+ if (is_global) { /* generic user data pointer */
u32 mem_size;
t = btf_type_skip_modifiers(btf, t->type, NULL);
ref_t = btf_resolve_size(btf, t, &mem_size);
if (IS_ERR(ref_t)) {
- bpf_log(log,
- "arg#%d reference type('%s %s') size cannot be determined: %ld\n",
- i, btf_type_str(t), btf_name_by_offset(btf, t->name_off),
+ bpf_log(log, "arg#%d reference type('%s %s') size cannot be determined: %ld\n",
+ i, btf_type_str(t), btf_name_by_offset(btf, t->name_off),
PTR_ERR(ref_t));
return -EINVAL;
}
- sub->args[i].arg_type = is_nonnull ? ARG_PTR_TO_MEM : ARG_PTR_TO_MEM_OR_NULL;
+ sub->args[i].arg_type = ARG_PTR_TO_MEM | PTR_MAYBE_NULL;
+ if (tags & ARG_TAG_NONNULL)
+ sub->args[i].arg_type &= ~PTR_MAYBE_NULL;
sub->args[i].mem_size = mem_size;
continue;
}
- if (is_nonnull) {
- bpf_log(log, "arg#%d marked as non-null, but is not a pointer type\n", i);
+
+skip_pointer:
+ if (tags) {
+ bpf_log(log, "arg#%d has pointer tag, but is not a pointer type\n", i);
return -EINVAL;
}
if (btf_type_is_int(t) || btf_is_any_enum(t)) {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d5f4ff1eb235..271c82bf9697 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20037,7 +20037,6 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
state->first_insn_idx = env->subprog_info[subprog].start;
state->last_insn_idx = -1;
-
regs = state->frame[state->curframe]->regs;
if (subprog || env->prog->type == BPF_PROG_TYPE_EXT) {
const char *sub_name = subprog_name(env, subprog);
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH bpf-next 4/8] bpf: support multiple tags per argument
2024-01-05 0:09 [PATCH bpf-next 0/8] PTR_TO_BTF_ID arguments in global subprogs Andrii Nakryiko
` (2 preceding siblings ...)
2024-01-05 0:09 ` [PATCH bpf-next 3/8] bpf: prepare btf_prepare_func_args() for multiple tags per argument Andrii Nakryiko
@ 2024-01-05 0:09 ` Andrii Nakryiko
2024-01-05 0:09 ` [PATCH bpf-next 5/8] bpf: add __arg_trusted and __arg_untrusted global func tags Andrii Nakryiko
` (5 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2024-01-05 0:09 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Dave Marchevsky
Add ability to iterate multiple decl_tag types pointed to the same
function argument. Use this to support multiple __arg_xxx tags per
global subprog argument.
We leave btf_find_decl_tag_value() intact, but change its implementation
to use a new btf_find_next_decl_tag() which can be straightforwardly
used to find next BTF type ID of a matching btf_decl_tag type.
btf_prepare_func_args() is switched from btf_find_decl_tag_value() to
btf_find_next_decl_tag() to gain multiple tags per argument support.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
include/linux/bpf.h | 2 ++
kernel/bpf/btf.c | 64 +++++++++++++++++++++++++++++----------------
2 files changed, 43 insertions(+), 23 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 7671530d6e4e..7ab4a290418a 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2471,6 +2471,8 @@ int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *pr
struct btf *btf, const struct btf_type *t);
const char *btf_find_decl_tag_value(const struct btf *btf, const struct btf_type *pt,
int comp_idx, const char *tag_key);
+int btf_find_next_decl_tag(const struct btf *btf, const struct btf_type *pt,
+ int comp_idx, const char *tag_key, int last_id);
struct bpf_prog *bpf_prog_by_id(u32 id);
struct bpf_link *bpf_link_by_id(u32 id);
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index ccaf57e755fc..368d8fe19d35 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3310,30 +3310,48 @@ static int btf_find_kptr(const struct btf *btf, const struct btf_type *t,
return BTF_FIELD_FOUND;
}
-const char *btf_find_decl_tag_value(const struct btf *btf, const struct btf_type *pt,
- int comp_idx, const char *tag_key)
+int btf_find_next_decl_tag(const struct btf *btf, const struct btf_type *pt,
+ int comp_idx, const char *tag_key, int last_id)
{
- const char *value = NULL;
- int i;
+ int len = strlen(tag_key);
+ int i, n;
- for (i = 1; i < btf_nr_types(btf); i++) {
+ for (i = last_id + 1, n = btf_nr_types(btf); i < n; i++) {
const struct btf_type *t = btf_type_by_id(btf, i);
- int len = strlen(tag_key);
if (!btf_type_is_decl_tag(t))
continue;
- if (pt != btf_type_by_id(btf, t->type) ||
- btf_type_decl_tag(t)->component_idx != comp_idx)
+ if (pt != btf_type_by_id(btf, t->type))
+ continue;
+ if (btf_type_decl_tag(t)->component_idx != comp_idx)
continue;
if (strncmp(__btf_name_by_offset(btf, t->name_off), tag_key, len))
continue;
- /* Prevent duplicate entries for same type */
- if (value)
- return ERR_PTR(-EEXIST);
- value = __btf_name_by_offset(btf, t->name_off) + len;
+ return i;
}
- if (!value)
- return ERR_PTR(-ENOENT);
+ return -ENOENT;
+}
+
+const char *btf_find_decl_tag_value(const struct btf *btf, const struct btf_type *pt,
+ int comp_idx, const char *tag_key)
+{
+ const char *value = NULL;
+ const struct btf_type *t;
+ int len, id;
+
+ id = btf_find_next_decl_tag(btf, pt, comp_idx, tag_key, 0);
+ if (id < 0)
+ return ERR_PTR(id);
+
+ t = btf_type_by_id(btf, id);
+ len = strlen(tag_key);
+ value = __btf_name_by_offset(btf, t->name_off) + len;
+
+ /* Prevent duplicate entries for same type */
+ id = btf_find_next_decl_tag(btf, pt, comp_idx, tag_key, id);
+ if (id >= 0)
+ return ERR_PTR(-EEXIST);
+
return value;
}
@@ -6870,20 +6888,16 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
* Only PTR_TO_CTX and SCALAR are supported atm.
*/
for (i = 0; i < nargs; i++) {
- const char *tag;
u32 tags = 0;
+ int id = 0;
- tag = btf_find_decl_tag_value(btf, fn_t, i, "arg:");
- if (IS_ERR(tag) && PTR_ERR(tag) == -ENOENT) {
- tag = NULL;
- } else if (IS_ERR(tag)) {
- bpf_log(log, "arg#%d type's tag fetching failure: %ld\n", i, PTR_ERR(tag));
- return PTR_ERR(tag);
- }
/* 'arg:<tag>' decl_tag takes precedence over derivation of
* register type from BTF type itself
*/
- if (tag) {
+ while ((id = btf_find_next_decl_tag(btf, fn_t, i, "arg:", id)) > 0) {
+ const struct btf_type *tag_t = btf_type_by_id(btf, id);
+ const char *tag = __btf_name_by_offset(btf, tag_t->name_off) + 4;
+
/* disallow arg tags in static subprogs */
if (!is_global) {
bpf_log(log, "arg#%d type tag is not supported in static functions\n", i);
@@ -6899,6 +6913,10 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
return -EOPNOTSUPP;
}
}
+ if (id != -ENOENT) {
+ bpf_log(log, "arg#%d type tag fetching failure: %d\n", i, id);
+ return id;
+ }
t = btf_type_by_id(btf, args[i].type);
while (btf_type_is_modifier(t))
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH bpf-next 5/8] bpf: add __arg_trusted and __arg_untrusted global func tags
2024-01-05 0:09 [PATCH bpf-next 0/8] PTR_TO_BTF_ID arguments in global subprogs Andrii Nakryiko
` (3 preceding siblings ...)
2024-01-05 0:09 ` [PATCH bpf-next 4/8] bpf: support " Andrii Nakryiko
@ 2024-01-05 0:09 ` Andrii Nakryiko
2024-01-12 2:05 ` Alexei Starovoitov
2024-01-05 0:09 ` [PATCH bpf-next 6/8] libbpf: add __arg_trusted and __arg_untrusted annotation macros Andrii Nakryiko
` (4 subsequent siblings)
9 siblings, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2024-01-05 0:09 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Dave Marchevsky
Add support for passing PTR_TO_BTF_ID registers to global subprogs.
Currently two flavors are supported: trusted and untrusted. In both
cases we assume _OR_NULL variants, so user would be forced to do a NULL
check. __arg_nonnull can be used in conjunction with
__arg_{trusted,untrusted} annotation to avoid unnecessary NULL checks,
and subprog caller will be forced to provided known non-NULL pointers.
Alternatively, we can add __arg_nullable and change default to be
non-_OR_NULL, and __arg_nullable will allow to force NULL checks, if
necessary. This probably would be a better usability overall, so let's
discuss this.
Note, we disallow global subprogs to destroy passed in PTR_TO_BTF_ID
arguments, even the trusted one. We achieve that by not setting
ref_obj_id when validating subprog code. This basically enforces (in
Rust terms) borrowing semantics vs move semantics. Borrowing semantics
seems to be a better fit for isolated global subprog validation
approach.
Implementation-wise, we utilize existing logic for matching
user-provided BTF type to kernel-side BTF type, used by BPF CO-RE logic
and following same matching rules. We enforce a unique match for types.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
include/linux/bpf_verifier.h | 1 +
kernel/bpf/btf.c | 103 +++++++++++++++---
kernel/bpf/verifier.c | 31 ++++++
.../bpf/progs/nested_trust_failure.c | 2 +-
4 files changed, 123 insertions(+), 14 deletions(-)
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index d07d857ca67f..eaea129c38ff 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -610,6 +610,7 @@ struct bpf_subprog_arg_info {
enum bpf_arg_type arg_type;
union {
u32 mem_size;
+ u32 btf_id;
};
};
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 368d8fe19d35..287a0581516e 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6802,9 +6802,78 @@ static bool btf_is_dynptr_ptr(const struct btf *btf, const struct btf_type *t)
return false;
}
+struct bpf_cand_cache {
+ const char *name;
+ u32 name_len;
+ u16 kind;
+ u16 cnt;
+ struct {
+ const struct btf *btf;
+ u32 id;
+ } cands[];
+};
+
+static DEFINE_MUTEX(cand_cache_mutex);
+
+static struct bpf_cand_cache *
+bpf_core_find_cands(struct bpf_core_ctx *ctx, u32 local_type_id);
+
+static int btf_get_ptr_to_btf_id(struct bpf_verifier_log *log, int arg_idx,
+ const struct btf *btf, const struct btf_type *t)
+{
+ struct bpf_cand_cache *cc;
+ struct bpf_core_ctx ctx = {
+ .btf = btf,
+ .log = log,
+ };
+ u32 kern_type_id, type_id;
+ int err = 0;
+
+ /* skip PTR and modifiers */
+ type_id = t->type;
+ t = btf_type_by_id(btf, t->type);
+ while (btf_type_is_modifier(t)) {
+ type_id = t->type;
+ t = btf_type_by_id(btf, t->type);
+ }
+
+ mutex_lock(&cand_cache_mutex);
+ cc = bpf_core_find_cands(&ctx, type_id);
+ if (IS_ERR(cc)) {
+ err = PTR_ERR(cc);
+ bpf_log(log, "arg#%d reference type('%s %s') candidate matching error: %d\n",
+ arg_idx, btf_type_str(t), __btf_name_by_offset(btf, t->name_off),
+ err);
+ goto cand_cache_unlock;
+ }
+ if (cc->cnt != 1) {
+ bpf_log(log, "arg#%d reference type('%s %s') %s\n",
+ arg_idx, btf_type_str(t), __btf_name_by_offset(btf, t->name_off),
+ cc->cnt == 0 ? "has no matches" : "is ambiguous");
+ err = cc->cnt == 0 ? -ENOENT : -ESRCH;
+ goto cand_cache_unlock;
+ }
+ if (strcmp(cc->cands[0].btf->name, "vmlinux") != 0) {
+ bpf_log(log, "arg#%d reference type('%s %s') points to kernel module type (unsupported)\n",
+ arg_idx, btf_type_str(t), __btf_name_by_offset(btf, t->name_off));
+ err = -EOPNOTSUPP;
+ goto cand_cache_unlock;
+ }
+ kern_type_id = cc->cands[0].id;
+
+cand_cache_unlock:
+ mutex_unlock(&cand_cache_mutex);
+ if (err)
+ return err;
+
+ return kern_type_id;
+}
+
enum btf_arg_tag {
ARG_TAG_CTX = 0x1,
ARG_TAG_NONNULL = 0x2,
+ ARG_TAG_TRUSTED = 0x4,
+ ARG_TAG_UNTRUSTED = 0x8,
};
/* Process BTF of a function to produce high-level expectation of function
@@ -6906,6 +6975,10 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
if (strcmp(tag, "ctx") == 0) {
tags |= ARG_TAG_CTX;
+ } else if (strcmp(tag, "trusted") == 0) {
+ tags |= ARG_TAG_TRUSTED;
+ } else if (strcmp(tag, "untrusted") == 0) {
+ tags |= ARG_TAG_UNTRUSTED;
} else if (strcmp(tag, "nonnull") == 0) {
tags |= ARG_TAG_NONNULL;
} else {
@@ -6940,6 +7013,23 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
sub->args[i].arg_type = ARG_PTR_TO_DYNPTR | MEM_RDONLY;
continue;
}
+ if (tags & (ARG_TAG_TRUSTED | ARG_TAG_UNTRUSTED)) {
+ int kern_type_id;
+
+ kern_type_id = btf_get_ptr_to_btf_id(log, i, btf, t);
+ if (kern_type_id < 0)
+ return kern_type_id;
+
+ sub->args[i].arg_type = ARG_PTR_TO_BTF_ID | PTR_UNTRUSTED | PTR_MAYBE_NULL;
+ if (tags & ARG_TAG_TRUSTED) {
+ sub->args[i].arg_type &= ~PTR_UNTRUSTED;
+ sub->args[i].arg_type |= PTR_TRUSTED;
+ }
+ if (tags & ARG_TAG_NONNULL)
+ sub->args[i].arg_type &= ~PTR_MAYBE_NULL;
+ sub->args[i].btf_id = kern_type_id;
+ continue;
+ }
if (is_global) { /* generic user data pointer */
u32 mem_size;
@@ -8042,17 +8132,6 @@ size_t bpf_core_essential_name_len(const char *name)
return n;
}
-struct bpf_cand_cache {
- const char *name;
- u32 name_len;
- u16 kind;
- u16 cnt;
- struct {
- const struct btf *btf;
- u32 id;
- } cands[];
-};
-
static void bpf_free_cands(struct bpf_cand_cache *cands)
{
if (!cands->cnt)
@@ -8073,8 +8152,6 @@ static struct bpf_cand_cache *vmlinux_cand_cache[VMLINUX_CAND_CACHE_SIZE];
#define MODULE_CAND_CACHE_SIZE 31
static struct bpf_cand_cache *module_cand_cache[MODULE_CAND_CACHE_SIZE];
-static DEFINE_MUTEX(cand_cache_mutex);
-
static void __print_cand_cache(struct bpf_verifier_log *log,
struct bpf_cand_cache **cache,
int cache_size)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 271c82bf9697..b43a74b8487b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8076,6 +8076,7 @@ static const struct bpf_reg_types btf_ptr_types = {
PTR_TO_BTF_ID,
PTR_TO_BTF_ID | PTR_TRUSTED,
PTR_TO_BTF_ID | MEM_RCU,
+ PTR_TO_BTF_ID | PTR_UNTRUSTED,
},
};
static const struct bpf_reg_types percpu_btf_ptr_types = {
@@ -8262,6 +8263,12 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
case PTR_TO_BTF_ID | MEM_PERCPU | PTR_TRUSTED:
/* Handled by helper specific checks */
break;
+ case PTR_TO_BTF_ID | PTR_UNTRUSTED:
+ if (!(arg_type & PTR_UNTRUSTED)) {
+ verbose(env, "Passing unexpected untrusted pointer as arg#%d\n", regno);
+ return -EACCES;
+ }
+ break;
default:
verbose(env, "verifier internal error: invalid PTR_TO_BTF_ID register for type match\n");
return -EFAULT;
@@ -9300,6 +9307,18 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
ret = process_dynptr_func(env, regno, -1, arg->arg_type, 0);
if (ret)
return ret;
+ } else if (base_type(arg->arg_type) == ARG_PTR_TO_BTF_ID) {
+ struct bpf_call_arg_meta meta;
+ int err;
+
+ if (register_is_null(reg) && type_may_be_null(arg->arg_type))
+ continue;
+
+ memset(&meta, 0, sizeof(meta)); /* leave func_id as zero */
+ err = check_reg_type(env, regno, arg->arg_type, &arg->btf_id, &meta);
+ err = err ?: check_func_arg_reg_off(env, reg, regno, arg->arg_type);
+ if (err)
+ return err;
} else {
bpf_log(log, "verifier bug: unrecognized arg#%d type %d\n",
i, arg->arg_type);
@@ -20080,6 +20099,18 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
mark_reg_known_zero(env, regs, i);
reg->mem_size = arg->mem_size;
reg->id = ++env->id_gen;
+ } else if (base_type(arg->arg_type) == ARG_PTR_TO_BTF_ID) {
+ reg->type = PTR_TO_BTF_ID;
+ if (arg->arg_type & PTR_MAYBE_NULL)
+ reg->type |= PTR_MAYBE_NULL;
+ if (arg->arg_type & PTR_UNTRUSTED)
+ reg->type |= PTR_UNTRUSTED;
+ if (arg->arg_type & PTR_TRUSTED)
+ reg->type |= PTR_TRUSTED;
+ mark_reg_known_zero(env, regs, i);
+ reg->btf = bpf_get_btf_vmlinux(); /* can't fail at this point */
+ reg->btf_id = arg->btf_id;
+ reg->id = ++env->id_gen;
} else {
WARN_ONCE(1, "BUG: unhandled arg#%d type %d\n",
i - BPF_REG_1, arg->arg_type);
diff --git a/tools/testing/selftests/bpf/progs/nested_trust_failure.c b/tools/testing/selftests/bpf/progs/nested_trust_failure.c
index ea39497f11ed..bb1001a1fb19 100644
--- a/tools/testing/selftests/bpf/progs/nested_trust_failure.c
+++ b/tools/testing/selftests/bpf/progs/nested_trust_failure.c
@@ -41,7 +41,7 @@ int BPF_PROG(test_invalid_nested_offset, struct task_struct *task, u64 clone_fla
/* Although R2 is of type sk_buff but sock_common is expected, we will hit untrusted ptr first. */
SEC("tp_btf/tcp_probe")
-__failure __msg("R2 type=untrusted_ptr_ expected=ptr_, trusted_ptr_, rcu_ptr_")
+__failure __msg("Passing unexpected untrusted pointer as arg#2")
int BPF_PROG(test_invalid_skb_field, struct sock *sk, struct sk_buff *skb)
{
bpf_sk_storage_get(&sk_storage_map, skb->next, 0, 0);
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 5/8] bpf: add __arg_trusted and __arg_untrusted global func tags
2024-01-05 0:09 ` [PATCH bpf-next 5/8] bpf: add __arg_trusted and __arg_untrusted global func tags Andrii Nakryiko
@ 2024-01-12 2:05 ` Alexei Starovoitov
2024-01-12 18:43 ` Andrii Nakryiko
0 siblings, 1 reply; 13+ messages in thread
From: Alexei Starovoitov @ 2024-01-12 2:05 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau,
Kernel Team, Dave Marchevsky
On Thu, Jan 4, 2024 at 4:09 PM Andrii Nakryiko <andrii@kernel.org> wrote:
>
> Add support for passing PTR_TO_BTF_ID registers to global subprogs.
> Currently two flavors are supported: trusted and untrusted. In both
> cases we assume _OR_NULL variants, so user would be forced to do a NULL
> check. __arg_nonnull can be used in conjunction with
> __arg_{trusted,untrusted} annotation to avoid unnecessary NULL checks,
> and subprog caller will be forced to provided known non-NULL pointers.
>
> Alternatively, we can add __arg_nullable and change default to be
> non-_OR_NULL, and __arg_nullable will allow to force NULL checks, if
> necessary. This probably would be a better usability overall, so let's
> discuss this.
Let's make __arg_trusted to be non-null by default.
I think it better matches existing kfuncs with KF_TRUSTED_ARGS.
Also pls use __arg_maybe_null name. "nullable" is difficult
to read and understand. maybe_null matches our internal names too.
Another thought..
may be add __arg_trusted_or_null alias that
will emit two decl tags "arg:trusted", "arg:maybe_null" ?
More below.
> Note, we disallow global subprogs to destroy passed in PTR_TO_BTF_ID
> arguments, even the trusted one. We achieve that by not setting
> ref_obj_id when validating subprog code. This basically enforces (in
> Rust terms) borrowing semantics vs move semantics. Borrowing semantics
> seems to be a better fit for isolated global subprog validation
> approach.
>
> Implementation-wise, we utilize existing logic for matching
> user-provided BTF type to kernel-side BTF type, used by BPF CO-RE logic
> and following same matching rules. We enforce a unique match for types.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
> include/linux/bpf_verifier.h | 1 +
> kernel/bpf/btf.c | 103 +++++++++++++++---
> kernel/bpf/verifier.c | 31 ++++++
> .../bpf/progs/nested_trust_failure.c | 2 +-
> 4 files changed, 123 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index d07d857ca67f..eaea129c38ff 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -610,6 +610,7 @@ struct bpf_subprog_arg_info {
> enum bpf_arg_type arg_type;
> union {
> u32 mem_size;
> + u32 btf_id;
> };
> };
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 368d8fe19d35..287a0581516e 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6802,9 +6802,78 @@ static bool btf_is_dynptr_ptr(const struct btf *btf, const struct btf_type *t)
> return false;
> }
>
> +struct bpf_cand_cache {
> + const char *name;
> + u32 name_len;
> + u16 kind;
> + u16 cnt;
> + struct {
> + const struct btf *btf;
> + u32 id;
> + } cands[];
> +};
> +
> +static DEFINE_MUTEX(cand_cache_mutex);
> +
> +static struct bpf_cand_cache *
> +bpf_core_find_cands(struct bpf_core_ctx *ctx, u32 local_type_id);
> +
> +static int btf_get_ptr_to_btf_id(struct bpf_verifier_log *log, int arg_idx,
> + const struct btf *btf, const struct btf_type *t)
> +{
> + struct bpf_cand_cache *cc;
> + struct bpf_core_ctx ctx = {
> + .btf = btf,
> + .log = log,
> + };
> + u32 kern_type_id, type_id;
> + int err = 0;
> +
> + /* skip PTR and modifiers */
> + type_id = t->type;
> + t = btf_type_by_id(btf, t->type);
> + while (btf_type_is_modifier(t)) {
> + type_id = t->type;
> + t = btf_type_by_id(btf, t->type);
> + }
> +
> + mutex_lock(&cand_cache_mutex);
> + cc = bpf_core_find_cands(&ctx, type_id);
heh. core-in-the-kernel got another use case :)
This allows flavors as well, so
bpf_prog(struct task_struct___v1 *tsk __arg_trusted)
will find the match.
I think it's fine to do. But let's add a test too.
I don't think patch 8 checks that.
> + if (IS_ERR(cc)) {
> + err = PTR_ERR(cc);
> + bpf_log(log, "arg#%d reference type('%s %s') candidate matching error: %d\n",
> + arg_idx, btf_type_str(t), __btf_name_by_offset(btf, t->name_off),
> + err);
> + goto cand_cache_unlock;
> + }
> + if (cc->cnt != 1) {
> + bpf_log(log, "arg#%d reference type('%s %s') %s\n",
> + arg_idx, btf_type_str(t), __btf_name_by_offset(btf, t->name_off),
> + cc->cnt == 0 ? "has no matches" : "is ambiguous");
> + err = cc->cnt == 0 ? -ENOENT : -ESRCH;
> + goto cand_cache_unlock;
> + }
> + if (strcmp(cc->cands[0].btf->name, "vmlinux") != 0) {
use btf_is_module() ?
> + bpf_log(log, "arg#%d reference type('%s %s') points to kernel module type (unsupported)\n",
> + arg_idx, btf_type_str(t), __btf_name_by_offset(btf, t->name_off));
> + err = -EOPNOTSUPP;
> + goto cand_cache_unlock;
> + }
> + kern_type_id = cc->cands[0].id;
> +
> +cand_cache_unlock:
> + mutex_unlock(&cand_cache_mutex);
> + if (err)
> + return err;
> +
> + return kern_type_id;
> +}
> +
> enum btf_arg_tag {
> ARG_TAG_CTX = 0x1,
> ARG_TAG_NONNULL = 0x2,
> + ARG_TAG_TRUSTED = 0x4,
> + ARG_TAG_UNTRUSTED = 0x8,
> };
>
> /* Process BTF of a function to produce high-level expectation of function
> @@ -6906,6 +6975,10 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
>
> if (strcmp(tag, "ctx") == 0) {
> tags |= ARG_TAG_CTX;
> + } else if (strcmp(tag, "trusted") == 0) {
> + tags |= ARG_TAG_TRUSTED;
> + } else if (strcmp(tag, "untrusted") == 0) {
> + tags |= ARG_TAG_UNTRUSTED;
> } else if (strcmp(tag, "nonnull") == 0) {
> tags |= ARG_TAG_NONNULL;
> } else {
> @@ -6940,6 +7013,23 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
> sub->args[i].arg_type = ARG_PTR_TO_DYNPTR | MEM_RDONLY;
> continue;
> }
> + if (tags & (ARG_TAG_TRUSTED | ARG_TAG_UNTRUSTED)) {
> + int kern_type_id;
> +
> + kern_type_id = btf_get_ptr_to_btf_id(log, i, btf, t);
> + if (kern_type_id < 0)
> + return kern_type_id;
> +
> + sub->args[i].arg_type = ARG_PTR_TO_BTF_ID | PTR_UNTRUSTED | PTR_MAYBE_NULL;
PTR_MAYBE_NULL doesn't make sense for untrusted.
It may be zero or -1 or 0xffff the bpf prog may or may not
compare that with NULL or any other value.
It doesn't affect safety and the verifier shouldn't be tracking
null-ness of untrusted pointers. Just to avoid extra code and run-time.
But more importantly...
ARG_PTR_TO_BTF_ID with PTR_UNTRUSTED looks scary to me.
base_type() trims flags and in many places we check
base_type(arg) == ARG_PTR_TO_BTF_ID
the rhs can come from helper defs in .arg1_type too.
A lot of code need to be carefully audited to make sure
we don't accidently introduce a safety issue because
PTR_TO_BTF_ID | PTR_UNTRUSTED was added to btf_ptr_types[].
The handling of it in check_reg_type() looks ok though...
> @@ -8262,6 +8263,12 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> case PTR_TO_BTF_ID | MEM_PERCPU | PTR_TRUSTED:
> /* Handled by helper specific checks */
> break;
> + case PTR_TO_BTF_ID | PTR_UNTRUSTED:
> + if (!(arg_type & PTR_UNTRUSTED)) {
> + verbose(env, "Passing unexpected untrusted pointer as arg#%d\n", regno);
> + return -EACCES;
> + }
both type and arg_type are untrusted, but I need to spend
a ton more time thinking it through.
Maybe avoid adding __arg_untrusted for now?
The patch will be easier to understand. At least for me.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 5/8] bpf: add __arg_trusted and __arg_untrusted global func tags
2024-01-12 2:05 ` Alexei Starovoitov
@ 2024-01-12 18:43 ` Andrii Nakryiko
0 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2024-01-12 18:43 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Kernel Team, Dave Marchevsky
On Thu, Jan 11, 2024 at 6:05 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Jan 4, 2024 at 4:09 PM Andrii Nakryiko <andrii@kernel.org> wrote:
> >
> > Add support for passing PTR_TO_BTF_ID registers to global subprogs.
> > Currently two flavors are supported: trusted and untrusted. In both
> > cases we assume _OR_NULL variants, so user would be forced to do a NULL
> > check. __arg_nonnull can be used in conjunction with
> > __arg_{trusted,untrusted} annotation to avoid unnecessary NULL checks,
> > and subprog caller will be forced to provided known non-NULL pointers.
> >
> > Alternatively, we can add __arg_nullable and change default to be
> > non-_OR_NULL, and __arg_nullable will allow to force NULL checks, if
> > necessary. This probably would be a better usability overall, so let's
> > discuss this.
>
> Let's make __arg_trusted to be non-null by default.
> I think it better matches existing kfuncs with KF_TRUSTED_ARGS.
yep, agreed, already did locally
>
> Also pls use __arg_maybe_null name. "nullable" is difficult
> to read and understand. maybe_null matches our internal names too.
not happy about verbose "maybe_null" naming, tbh, but I don't want to
bikeshed. I like "nullable" because it's a well-known concept across
many languages, so it doesn't seem to be that alien. And it nicely
complements __arg_nonnull.
But I'll rename it because I don't want bikeshedding.
>
> Another thought..
> may be add __arg_trusted_or_null alias that
> will emit two decl tags "arg:trusted", "arg:maybe_null" ?
no, I want to keep them separate. In the future this maybe_null can be
combined with some other tags, I don't want to create all combinations
as macros. I think __arg_trusted __arg_maybe_null is totally fine as
an annotation and doesn't save much in terms of typing if it's
combined into a single macro.
>
> More below.
>
> > Note, we disallow global subprogs to destroy passed in PTR_TO_BTF_ID
> > arguments, even the trusted one. We achieve that by not setting
> > ref_obj_id when validating subprog code. This basically enforces (in
> > Rust terms) borrowing semantics vs move semantics. Borrowing semantics
> > seems to be a better fit for isolated global subprog validation
> > approach.
> >
> > Implementation-wise, we utilize existing logic for matching
> > user-provided BTF type to kernel-side BTF type, used by BPF CO-RE logic
> > and following same matching rules. We enforce a unique match for types.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> > include/linux/bpf_verifier.h | 1 +
> > kernel/bpf/btf.c | 103 +++++++++++++++---
> > kernel/bpf/verifier.c | 31 ++++++
> > .../bpf/progs/nested_trust_failure.c | 2 +-
> > 4 files changed, 123 insertions(+), 14 deletions(-)
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index d07d857ca67f..eaea129c38ff 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -610,6 +610,7 @@ struct bpf_subprog_arg_info {
> > enum bpf_arg_type arg_type;
> > union {
> > u32 mem_size;
> > + u32 btf_id;
> > };
> > };
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 368d8fe19d35..287a0581516e 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6802,9 +6802,78 @@ static bool btf_is_dynptr_ptr(const struct btf *btf, const struct btf_type *t)
> > return false;
> > }
> >
> > +struct bpf_cand_cache {
> > + const char *name;
> > + u32 name_len;
> > + u16 kind;
> > + u16 cnt;
> > + struct {
> > + const struct btf *btf;
> > + u32 id;
> > + } cands[];
> > +};
> > +
> > +static DEFINE_MUTEX(cand_cache_mutex);
> > +
> > +static struct bpf_cand_cache *
> > +bpf_core_find_cands(struct bpf_core_ctx *ctx, u32 local_type_id);
> > +
> > +static int btf_get_ptr_to_btf_id(struct bpf_verifier_log *log, int arg_idx,
> > + const struct btf *btf, const struct btf_type *t)
> > +{
> > + struct bpf_cand_cache *cc;
> > + struct bpf_core_ctx ctx = {
> > + .btf = btf,
> > + .log = log,
> > + };
> > + u32 kern_type_id, type_id;
> > + int err = 0;
> > +
> > + /* skip PTR and modifiers */
> > + type_id = t->type;
> > + t = btf_type_by_id(btf, t->type);
> > + while (btf_type_is_modifier(t)) {
> > + type_id = t->type;
> > + t = btf_type_by_id(btf, t->type);
> > + }
> > +
> > + mutex_lock(&cand_cache_mutex);
> > + cc = bpf_core_find_cands(&ctx, type_id);
>
> heh. core-in-the-kernel got another use case :)
it's probably also what we should use in btf_translate_to_vmlinux(),
btw, and is the reason we got into this sk_buff vs __sk_buff confusion
in the first place. But that's not part of my upcoming changes.
>
> This allows flavors as well, so
> bpf_prog(struct task_struct___v1 *tsk __arg_trusted)
> will find the match.
> I think it's fine to do. But let's add a test too.
> I don't think patch 8 checks that.
Good point, will add.
>
> > + if (IS_ERR(cc)) {
> > + err = PTR_ERR(cc);
> > + bpf_log(log, "arg#%d reference type('%s %s') candidate matching error: %d\n",
> > + arg_idx, btf_type_str(t), __btf_name_by_offset(btf, t->name_off),
> > + err);
> > + goto cand_cache_unlock;
> > + }
> > + if (cc->cnt != 1) {
> > + bpf_log(log, "arg#%d reference type('%s %s') %s\n",
> > + arg_idx, btf_type_str(t), __btf_name_by_offset(btf, t->name_off),
> > + cc->cnt == 0 ? "has no matches" : "is ambiguous");
> > + err = cc->cnt == 0 ? -ENOENT : -ESRCH;
> > + goto cand_cache_unlock;
> > + }
> > + if (strcmp(cc->cands[0].btf->name, "vmlinux") != 0) {
>
> use btf_is_module() ?
oh, I was trying to find it and couldn't. Great, will use btf_is_module().
>
> > + bpf_log(log, "arg#%d reference type('%s %s') points to kernel module type (unsupported)\n",
> > + arg_idx, btf_type_str(t), __btf_name_by_offset(btf, t->name_off));
> > + err = -EOPNOTSUPP;
> > + goto cand_cache_unlock;
> > + }
> > + kern_type_id = cc->cands[0].id;
> > +
> > +cand_cache_unlock:
> > + mutex_unlock(&cand_cache_mutex);
> > + if (err)
> > + return err;
> > +
> > + return kern_type_id;
> > +}
> > +
> > enum btf_arg_tag {
> > ARG_TAG_CTX = 0x1,
> > ARG_TAG_NONNULL = 0x2,
> > + ARG_TAG_TRUSTED = 0x4,
> > + ARG_TAG_UNTRUSTED = 0x8,
> > };
> >
> > /* Process BTF of a function to produce high-level expectation of function
> > @@ -6906,6 +6975,10 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
> >
> > if (strcmp(tag, "ctx") == 0) {
> > tags |= ARG_TAG_CTX;
> > + } else if (strcmp(tag, "trusted") == 0) {
> > + tags |= ARG_TAG_TRUSTED;
> > + } else if (strcmp(tag, "untrusted") == 0) {
> > + tags |= ARG_TAG_UNTRUSTED;
> > } else if (strcmp(tag, "nonnull") == 0) {
> > tags |= ARG_TAG_NONNULL;
> > } else {
> > @@ -6940,6 +7013,23 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
> > sub->args[i].arg_type = ARG_PTR_TO_DYNPTR | MEM_RDONLY;
> > continue;
> > }
> > + if (tags & (ARG_TAG_TRUSTED | ARG_TAG_UNTRUSTED)) {
> > + int kern_type_id;
> > +
> > + kern_type_id = btf_get_ptr_to_btf_id(log, i, btf, t);
> > + if (kern_type_id < 0)
> > + return kern_type_id;
> > +
> > + sub->args[i].arg_type = ARG_PTR_TO_BTF_ID | PTR_UNTRUSTED | PTR_MAYBE_NULL;
>
> PTR_MAYBE_NULL doesn't make sense for untrusted.
> It may be zero or -1 or 0xffff the bpf prog may or may not
> compare that with NULL or any other value.
> It doesn't affect safety and the verifier shouldn't be tracking
> null-ness of untrusted pointers. Just to avoid extra code and run-time.
The idea was to allow the caller to pass explicit NULL for such an
argument to say "no parameter", basically. I understand that at
runtime you can have non-zero actual value and exception handling code
will handle that.
>
> But more importantly...
> ARG_PTR_TO_BTF_ID with PTR_UNTRUSTED looks scary to me.
> base_type() trims flags and in many places we check
> base_type(arg) == ARG_PTR_TO_BTF_ID
> the rhs can come from helper defs in .arg1_type too.
> A lot of code need to be carefully audited to make sure
> we don't accidently introduce a safety issue because
> PTR_TO_BTF_ID | PTR_UNTRUSTED was added to btf_ptr_types[].
> The handling of it in check_reg_type() looks ok though...
>
> > @@ -8262,6 +8263,12 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> > case PTR_TO_BTF_ID | MEM_PERCPU | PTR_TRUSTED:
> > /* Handled by helper specific checks */
> > break;
> > + case PTR_TO_BTF_ID | PTR_UNTRUSTED:
> > + if (!(arg_type & PTR_UNTRUSTED)) {
> > + verbose(env, "Passing unexpected untrusted pointer as arg#%d\n", regno);
> > + return -EACCES;
> > + }
>
> both type and arg_type are untrusted, but I need to spend
> a ton more time thinking it through.
> Maybe avoid adding __arg_untrusted for now?
> The patch will be easier to understand. At least for me.
>
Ok, no problem. I added __arg_untrusted for completeness and I can see
people wanting to pass PTR_TO_BTF_ID they got from tracepoint or
whatnot. But __arg_trusted is more critical and can't be worked
around, while you can get effectively __arg_untrusted with passing
uintptr_t argument and then doing bpf_rdonly_cast(). Will drop
__arg_untrusted for now.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH bpf-next 6/8] libbpf: add __arg_trusted and __arg_untrusted annotation macros
2024-01-05 0:09 [PATCH bpf-next 0/8] PTR_TO_BTF_ID arguments in global subprogs Andrii Nakryiko
` (4 preceding siblings ...)
2024-01-05 0:09 ` [PATCH bpf-next 5/8] bpf: add __arg_trusted and __arg_untrusted global func tags Andrii Nakryiko
@ 2024-01-05 0:09 ` Andrii Nakryiko
2024-01-05 0:09 ` [PATCH bpf-next 7/8] libbpf: add bpf_core_cast() macro Andrii Nakryiko
` (3 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2024-01-05 0:09 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Dave Marchevsky
Add __arg_trusted and __arg_untrusted to annotate global func args that
accept trusted and untrusted PTR_TO_BTF_ID arguments, respectively.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/bpf_helpers.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index 2324cc42b017..e6967a999ed9 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -190,6 +190,8 @@ enum libbpf_tristate {
#define __arg_ctx __attribute__((btf_decl_tag("arg:ctx")))
#define __arg_nonnull __attribute((btf_decl_tag("arg:nonnull")))
+#define __arg_trusted __attribute((btf_decl_tag("arg:trusted")))
+#define __arg_untrusted __attribute((btf_decl_tag("arg:untrusted")))
#ifndef ___bpf_concat
#define ___bpf_concat(a, b) a ## b
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH bpf-next 7/8] libbpf: add bpf_core_cast() macro
2024-01-05 0:09 [PATCH bpf-next 0/8] PTR_TO_BTF_ID arguments in global subprogs Andrii Nakryiko
` (5 preceding siblings ...)
2024-01-05 0:09 ` [PATCH bpf-next 6/8] libbpf: add __arg_trusted and __arg_untrusted annotation macros Andrii Nakryiko
@ 2024-01-05 0:09 ` Andrii Nakryiko
2024-01-05 0:09 ` [PATCH bpf-next 8/8] selftests/bpf: add trusted/untrusted global subprog arg tests Andrii Nakryiko
` (2 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2024-01-05 0:09 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Dave Marchevsky
Add bpf_core_cast() macro that wraps bpf_rdonly_cast() kfunc. It's more
ergonomic than kfunc, as it automatically extracts btf_id with
bpf_core_type_id_kernel(), and works with type names. It also casts result
to (T *) pointer. See the definition of the macro, it's self-explanatory.
libbpf declares bpf_rdonly_cast() extern as __weak __ksym and should be
safe to not conflict with other possible declarations in user code.
But we do have a conflict with current BPF selftests that declare their
externs with first argument as `void *obj`, while libbpf opts into more
permissive `const void *obj`. This causes conflict, so we fix up BPF
selftests uses in the same patch.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/bpf_core_read.h | 13 +++++++++++++
tools/testing/selftests/bpf/bpf_kfuncs.h | 2 +-
.../selftests/bpf/progs/sk_storage_omem_uncharge.c | 2 --
tools/testing/selftests/bpf/progs/type_cast.c | 4 +---
4 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/tools/lib/bpf/bpf_core_read.h b/tools/lib/bpf/bpf_core_read.h
index 7325a12692a3..4fdb7a7320d6 100644
--- a/tools/lib/bpf/bpf_core_read.h
+++ b/tools/lib/bpf/bpf_core_read.h
@@ -2,6 +2,8 @@
#ifndef __BPF_CORE_READ_H__
#define __BPF_CORE_READ_H__
+#include <bpf/bpf_helpers.h>
+
/*
* enum bpf_field_info_kind is passed as a second argument into
* __builtin_preserve_field_info() built-in to get a specific aspect of
@@ -292,6 +294,17 @@ enum bpf_enum_value_kind {
#define bpf_core_read_user_str(dst, sz, src) \
bpf_probe_read_user_str(dst, sz, (const void *)__builtin_preserve_access_index(src))
+extern void *bpf_rdonly_cast(const void *obj, __u32 btf_id) __ksym __weak;
+
+/*
+ * Cast provided pointer *ptr* into a pointer to a specified *type* in such
+ * a way that BPF verifier will become aware of associated kernel-side BTF
+ * type. This allows to access members of kernel types directly without the
+ * need to use BPF_CORE_READ() macros.
+ */
+#define bpf_core_cast(ptr, type) \
+ ((typeof(type) *)bpf_rdonly_cast((ptr), bpf_core_type_id_kernel(type)))
+
#define ___concat(a, b) a ## b
#define ___apply(fn, n) ___concat(fn, n)
#define ___nth(_1, _2, _3, _4, _5, _6, _7, _8, _9, _10, __11, N, ...) N
diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h b/tools/testing/selftests/bpf/bpf_kfuncs.h
index b4e78c1eb37b..24dafb289190 100644
--- a/tools/testing/selftests/bpf/bpf_kfuncs.h
+++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
@@ -53,7 +53,7 @@ extern int bpf_sock_addr_set_sun_path(struct bpf_sock_addr_kern *sa_kern,
void *bpf_cast_to_kern_ctx(void *) __ksym;
-void *bpf_rdonly_cast(void *obj, __u32 btf_id) __ksym;
+extern void *bpf_rdonly_cast(const void *obj, __u32 btf_id) __ksym __weak;
extern int bpf_get_file_xattr(struct file *file, const char *name,
struct bpf_dynptr *value_ptr) __ksym;
diff --git a/tools/testing/selftests/bpf/progs/sk_storage_omem_uncharge.c b/tools/testing/selftests/bpf/progs/sk_storage_omem_uncharge.c
index 3e745793b27a..934c4e5ada5b 100644
--- a/tools/testing/selftests/bpf/progs/sk_storage_omem_uncharge.c
+++ b/tools/testing/selftests/bpf/progs/sk_storage_omem_uncharge.c
@@ -12,8 +12,6 @@ int cookie_found = 0;
__u64 cookie = 0;
__u32 omem = 0;
-void *bpf_rdonly_cast(void *, __u32) __ksym;
-
struct {
__uint(type, BPF_MAP_TYPE_SK_STORAGE);
__uint(map_flags, BPF_F_NO_PREALLOC);
diff --git a/tools/testing/selftests/bpf/progs/type_cast.c b/tools/testing/selftests/bpf/progs/type_cast.c
index a9629ac230fd..98ab35893f51 100644
--- a/tools/testing/selftests/bpf/progs/type_cast.c
+++ b/tools/testing/selftests/bpf/progs/type_cast.c
@@ -4,6 +4,7 @@
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>
#include <bpf/bpf_core_read.h>
+#include "bpf_kfuncs.h"
struct {
__uint(type, BPF_MAP_TYPE_TASK_STORAGE);
@@ -19,9 +20,6 @@ char name[IFNAMSIZ];
unsigned int inum;
unsigned int meta_len, frag0_len, kskb_len, kskb2_len;
-void *bpf_cast_to_kern_ctx(void *) __ksym;
-void *bpf_rdonly_cast(void *, __u32) __ksym;
-
SEC("?xdp")
int md_xdp(struct xdp_md *ctx)
{
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH bpf-next 8/8] selftests/bpf: add trusted/untrusted global subprog arg tests
2024-01-05 0:09 [PATCH bpf-next 0/8] PTR_TO_BTF_ID arguments in global subprogs Andrii Nakryiko
` (6 preceding siblings ...)
2024-01-05 0:09 ` [PATCH bpf-next 7/8] libbpf: add bpf_core_cast() macro Andrii Nakryiko
@ 2024-01-05 0:09 ` Andrii Nakryiko
2024-01-07 22:22 ` [PATCH bpf-next 0/8] PTR_TO_BTF_ID arguments in global subprogs Eduard Zingerman
2024-01-12 2:40 ` patchwork-bot+netdevbpf
9 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2024-01-05 0:09 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Dave Marchevsky
Add a bunch of test cases validating behavior of __arg_trusted,
__arg_btf, and their combining with __arg_nonnull.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
.../selftests/bpf/prog_tests/verifier.c | 2 +
.../bpf/progs/verifier_global_ptr_args.c | 160 ++++++++++++++++++
2 files changed, 162 insertions(+)
create mode 100644 tools/testing/selftests/bpf/progs/verifier_global_ptr_args.c
diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index d62c5bf00e71..9c6072a19745 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -28,6 +28,7 @@
#include "verifier_div0.skel.h"
#include "verifier_div_overflow.skel.h"
#include "verifier_global_subprogs.skel.h"
+#include "verifier_global_ptr_args.skel.h"
#include "verifier_gotol.skel.h"
#include "verifier_helper_access_var_len.skel.h"
#include "verifier_helper_packet_access.skel.h"
@@ -140,6 +141,7 @@ void test_verifier_direct_stack_access_wraparound(void) { RUN(verifier_direct_st
void test_verifier_div0(void) { RUN(verifier_div0); }
void test_verifier_div_overflow(void) { RUN(verifier_div_overflow); }
void test_verifier_global_subprogs(void) { RUN(verifier_global_subprogs); }
+void test_verifier_global_ptr_args(void) { RUN(verifier_global_ptr_args); }
void test_verifier_gotol(void) { RUN(verifier_gotol); }
void test_verifier_helper_access_var_len(void) { RUN(verifier_helper_access_var_len); }
void test_verifier_helper_packet_access(void) { RUN(verifier_helper_packet_access); }
diff --git a/tools/testing/selftests/bpf/progs/verifier_global_ptr_args.c b/tools/testing/selftests/bpf/progs/verifier_global_ptr_args.c
new file mode 100644
index 000000000000..e0881c9e6706
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_global_ptr_args.c
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_core_read.h>
+#include "bpf_misc.h"
+#include "xdp_metadata.h"
+#include "bpf_kfuncs.h"
+
+extern struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym __weak;
+extern void bpf_task_release(struct task_struct *p) __ksym __weak;
+
+__weak int subprog_btf_task_nullable(struct task_struct *task __arg_untrusted)
+{
+ if (!task)
+ return 0;
+ return task->pid + task->tgid;
+}
+
+SEC("?kprobe")
+__success __log_level(2)
+__msg("Validating subprog_btf_task_nullable() func#1...")
+__msg(": R1=untrusted_ptr_or_null_task_struct(id=2) R10=fp0")
+int btf_task_arg_nullable(void *ctx)
+{
+ struct task_struct *t = (void *)bpf_get_current_task();
+ struct task_struct *untrusted = bpf_core_cast(t, struct task_struct);
+
+ return subprog_btf_task_nullable(untrusted) + subprog_btf_task_nullable(NULL);
+}
+
+__weak int subprog_btf_task_nonnull(struct task_struct *task __arg_untrusted __arg_nonnull)
+{
+ return task->pid + task->tgid;
+}
+
+SEC("?kprobe")
+__failure __log_level(2)
+__msg("R1 type=scalar expected=ptr_, trusted_ptr_, rcu_ptr_")
+__msg("Caller passes invalid args into func#1 ('subprog_btf_task_nonnull')")
+int btf_task_arg_nonnull_fail1(void *ctx)
+{
+ return subprog_btf_task_nonnull(NULL);
+}
+
+SEC("?tp_btf/task_newtask")
+__failure __log_level(2)
+__msg("R1 type=ptr_or_null_ expected=ptr_, trusted_ptr_, rcu_ptr_")
+__msg("Caller passes invalid args into func#1 ('subprog_btf_task_nonnull')")
+int btf_task_arg_nonnull_fail2(void *ctx)
+{
+ struct task_struct *t = bpf_get_current_task_btf();
+ struct task_struct *nullable;
+ int res;
+
+ nullable = bpf_task_acquire(t);
+
+ /* should fail, PTR_TO_BTF_ID_OR_NULL */
+ res = subprog_btf_task_nonnull(nullable);
+
+ if (nullable)
+ bpf_task_release(nullable);
+
+ return res;
+}
+
+SEC("?kprobe")
+__success __log_level(2)
+__msg("Validating subprog_btf_task_nonnull() func#1...")
+__msg(": R1=untrusted_ptr_task_struct(id=2) R10=fp0")
+int btf_task_arg_nonnull(void *ctx)
+{
+ struct task_struct *t = (void *)bpf_get_current_task();
+
+ return subprog_btf_task_nonnull(bpf_core_cast(t, typeof(*t)));
+}
+
+__weak int subprog_nullable_trusted_task(struct task_struct *task __arg_trusted)
+{
+ char buf[16];
+
+ if (!task)
+ return 0;
+
+ return bpf_copy_from_user_task(&buf, sizeof(buf), NULL, task, 0);
+}
+
+SEC("?uprobe.s")
+__success __log_level(2)
+__msg("Validating subprog_nullable_trusted_task() func#1...")
+__msg(": R1=trusted_ptr_or_null_task_struct(id=1) R10=fp0")
+int trusted_ptr_nullable(void *ctx)
+{
+ struct task_struct *t = bpf_get_current_task_btf();
+
+ return subprog_nullable_trusted_task(t);
+}
+
+__weak int subprog_nonnull_trusted_task(struct task_struct *task __arg_trusted __arg_nonnull)
+{
+ char buf[16];
+
+ return bpf_copy_from_user_task(&buf, sizeof(buf), NULL, task, 0);
+}
+
+SEC("?uprobe.s")
+__success __log_level(2)
+__msg("Validating subprog_nonnull_trusted_task() func#1...")
+__msg(": R1=trusted_ptr_task_struct(id=1) R10=fp0")
+int trusted_ptr_nonnull(void *ctx)
+{
+ struct task_struct *t = bpf_get_current_task_btf();
+
+ return subprog_nonnull_trusted_task(t);
+}
+
+__weak int subprog_trusted_destroy(struct task_struct *task __arg_trusted)
+{
+ if (!task)
+ return 0;
+
+ bpf_task_release(task); /* should be rejected */
+
+ return 0;
+}
+
+SEC("?tp_btf/task_newtask")
+__failure __log_level(2)
+__msg("release kernel function bpf_task_release expects refcounted PTR_TO_BTF_ID")
+int BPF_PROG(trusted_destroy_fail, struct task_struct *task, u64 clone_flags)
+{
+ return subprog_trusted_destroy(task);
+}
+
+__weak int subprog_trusted_acq_rel(struct task_struct *task __arg_trusted)
+{
+ struct task_struct *owned;
+
+ if (!task)
+ return 0;
+
+ owned = bpf_task_acquire(task);
+ if (!owned)
+ return 0;
+
+ bpf_task_release(owned); /* this one is OK, we acquired it locally */
+
+ return 0;
+}
+
+SEC("?tp_btf/task_newtask")
+__success __log_level(2)
+int BPF_PROG(trusted_acq_rel, struct task_struct *task, u64 clone_flags)
+{
+ return subprog_trusted_acq_rel(task);
+}
+
+char _license[] SEC("license") = "GPL";
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 0/8] PTR_TO_BTF_ID arguments in global subprogs
2024-01-05 0:09 [PATCH bpf-next 0/8] PTR_TO_BTF_ID arguments in global subprogs Andrii Nakryiko
` (7 preceding siblings ...)
2024-01-05 0:09 ` [PATCH bpf-next 8/8] selftests/bpf: add trusted/untrusted global subprog arg tests Andrii Nakryiko
@ 2024-01-07 22:22 ` Eduard Zingerman
2024-01-12 2:40 ` patchwork-bot+netdevbpf
9 siblings, 0 replies; 13+ messages in thread
From: Eduard Zingerman @ 2024-01-07 22:22 UTC (permalink / raw)
To: Andrii Nakryiko, bpf, ast, daniel, martin.lau
Cc: kernel-team, Dave Marchevsky
On Thu, 2024-01-04 at 16:09 -0800, Andrii Nakryiko wrote:
> This patch set follows recent changes that added btf_decl_tag-based argument
> annotation support for global subprogs. This time we add ability to pass
> PTR_TO_BTF_ID (BTF-aware kernel pointers) arguments into global subprograms.
> We support explicitly trusted and untrusted arguments. Legacy semi-trusted
> variant is not supported.
>
> Patches #2 through #4 do preparatory refactorings to add support for multiple
> tags per argument. This is important for being able to use modifiers like
> __arg_nonnull together with trusted/untrusted arguments.
>
> Patch #5 is adding the actual __arg_trusted and __arg_untrusted support.
>
> It also raises a question about default nullable vs non-nullable semantics for
> PTR_TO_BTF_ID arguments. It feels like having both __arg_nonnull and
> __arg_nullable would provide the best kind of experience and flexibility, but
> for now we implement nullable by default semantics, as a more conservative
> behavior.
>
> Patch #7 adds bpf_core_cast() helper macro which is a wrapper around
> bpf_rdonly_cast() kfunc, but hides BTF ID manipulations behind more
> user-friendly type argument instead. We utilize this macro in selftests added
> in patch #8.
>
> Patch #8 adds a bunch of positive and negative tests to validate expected
> semantics for various trusted/untrusted + nullable/non-null variants. We also
> make sure that global subprog cannot destroy PTR_TO_BTF_ID, as that would
> wreak havoc in caller program that is not aware of this possibility.
>
> There were proposals to do kernel-side type enforcement for __arg_ctx, let's
> decide whether we should do that and for which program types, and I can
> accommodate the logic in future revisions.
>
> Cc: Dave Marchevsky <davemarchevsky@meta.com>
Full patch-set looks good to me.
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 0/8] PTR_TO_BTF_ID arguments in global subprogs
2024-01-05 0:09 [PATCH bpf-next 0/8] PTR_TO_BTF_ID arguments in global subprogs Andrii Nakryiko
` (8 preceding siblings ...)
2024-01-07 22:22 ` [PATCH bpf-next 0/8] PTR_TO_BTF_ID arguments in global subprogs Eduard Zingerman
@ 2024-01-12 2:40 ` patchwork-bot+netdevbpf
9 siblings, 0 replies; 13+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-01-12 2:40 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, ast, daniel, martin.lau, kernel-team, davemarchevsky
Hello:
This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Thu, 4 Jan 2024 16:09:01 -0800 you wrote:
> This patch set follows recent changes that added btf_decl_tag-based argument
> annotation support for global subprogs. This time we add ability to pass
> PTR_TO_BTF_ID (BTF-aware kernel pointers) arguments into global subprograms.
> We support explicitly trusted and untrusted arguments. Legacy semi-trusted
> variant is not supported.
>
> Patches #2 through #4 do preparatory refactorings to add support for multiple
> tags per argument. This is important for being able to use modifiers like
> __arg_nonnull together with trusted/untrusted arguments.
>
> [...]
Here is the summary with links:
- [bpf-next,1/8] selftests/bpf: fix test_loader check message
https://git.kernel.org/bpf/bpf-next/c/7c5f9568564a
- [bpf-next,2/8] bpf: make sure scalar args don't accept __arg_nonnull tag
https://git.kernel.org/bpf/bpf-next/c/ca3950495684
- [bpf-next,3/8] bpf: prepare btf_prepare_func_args() for multiple tags per argument
https://git.kernel.org/bpf/bpf-next/c/5436740cc03d
- [bpf-next,4/8] bpf: support multiple tags per argument
https://git.kernel.org/bpf/bpf-next/c/6a21d4e515f5
- [bpf-next,5/8] bpf: add __arg_trusted and __arg_untrusted global func tags
(no matching commit)
- [bpf-next,6/8] libbpf: add __arg_trusted and __arg_untrusted annotation macros
(no matching commit)
- [bpf-next,7/8] libbpf: add bpf_core_cast() macro
(no matching commit)
- [bpf-next,8/8] selftests/bpf: add trusted/untrusted global subprog arg tests
(no matching commit)
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 13+ messages in thread