| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAEf4BzbS_JFW70Z_68hDtN4VTkLfohkR0PV0d9jCJRjZEhc01Q@mail.gmail.com>
Date: Tue, 28 Jul 2020 16:27:21 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Jiri Olsa <jolsa@...nel.org>
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andriin@...com>,
Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
Martin KaFai Lau <kafai@...com>,
David Miller <davem@...hat.com>,
John Fastabend <john.fastabend@...il.com>,
Wenbo Zhang <ethercflow@...il.com>,
KP Singh <kpsingh@...omium.org>,
Brendan Gregg <bgregg@...flix.com>,
Florent Revest <revest@...omium.org>,
Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v8 bpf-next 06/13] bpf: Factor btf_struct_access function
On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa <jolsa@...nel.org> wrote:
>
> Adding btf_struct_walk function that walks through the
> struct type + given offset and returns following values:
>
> enum walk_return {
> /* < 0 error */
> walk_scalar = 0,
> walk_ptr,
> walk_struct,
> };
>
> walk_scalar - when SCALAR_VALUE is found
> walk_ptr - when pointer value is found, its ID is stored
> in 'rid' output param
> walk_struct - when nested struct object is found, its ID is stored
> in 'rid' output param
>
> It will be used in following patches to get all nested
> struct objects for given type and offset.
>
> The btf_struct_access now calls btf_struct_walk function,
> as long as it gets nested structs as return value.
>
> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> ---
It actually worked out to a pretty minimal changes to
btf_struct_access, I'm pleasantly surprised :))
Logic seems correct, just have few nits about naming and a bit more
safe handling in btf_struct_access, see below.
> kernel/bpf/btf.c | 73 +++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 60 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 841be6c49f11..1ab5fd5bf992 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3873,16 +3873,22 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> return true;
> }
>
> -int btf_struct_access(struct bpf_verifier_log *log,
> - const struct btf_type *t, int off, int size,
> - enum bpf_access_type atype,
> - u32 *next_btf_id)
> +enum walk_return {
> + /* < 0 error */
> + walk_scalar = 0,
> + walk_ptr,
> + walk_struct,
> +};
let's keep enum values in ALL_CAPS? walk_return is also a bit generic,
maybe something like bpf_struct_walk_result?
> +
> +static int btf_struct_walk(struct bpf_verifier_log *log,
> + const struct btf_type *t, int off, int size,
> + u32 *rid)
> {
> u32 i, moff, mtrue_end, msize = 0, total_nelems = 0;
> const struct btf_type *mtype, *elem_type = NULL;
> const struct btf_member *member;
> const char *tname, *mname;
> - u32 vlen;
> + u32 vlen, elem_id, mid;
>
> again:
> tname = __btf_name_by_offset(btf_vmlinux, t->name_off);
> @@ -3924,8 +3930,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
> goto error;
>
> off = (off - moff) % elem_type->size;
> - return btf_struct_access(log, elem_type, off, size, atype,
> - next_btf_id);
> + return btf_struct_walk(log, elem_type, off, size, rid);
oh, btw, this is a recursion in the kernel, let's fix that? I think it
could easily be just `goto again` here?
>
> error:
> bpf_log(log, "access beyond struct %s at off %u size %u\n",
> @@ -3954,7 +3959,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
> */
> if (off <= moff &&
> BITS_ROUNDUP_BYTES(end_bit) <= off + size)
> - return SCALAR_VALUE;
> + return walk_scalar;
>
> /* off may be accessing a following member
> *
[...]
> @@ -4066,11 +4080,10 @@ int btf_struct_access(struct bpf_verifier_log *log,
> mname, moff, tname, off, size);
> return -EACCES;
> }
> -
> stype = btf_type_skip_modifiers(btf_vmlinux, mtype->type, &id);
> if (btf_type_is_struct(stype)) {
> - *next_btf_id = id;
> - return PTR_TO_BTF_ID;
> + *rid = id;
nit: rid is a very opaque name, I find next_btf_id more appropriate
(even if it's meaning changes depending on walk_ptr vs walk_struct.
> + return walk_ptr;
> }
> }
>
> @@ -4087,12 +4100,46 @@ int btf_struct_access(struct bpf_verifier_log *log,
> return -EACCES;
> }
>
> - return SCALAR_VALUE;
> + return walk_scalar;
> }
> bpf_log(log, "struct %s doesn't have field at offset %d\n", tname, off);
> return -EINVAL;
> }
>
> +int btf_struct_access(struct bpf_verifier_log *log,
> + const struct btf_type *t, int off, int size,
> + enum bpf_access_type atype __maybe_unused,
> + u32 *next_btf_id)
> +{
> + int err;
> + u32 id;
> +
> + do {
> + err = btf_struct_walk(log, t, off, size, &id);
> + if (err < 0)
> + return err;
> +
> + /* We found the pointer or scalar on t+off,
> + * we're done.
> + */
> + if (err == walk_ptr) {
> + *next_btf_id = id;
> + return PTR_TO_BTF_ID;
> + }
> + if (err == walk_scalar)
> + return SCALAR_VALUE;
> +
> + /* We found nested struct, so continue the search
> + * by diving in it. At this point the offset is
> + * aligned with the new type, so set it to 0.
> + */
> + t = btf_type_by_id(btf_vmlinux, id);
> + off = 0;
It's very easy to miss that this case corresponds to walk_struct here.
If someone in the future adds a 4th special value, it will be too easy
to forget to update this piece of logic. So when dealing with enums, I
generally prefer this approach:
switch (err) {
case walk_ptr:
...
case walk_scalar:
...
case walk_struct:
...
default: /* complain loudly here */
}
WDYT?
> + } while (t);
> +
> + return -EINVAL;
> +}
> +
> int btf_resolve_helper_id(struct bpf_verifier_log *log,
> const struct bpf_func_proto *fn, int arg)
> {
> --
> 2.25.4
>
Powered by blists - more mailing lists