netdev - Re: [RFC PATCH bpf-next 10/16] bpf: support variable length array in tracing programs

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4Bzb6Uied+4pE0+QbjoeBWVzVHmjEfPGfr5Gr_FKZg_CTEQ@mail.gmail.com>
Date:   Mon, 13 Apr 2020 17:13:39 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [RFC PATCH bpf-next 10/16] bpf: support variable length array in
 tracing programs

On Wed, Apr 8, 2020 at 4:26 PM Yonghong Song <yhs@...com> wrote:
>
> In /proc/net/ipv6_route, we have
>   struct fib6_info {
>     struct fib6_table *fib6_table;
>     ...
>     struct fib6_nh fib6_nh[0];
>   }
>   struct fib6_nh {
>     struct fib_nh_common nh_common;
>     struct rt6_info **rt6i_pcpu;
>     struct rt6_exception_bucket *rt6i_exception_bucket;
>   };
>   struct fib_nh_common {
>     ...
>     u8 nhc_gw_family;
>     ...
>   }
>
> The access:
>   struct fib6_nh *fib6_nh = &rt->fib6_nh;
>   ... fib6_nh->nh_common.nhc_gw_family ...
>
> This patch ensures such an access is handled properly.
>
> Signed-off-by: Yonghong Song <yhs@...com>
> ---
>  kernel/bpf/btf.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index d65c6912bdaf..89a0d983b169 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3837,6 +3837,31 @@ int btf_struct_access(struct bpf_verifier_log *log,
>         }
>
>         if (off + size > t->size) {
> +               /* If the last element is a variable size array, we may
> +                * need to relax the rule.
> +                */
> +               struct btf_array *array_elem;
> +               u32 vlen = btf_type_vlen(t);
> +               u32 last_member_type;
> +
> +               member = btf_type_member(t);
> +               last_member_type = member[vlen - 1].type;

vlen could be zero, and this will be bad


> +               mtype = btf_type_by_id(btf_vmlinux, last_member_type);

might want to strip modifiers here?

> +               if (!btf_type_is_array(mtype))
> +                       goto error;
> +

should probably check that off is >= last_member's offset within a
struct? Otherwise access might be spanning previous field and this
array?

> +               array_elem = (struct btf_array *)(mtype + 1);
> +               if (array_elem->nelems != 0)
> +                       goto error;
> +
> +               elem_type = btf_type_by_id(btf_vmlinux, array_elem->type);

strip modifiers

> +               if (!btf_type_is_struct(elem_type))
> +                       goto error;
> +
> +               off = (off - t->size) % elem_type->size;

I think it will be safer to use field offset, not struct size.
Consider example below

$ cat test-test.c
struct bla {
        long a;
        int b;
        char c[];
};

int main() {
        static struct bla *x = 0;
        return 0;
}

$ pahole -F btf -C bla test-test.o
struct bla {
        long int                   a;                    /*     0     8 */
        int                        b;                    /*     8     4 */
        char                       c[];                  /*    12     0 */

        /* size: 16, cachelines: 1, members: 3 */
        /* padding: 4 */
        /* last cacheline: 16 bytes */
};

c is at offset 12, but struct size is 16 due to long alignment. It
could be a 4-byte struct instead of char there.

> +               return btf_struct_access(log, elem_type, off, size, atype, next_btf_id);
> +
> +error:
>                 bpf_log(log, "access beyond struct %s at off %u size %u\n",
>                         tname, off, size);
>                 return -EACCES;
> --
> 2.24.1
>