On Wed, 9 Sep 2020 at 21:03, Martin KaFai Lau <kafai@xxxxxx> wrote:
>
[...]
> > @@ -4002,29 +4001,23 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > goto err_type;
> > }
> > } else if (arg_type == ARG_PTR_TO_BTF_ID) {
> > - bool ids_match = false;
> > + const u32 *btf_id = fn->arg_btf_id[arg];
> >
> > expected_type = PTR_TO_BTF_ID;
> > if (type != expected_type)
> > goto err_type;
> > - if (!fn->check_btf_id) {
> > - if (reg->btf_id != meta->btf_id) {
> > - ids_match = btf_struct_ids_match(&env->log, reg->off, reg->btf_id,
> > - meta->btf_id);
> > - if (!ids_match) {
> > - verbose(env, "Helper has type %s got %s in R%d\n",
> > - kernel_type_name(meta->btf_id),
> > - kernel_type_name(reg->btf_id), regno);
> > - return -EACCES;
> > - }
> > - }
> > - } else if (!fn->check_btf_id(reg->btf_id, arg)) {
> > - verbose(env, "Helper does not support %s in R%d\n",
> > - kernel_type_name(reg->btf_id), regno);
> >
> > + if (!btf_id) {
> > + verbose(env, "verifier internal error: missing BTF ID\n");
> check_func_proto() could be a better place for this check.
The wrinkle here is that this also protects from someone adding PTR_TO_BTF_ID to
compatible_reg_types without specifying a btf_id. I want to do that for
ARG_PTR_TO_SOCK_COMMON_OR_NULL in a follow up, so I think the test here makes
sense.
However, I think ensuring btf_id in check_func_proto in addition to
this is a good idea.
[...]
> > diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> > index a0d1a3265b71..442a34a7ee2b 100644
> > --- a/net/core/bpf_sk_storage.c
> > +++ b/net/core/bpf_sk_storage.c
> > @@ -357,6 +357,7 @@ const struct bpf_func_proto bpf_sk_storage_get_proto = {
> > .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
> > .arg1_type = ARG_CONST_MAP_PTR,
> > .arg2_type = ARG_PTR_TO_SOCKET,
> > + .arg2_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
> This change is not needed. It is not taking ARG_PTR_TO_BTF_ID.
>
> > .arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> > .arg4_type = ARG_ANYTHING,
> > };
> > @@ -377,21 +378,18 @@ const struct bpf_func_proto bpf_sk_storage_delete_proto = {
> > .ret_type = RET_INTEGER,
> > .arg1_type = ARG_CONST_MAP_PTR,
> > .arg2_type = ARG_PTR_TO_SOCKET,
> > + .arg2_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
> Same here.
Don't know where these came from, sorry.
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
