On 5/18/22 11:06 AM, Kumar Kartikeya Dwivedi wrote:
On Wed, May 18, 2022 at 11:28:12PM IST, Yonghong Song wrote:
On 5/18/22 3:43 AM, Lorenzo Bianconi wrote:
From: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
Similar to how we detect mem, size pairs in kfunc, teach verifier to
treat __ref suffix on argument name to imply that it must be a
referenced pointer when passed to kfunc. This is required to ensure that
kfunc that operate on some object only work on acquired pointers and not
normal PTR_TO_BTF_ID with same type which can be obtained by pointer
walking. Release functions need not specify such suffix on release
arguments as they are already expected to receive one referenced
argument.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
---
kernel/bpf/btf.c | 40 ++++++++++++++++++++++++++++++----------
net/bpf/test_run.c | 5 +++++
2 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 2f0b0440131c..83a354732d96 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6021,18 +6021,13 @@ static bool __btf_type_is_scalar_struct(struct bpf_verifier_log *log,
return true;
}
-static bool is_kfunc_arg_mem_size(const struct btf *btf,
- const struct btf_param *arg,
- const struct bpf_reg_state *reg)
+static bool btf_param_match_suffix(const struct btf *btf,
+ const struct btf_param *arg,
+ const char *suffix)
{
- int len, sfx_len = sizeof("__sz") - 1;
- const struct btf_type *t;
+ int len, sfx_len = strlen(suffix);
const char *param_name;
- t = btf_type_skip_modifiers(btf, arg->type, NULL);
- if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
- return false;
-
/* In the future, this can be ported to use BTF tagging */
param_name = btf_name_by_offset(btf, arg->name_off);
if (str_is_empty(param_name))
@@ -6041,12 +6036,31 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf,
if (len < sfx_len)
return false;
param_name += len - sfx_len;
- if (strncmp(param_name, "__sz", sfx_len))
+ if (strncmp(param_name, suffix, sfx_len))
return false;
return true;
}
+static bool is_kfunc_arg_ref(const struct btf *btf,
+ const struct btf_param *arg)
+{
+ return btf_param_match_suffix(btf, arg, "__ref");
Do we also need to do btf_type_skip_modifiers and to ensure
the type after skipping modifiers are a pointer type?
The current implementation should work for
bpf_kfunc_call_test_ref(), but with additional checking
we may avoid some accidental mistakes.
The point where this check happens, arg[i].type is already known to be a pointer
type, after skipping modifiers.
Okay, maybe we can add a comment here like
/* the arg has been verified as a pointer */
But anyway, this is minor.
Acked-by: Yonghong Song <yhs@xxxxxx>
+}
+
+static bool is_kfunc_arg_mem_size(const struct btf *btf,
+ const struct btf_param *arg,
+ const struct bpf_reg_state *reg)
+{
+ const struct btf_type *t;
+
+ t = btf_type_skip_modifiers(btf, arg->type, NULL);
+ if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
+ return false;
+
+ return btf_param_match_suffix(btf, arg, "__sz");
+}
+
static int btf_check_func_arg_match(struct bpf_verifier_env *env,
const struct btf *btf, u32 func_id,
struct bpf_reg_state *regs,
@@ -6115,6 +6129,12 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
return -EINVAL;
}
+ /* Check if argument must be a referenced pointer */
+ if (is_kfunc && is_kfunc_arg_ref(btf, args + i) && !reg->ref_obj_id) {
+ bpf_log(log, "R%d must be referenced\n", regno);
+ return -EINVAL;
+ }
+
ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
ref_tname = btf_name_by_offset(btf, ref_t->name_off);
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 4d08cca771c7..adbc7dd18511 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -690,6 +690,10 @@ noinline void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len)
{
}
+noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref)
+{
+}
+
__diag_pop();
[...]
--
Kartikeya
