The verifier assumes that map values are simple blobs of memory, and
therefore treats ARG_PTR_TO_MAP_VALUE, etc. as such. However, there are
map types where this isn't true. For example, sockmap and sockhash store
sockets. In general this isn't a big problem: we can just
write helpers that explicitly requests PTR_TO_SOCKET instead of
ARG_PTR_TO_MAP_VALUE.
The one exception are the standard map helpers like map_update_elem,
map_lookup_elem, etc. Here it would be nice we could overload the
function prototype for different kinds of maps. Unfortunately, this
isn't entirely straight forward:
We only know the type of the map once we have resolved meta->map_ptr
in check_func_arg. This means we can't swap out the prototype
in check_helper_call until we're half way through the function.
Instead, modify check_func_arg to treat ARG_PTR_TO_MAP_VALUE* to
mean "the native type for the map" instead of "pointer to memory"
for sockmap and sockhash. This means we don't have to modify the
function prototype at all
Signed-off-by: Lorenz Bauer <[email protected]>
---
kernel/bpf/verifier.c | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index b6ccfce3bf4c..47f9b94bb9d4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3872,6 +3872,38 @@ static int int_ptr_type_to_size(enum bpf_arg_type type)
return -EINVAL;
}
+static int override_map_arg_type(struct bpf_verifier_env *env,
+ const struct bpf_call_arg_meta *meta,
+ enum bpf_arg_type *arg_type)
+{
+ if (!meta->map_ptr) {
+ /* kernel subsystem misconfigured verifier */
+ verbose(env, "invalid map_ptr to access map->type\n");
+ return -EACCES;
+ }
+
+ switch (meta->map_ptr->map_type) {
+ case BPF_MAP_TYPE_SOCKMAP:
+ case BPF_MAP_TYPE_SOCKHASH:
+ switch (*arg_type) {
+ case ARG_PTR_TO_MAP_VALUE:
+ *arg_type = ARG_PTR_TO_SOCKET;
+ break;
+ case ARG_PTR_TO_MAP_VALUE_OR_NULL:
+ *arg_type = ARG_PTR_TO_SOCKET_OR_NULL;
+ break;
+ default:
+ verbose(env, "invalid arg_type for sockmap/sockhash\n");
+ return -EINVAL;
+ }
+ break;
+
+ default:
+ break;
+ }
+ return 0;
+}
+
static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
struct bpf_call_arg_meta *meta,
const struct bpf_func_proto *fn)
@@ -3904,6 +3936,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
return -EACCES;
}
+ if (arg_type == ARG_PTR_TO_MAP_VALUE ||
+ arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
+ arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
+ err = override_map_arg_type(env, meta, &arg_type);
+ if (err)
+ return err;
+ }
+
if (arg_type == ARG_PTR_TO_MAP_KEY ||
arg_type == ARG_PTR_TO_MAP_VALUE ||
arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
--
2.25.1
On 8/19/20 2:24 AM, Lorenz Bauer wrote:
> The verifier assumes that map values are simple blobs of memory, and
> therefore treats ARG_PTR_TO_MAP_VALUE, etc. as such. However, there are
> map types where this isn't true. For example, sockmap and sockhash store
> sockets. In general this isn't a big problem: we can just
> write helpers that explicitly requests PTR_TO_SOCKET instead of
> ARG_PTR_TO_MAP_VALUE.
>
> The one exception are the standard map helpers like map_update_elem,
> map_lookup_elem, etc. Here it would be nice we could overload the
> function prototype for different kinds of maps. Unfortunately, this
> isn't entirely straight forward:
> We only know the type of the map once we have resolved meta->map_ptr
> in check_func_arg. This means we can't swap out the prototype
> in check_helper_call until we're half way through the function.
>
> Instead, modify check_func_arg to treat ARG_PTR_TO_MAP_VALUE* to
> mean "the native type for the map" instead of "pointer to memory"
> for sockmap and sockhash. This means we don't have to modify the
> function prototype at all
>
> Signed-off-by: Lorenz Bauer <[email protected]>
> ---
> kernel/bpf/verifier.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index b6ccfce3bf4c..47f9b94bb9d4 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3872,6 +3872,38 @@ static int int_ptr_type_to_size(enum bpf_arg_type type)
> return -EINVAL;
> }
>
> +static int override_map_arg_type(struct bpf_verifier_env *env,
> + const struct bpf_call_arg_meta *meta,
> + enum bpf_arg_type *arg_type)
> +{
> + if (!meta->map_ptr) {
> + /* kernel subsystem misconfigured verifier */
> + verbose(env, "invalid map_ptr to access map->type\n");
> + return -EACCES;
> + }
> +
> + switch (meta->map_ptr->map_type) {
> + case BPF_MAP_TYPE_SOCKMAP:
> + case BPF_MAP_TYPE_SOCKHASH:
> + switch (*arg_type) {
> + case ARG_PTR_TO_MAP_VALUE:
> + *arg_type = ARG_PTR_TO_SOCKET;
> + break;
> + case ARG_PTR_TO_MAP_VALUE_OR_NULL:
> + *arg_type = ARG_PTR_TO_SOCKET_OR_NULL;
> + break;
> + default:
> + verbose(env, "invalid arg_type for sockmap/sockhash\n");
> + return -EINVAL;
> + }
> + break;
> +
> + default:
> + break;
> + }
> + return 0;
> +}
> +
> static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> struct bpf_call_arg_meta *meta,
> const struct bpf_func_proto *fn)
> @@ -3904,6 +3936,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> return -EACCES;
> }
>
> + if (arg_type == ARG_PTR_TO_MAP_VALUE ||
> + arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
> + arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
We probably do not need ARG_PTR_TO_UNINIT_MAP_VALUE here.
Do we need ARG_PTR_TO_MAP_VALUE_OR_NULL? bpf_map_update_elem arg type
is ARG_PTR_TO_MAP_VALUE.
> + err = override_map_arg_type(env, meta, &arg_type);
> + if (err)
> + return err;
> + }
> +
> if (arg_type == ARG_PTR_TO_MAP_KEY ||
> arg_type == ARG_PTR_TO_MAP_VALUE ||
> arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
>
Lorenz Bauer wrote:
> The verifier assumes that map values are simple blobs of memory, and
> therefore treats ARG_PTR_TO_MAP_VALUE, etc. as such. However, there are
> map types where this isn't true. For example, sockmap and sockhash store
> sockets. In general this isn't a big problem: we can just
> write helpers that explicitly requests PTR_TO_SOCKET instead of
> ARG_PTR_TO_MAP_VALUE.
>
> The one exception are the standard map helpers like map_update_elem,
> map_lookup_elem, etc. Here it would be nice we could overload the
> function prototype for different kinds of maps. Unfortunately, this
> isn't entirely straight forward:
> We only know the type of the map once we have resolved meta->map_ptr
> in check_func_arg. This means we can't swap out the prototype
> in check_helper_call until we're half way through the function.
>
> Instead, modify check_func_arg to treat ARG_PTR_TO_MAP_VALUE* to
> mean "the native type for the map" instead of "pointer to memory"
> for sockmap and sockhash. This means we don't have to modify the
> function prototype at all
>
> Signed-off-by: Lorenz Bauer <[email protected]>
> ---
> kernel/bpf/verifier.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index b6ccfce3bf4c..47f9b94bb9d4 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3872,6 +3872,38 @@ static int int_ptr_type_to_size(enum bpf_arg_type type)
> return -EINVAL;
> }
>
> +static int override_map_arg_type(struct bpf_verifier_env *env,
> + const struct bpf_call_arg_meta *meta,
> + enum bpf_arg_type *arg_type)
One nit can we rename this to refine_map_arg_type or resolve_map_arg_type I
don't like the name "override" we are getting a more precise type here.
> +{
> + if (!meta->map_ptr) {
> + /* kernel subsystem misconfigured verifier */
> + verbose(env, "invalid map_ptr to access map->type\n");
> + return -EACCES;
> + }
> +
> + switch (meta->map_ptr->map_type) {
> + case BPF_MAP_TYPE_SOCKMAP:
> + case BPF_MAP_TYPE_SOCKHASH:
> + switch (*arg_type) {
> + case ARG_PTR_TO_MAP_VALUE:
> + *arg_type = ARG_PTR_TO_SOCKET;
> + break;
> + case ARG_PTR_TO_MAP_VALUE_OR_NULL:
> + *arg_type = ARG_PTR_TO_SOCKET_OR_NULL;
> + break;
> + default:
> + verbose(env, "invalid arg_type for sockmap/sockhash\n");
Might be worth pushing the arg_type into the verbose message so its obvious
where the types went wrong. We will probably "know" just based on the
switch in front of this, but users in general wont. Just a suggestion if
you think its overkill go ahead and skip.
> + return -EINVAL;
> + }
> + break;
> +
> + default:
> + break;
> + }
> + return 0;
> +}
> +
Otherwise LGTM.
> static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> struct bpf_call_arg_meta *meta,
> const struct bpf_func_proto *fn)
> @@ -3904,6 +3936,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> return -EACCES;
> }
>
> + if (arg_type == ARG_PTR_TO_MAP_VALUE ||
> + arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
> + arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
> + err = override_map_arg_type(env, meta, &arg_type);
> + if (err)
> + return err;
> + }
> +
> if (arg_type == ARG_PTR_TO_MAP_KEY ||
> arg_type == ARG_PTR_TO_MAP_VALUE ||
> arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
> --
> 2.25.1
>
On Wed, 19 Aug 2020 at 21:13, Yonghong Song <[email protected]> wrote:
>
>
>
> On 8/19/20 2:24 AM, Lorenz Bauer wrote:
> > The verifier assumes that map values are simple blobs of memory, and
> > therefore treats ARG_PTR_TO_MAP_VALUE, etc. as such. However, there are
> > map types where this isn't true. For example, sockmap and sockhash store
> > sockets. In general this isn't a big problem: we can just
> > write helpers that explicitly requests PTR_TO_SOCKET instead of
> > ARG_PTR_TO_MAP_VALUE.
> >
> > The one exception are the standard map helpers like map_update_elem,
> > map_lookup_elem, etc. Here it would be nice we could overload the
> > function prototype for different kinds of maps. Unfortunately, this
> > isn't entirely straight forward:
> > We only know the type of the map once we have resolved meta->map_ptr
> > in check_func_arg. This means we can't swap out the prototype
> > in check_helper_call until we're half way through the function.
> >
> > Instead, modify check_func_arg to treat ARG_PTR_TO_MAP_VALUE* to
> > mean "the native type for the map" instead of "pointer to memory"
> > for sockmap and sockhash. This means we don't have to modify the
> > function prototype at all
> >
> > Signed-off-by: Lorenz Bauer <[email protected]>
> > ---
> > kernel/bpf/verifier.c | 40 ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 40 insertions(+)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index b6ccfce3bf4c..47f9b94bb9d4 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -3872,6 +3872,38 @@ static int int_ptr_type_to_size(enum bpf_arg_type type)
> > return -EINVAL;
> > }
> >
> > +static int override_map_arg_type(struct bpf_verifier_env *env,
> > + const struct bpf_call_arg_meta *meta,
> > + enum bpf_arg_type *arg_type)
> > +{
> > + if (!meta->map_ptr) {
> > + /* kernel subsystem misconfigured verifier */
> > + verbose(env, "invalid map_ptr to access map->type\n");
> > + return -EACCES;
> > + }
> > +
> > + switch (meta->map_ptr->map_type) {
> > + case BPF_MAP_TYPE_SOCKMAP:
> > + case BPF_MAP_TYPE_SOCKHASH:
> > + switch (*arg_type) {
> > + case ARG_PTR_TO_MAP_VALUE:
> > + *arg_type = ARG_PTR_TO_SOCKET;
> > + break;
> > + case ARG_PTR_TO_MAP_VALUE_OR_NULL:
> > + *arg_type = ARG_PTR_TO_SOCKET_OR_NULL;
> > + break;
> > + default:
> > + verbose(env, "invalid arg_type for sockmap/sockhash\n");
> > + return -EINVAL;
> > + }
> > + break;
> > +
> > + default:
> > + break;
> > + }
> > + return 0;
> > +}
> > +
> > static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > struct bpf_call_arg_meta *meta,
> > const struct bpf_func_proto *fn)
> > @@ -3904,6 +3936,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > return -EACCES;
> > }
> >
> > + if (arg_type == ARG_PTR_TO_MAP_VALUE ||
> > + arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
> > + arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
>
> We probably do not need ARG_PTR_TO_UNINIT_MAP_VALUE here.
>
> Do we need ARG_PTR_TO_MAP_VALUE_OR_NULL? bpf_map_update_elem arg type
> is ARG_PTR_TO_MAP_VALUE.
I did this to be consistent: in a single function definition you
either get the map specific
types or the regular semantics. You don't get to mix and match them.
For the same
reason I included ARG_PTR_TO_UNINIT_MAP_VALUE: the semantics don't make
sense for sockmap, so a function using this doesn't make sense either.
>
> > + err = override_map_arg_type(env, meta, &arg_type);
> > + if (err)
> > + return err;
> > + }
> > +
> > if (arg_type == ARG_PTR_TO_MAP_KEY ||
> > arg_type == ARG_PTR_TO_MAP_VALUE ||
> > arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
> >
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
http://www.cloudflare.com