| Message ID | 20220809134603.1769279-4-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bpf: Add kfuncs for PKCS#7 signature verification | expand |
On 8/9/22 3:45 PM, Roberto Sassu wrote: > Allow the bpf_dynptr_kern parameter to be specified in kfuncs. Also, ensure > that the dynamic pointer is valid and initialized. > > Cc: Joanne Koong <joannelkoong@gmail.com> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > include/linux/bpf_verifier.h | 3 +++ > kernel/bpf/btf.c | 17 +++++++++++++++++ > kernel/bpf/verifier.c | 4 ++-- > 3 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > index 2e3bad8640dc..55876fbdbae2 100644 > --- a/include/linux/bpf_verifier.h > +++ b/include/linux/bpf_verifier.h > @@ -560,6 +560,9 @@ int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state > u32 regno); > int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg, > u32 regno, u32 mem_size); > +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, > + struct bpf_reg_state *reg, > + enum bpf_arg_type arg_type); > > /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */ > static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog, > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 67dfc728fbf8..17cca396c89f 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -6363,6 +6363,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > if (is_kfunc) { > bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], ®s[regno + 1]); > + bool arg_dynptr = btf_type_is_struct(ref_t) && > + !strcmp(ref_tname, "bpf_dynptr_kern"); For the strcmp(), could we add some BUILD_BUG_ON() if "bpf_dynptr_kern" ever gets renamed? I played a bit with the below compile-tested toy example. If we rename foo_bar into something else, we then get: [...] CC net/core/dev.o In file included from <command-line>: net/core/dev.c: In function ‘net_dev_init’: net/core/dev.c:11376:25: error: invalid application of ‘sizeof’ to incomplete type ‘struct foo_bar’ 11376 | ({ BUILD_BUG_ON(sizeof(struct x) < 0); \ | ^~~~~~ [...] diff --git a/net/core/dev.c b/net/core/dev.c index 716df64fcfa5..a2ed271f7ded 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -11368,16 +11368,30 @@ static struct pernet_operations __net_initdata default_device_ops = { * */ +struct foo_bar { + int a; +}; + +#define stringify_struct(x) \ + ({ BUILD_BUG_ON(sizeof(struct x) < 0); \ + __stringify(x); }) + /* * This is called single threaded during boot, so no need * to take the rtnl semaphore. */ static int __init net_dev_init(void) { + const char *ref_tname = "abc"; int i, rc = -ENOMEM; BUG_ON(!dev_boot_phase); + printk("%s\n", stringify_struct(foo_bar)); + + if (!strcmp(ref_tname, stringify_struct(foo_bar))) + goto out; + if (dev_proc_init()) goto out;
On 8/9/22 3:45 PM, Roberto Sassu wrote: [...] > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 67dfc728fbf8..17cca396c89f 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -6363,6 +6363,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > if (is_kfunc) { > bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], ®s[regno + 1]); > + bool arg_dynptr = btf_type_is_struct(ref_t) && > + !strcmp(ref_tname, "bpf_dynptr_kern"); > > /* Permit pointer to mem, but only when argument > * type is pointer to scalar, or struct composed > @@ -6372,6 +6374,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > */ > if (!btf_type_is_scalar(ref_t) && > !__btf_type_is_scalar_struct(log, btf, ref_t, 0) && > + !arg_dynptr && > (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) { > bpf_log(log, > "arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n", > @@ -6379,6 +6382,20 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > return -EINVAL; > } > > + /* Assume initialized dynptr. */ This comment is a bit misleading, too, given we don't assume but enforce it. I'd probably just fold this into above one where we permit pointer to mem given the test there gets extended anyway, so the comment should be in line with the tests. > + if (arg_dynptr) { > + if (!is_dynptr_reg_valid_init(env, reg, > + ARG_PTR_TO_DYNPTR)) { > + bpf_log(log, > + "arg#%d pointer type %s %s must be initialized\n", > + i, btf_type_str(ref_t), > + ref_tname); > + return -EINVAL; > + } > + > + continue; > + } > + > /* Check for mem, len pair */ > if (arg_mem_size) { > if (check_kfunc_mem_size_reg(env, ®s[regno + 1], regno + 1)) {
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 2e3bad8640dc..55876fbdbae2 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -560,6 +560,9 @@ int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state u32 regno); int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg, u32 regno, u32 mem_size); +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, + struct bpf_reg_state *reg, + enum bpf_arg_type arg_type); /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */ static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog, diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 67dfc728fbf8..17cca396c89f 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -6363,6 +6363,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, if (is_kfunc) { bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], ®s[regno + 1]); + bool arg_dynptr = btf_type_is_struct(ref_t) && + !strcmp(ref_tname, "bpf_dynptr_kern"); /* Permit pointer to mem, but only when argument * type is pointer to scalar, or struct composed @@ -6372,6 +6374,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, */ if (!btf_type_is_scalar(ref_t) && !__btf_type_is_scalar_struct(log, btf, ref_t, 0) && + !arg_dynptr && (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) { bpf_log(log, "arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n", @@ -6379,6 +6382,20 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, return -EINVAL; } + /* Assume initialized dynptr. */ + if (arg_dynptr) { + if (!is_dynptr_reg_valid_init(env, reg, + ARG_PTR_TO_DYNPTR)) { + bpf_log(log, + "arg#%d pointer type %s %s must be initialized\n", + i, btf_type_str(ref_t), + ref_tname); + return -EINVAL; + } + + continue; + } + /* Check for mem, len pair */ if (arg_mem_size) { if (check_kfunc_mem_size_reg(env, ®s[regno + 1], regno + 1)) { diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 843a966cd02b..3c52246a1ceb 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -773,8 +773,8 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_ return true; } -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg, - enum bpf_arg_type arg_type) +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg, + enum bpf_arg_type arg_type) { struct bpf_func_state *state = func(env, reg); int spi = get_spi(reg->off);
Allow the bpf_dynptr_kern parameter to be specified in kfuncs. Also, ensure that the dynamic pointer is valid and initialized. Cc: Joanne Koong <joannelkoong@gmail.com> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- include/linux/bpf_verifier.h | 3 +++ kernel/bpf/btf.c | 17 +++++++++++++++++ kernel/bpf/verifier.c | 4 ++-- 3 files changed, 22 insertions(+), 2 deletions(-)