| 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: <eee61147-90a6-b1fb-5278-beb61afe7784@fb.com>
Date: Mon, 21 May 2018 13:15:24 -0700
From: Yonghong Song <yhs@...com>
To: Martin KaFai Lau <kafai@...com>, <netdev@...r.kernel.org>
CC: Alexei Starovoitov <ast@...com>,
Daniel Borkmann <daniel@...earbox.net>, <kernel-team@...com>
Subject: Re: [PATCH bpf-next 2/7] bpf: btf: Change how section is supported in
btf_header
On 5/18/18 5:16 PM, Martin KaFai Lau wrote:
> There are currently unused section descriptions in the btf_header. Those
> sections are here to support future BTF use cases. For example, the
> func section (func_off) is to support function signature (e.g. the BPF
> prog function signature).
>
> Instead of spelling out all potential sections up-front in the btf_header.
> This patch makes changes to btf_header such that extending it (e.g. adding
> a section) is possible later. The unused ones can be removed for now and
> they can be added back later.
>
> This patch:
> 1. adds a hdr_len to the btf_header. It will allow adding
> sections (and other info like parent_label and parent_name)
> later. The check is similar to the existing bpf_attr.
> If a user passes in a longer hdr_len, the kernel
> ensures the extra tailing bytes are 0.
>
> 2. allows the section order in the BTF object to be
> different from its sec_off order in btf_header.
>
> 3. each sec_off is followed by a sec_len. It must not have gap or
> overlapping among sections.
>
> The string section is ensured to be at the end due to the 4 bytes
> alignment requirement of the type section.
>
> The above changes will allow enough flexibility to
> add new sections (and other info) to the btf_header later.
>
> This patch also removes an unnecessary !err check
> at the end of btf_parse().
>
> Signed-off-by: Martin KaFai Lau <kafai@...com>
> ---
> include/uapi/linux/btf.h | 8 +-
> kernel/bpf/btf.c | 207 +++++++++++++++++++++++++++++++++++------------
> 2 files changed, 158 insertions(+), 57 deletions(-)
>
> diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h
> index bcb56ee47014..4fa479741a02 100644
> --- a/include/uapi/linux/btf.h
> +++ b/include/uapi/linux/btf.h
> @@ -12,15 +12,11 @@ struct btf_header {
> __u16 magic;
> __u8 version;
> __u8 flags;
> -
> - __u32 parent_label;
> - __u32 parent_name;
> + __u32 hdr_len;
>
> /* All offsets are in bytes relative to the end of this header */
> - __u32 label_off; /* offset of label section */
> - __u32 object_off; /* offset of data object section*/
> - __u32 func_off; /* offset of function section */
> __u32 type_off; /* offset of type section */
> + __u32 type_len; /* length of type section */
> __u32 str_off; /* offset of string section */
> __u32 str_len; /* length of string section */
> };
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index ded10ab47b8a..536e5981ad8c 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -12,6 +12,7 @@
> #include <linux/uaccess.h>
> #include <linux/kernel.h>
> #include <linux/idr.h>
> +#include <linux/sort.h>
> #include <linux/bpf_verifier.h>
> #include <linux/btf.h>
>
> @@ -184,15 +185,13 @@ static DEFINE_IDR(btf_idr);
> static DEFINE_SPINLOCK(btf_idr_lock);
>
> struct btf {
> - union {
> - struct btf_header *hdr;
> - void *data;
> - };
> + void *data;
> struct btf_type **types;
> u32 *resolved_ids;
> u32 *resolved_sizes;
> const char *strings;
> void *nohdr_data;
> + struct btf_header hdr;
> u32 nr_types;
> u32 types_size;
> u32 data_size;
> @@ -227,6 +226,12 @@ enum resolve_mode {
> };
>
> #define MAX_RESOLVE_DEPTH 32
> +#define NR_SECS 2
Not sure whether it is necessary to define NR_SECS 2 here or not.
See below.
> +
> +struct btf_sec_info {
> + u32 off;
> + u32 len;
> +};
>
> struct btf_verifier_env {
> struct btf *btf;
> @@ -418,14 +423,14 @@ static const struct btf_kind_operations *btf_type_ops(const struct btf_type *t)
> static bool btf_name_offset_valid(const struct btf *btf, u32 offset)
> {
> return !BTF_STR_TBL_ELF_ID(offset) &&
> - BTF_STR_OFFSET(offset) < btf->hdr->str_len;
> + BTF_STR_OFFSET(offset) < btf->hdr.str_len;
> }
>
> static const char *btf_name_by_offset(const struct btf *btf, u32 offset)
> {
> if (!BTF_STR_OFFSET(offset))
> return "(anon)";
> - else if (BTF_STR_OFFSET(offset) < btf->hdr->str_len)
> + else if (BTF_STR_OFFSET(offset) < btf->hdr.str_len)
> return &btf->strings[BTF_STR_OFFSET(offset)];
> else
> return "(invalid-name-offset)";
> @@ -536,7 +541,8 @@ static void btf_verifier_log_member(struct btf_verifier_env *env,
> __btf_verifier_log(log, "\n");
> }
>
> -static void btf_verifier_log_hdr(struct btf_verifier_env *env)
> +static void btf_verifier_log_hdr(struct btf_verifier_env *env,
> + u32 btf_data_size)
> {
> struct bpf_verifier_log *log = &env->log;
> const struct btf *btf = env->btf;
> @@ -545,19 +551,16 @@ static void btf_verifier_log_hdr(struct btf_verifier_env *env)
> if (!bpf_verifier_log_needed(log))
> return;
>
> - hdr = btf->hdr;
> + hdr = &btf->hdr;
> __btf_verifier_log(log, "magic: 0x%x\n", hdr->magic);
> __btf_verifier_log(log, "version: %u\n", hdr->version);
> __btf_verifier_log(log, "flags: 0x%x\n", hdr->flags);
> - __btf_verifier_log(log, "parent_label: %u\n", hdr->parent_label);
> - __btf_verifier_log(log, "parent_name: %u\n", hdr->parent_name);
> - __btf_verifier_log(log, "label_off: %u\n", hdr->label_off);
> - __btf_verifier_log(log, "object_off: %u\n", hdr->object_off);
> - __btf_verifier_log(log, "func_off: %u\n", hdr->func_off);
> + __btf_verifier_log(log, "hdr_len: %u\n", hdr->hdr_len);
> __btf_verifier_log(log, "type_off: %u\n", hdr->type_off);
> + __btf_verifier_log(log, "type_len: %u\n", hdr->type_len);
> __btf_verifier_log(log, "str_off: %u\n", hdr->str_off);
> __btf_verifier_log(log, "str_len: %u\n", hdr->str_len);
> - __btf_verifier_log(log, "btf_total_size: %u\n", btf->data_size);
> + __btf_verifier_log(log, "btf_total_size: %u\n", btf_data_size);
> }
>
> static int btf_add_type(struct btf_verifier_env *env, struct btf_type *t)
> @@ -1754,9 +1757,9 @@ static int btf_check_all_metas(struct btf_verifier_env *env)
> struct btf_header *hdr;
> void *cur, *end;
>
> - hdr = btf->hdr;
> + hdr = &btf->hdr;
> cur = btf->nohdr_data + hdr->type_off;
> - end = btf->nohdr_data + hdr->str_off;
> + end = btf->nohdr_data + hdr->type_len;
>
> env->log_type_id = 1;
> while (cur < end) {
> @@ -1866,8 +1869,20 @@ static int btf_check_all_types(struct btf_verifier_env *env)
>
> static int btf_parse_type_sec(struct btf_verifier_env *env)
> {
> + const struct btf_header *hdr = &env->btf->hdr;
> int err;
>
> + /* Type section must align to 4 bytes */
> + if (hdr->type_off & (sizeof(u32) - 1)) {
> + btf_verifier_log(env, "Unaligned type_off");
> + return -EINVAL;
> + }
> +
> + if (!hdr->type_len) {
> + btf_verifier_log(env, "No type found");
> + return -EINVAL;
> + }
> +
> err = btf_check_all_metas(env);
> if (err)
> return err;
> @@ -1881,10 +1896,15 @@ static int btf_parse_str_sec(struct btf_verifier_env *env)
> struct btf *btf = env->btf;
> const char *start, *end;
>
> - hdr = btf->hdr;
> + hdr = &btf->hdr;
> start = btf->nohdr_data + hdr->str_off;
> end = start + hdr->str_len;
>
> + if (end != btf->data + btf->data_size) {
> + btf_verifier_log(env, "String section is not at the end");
> + return -EINVAL;
> + }
> +
> if (!hdr->str_len || hdr->str_len - 1 > BTF_MAX_NAME_OFFSET ||
> start[0] || end[-1]) {
> btf_verifier_log(env, "Invalid string section");
> @@ -1896,20 +1916,119 @@ static int btf_parse_str_sec(struct btf_verifier_env *env)
> return 0;
> }
>
> -static int btf_parse_hdr(struct btf_verifier_env *env)
> +static const size_t btf_sec_info_offset[] = {
> + offsetof(struct btf_header, type_off),
> + offsetof(struct btf_header, str_off),
> +};
Maybe we can define NR_SECS is
ARRAY_SIZE(btf_sec_info_offset)/sizeof(size_t)?
> +
> +static int btf_sec_info_cmp(const void *a, const void *b)
> {
> + const struct btf_sec_info *x = a;
> + const struct btf_sec_info *y = b;
> +
> + return (int)(x->off - y->off) ? : (int)(x->len - y->len);
> +}
> +
> +static int btf_check_sec_info(struct btf_verifier_env *env,
> + u32 btf_data_size)
> +{
> + struct btf_sec_info secs[NR_SECS];
> + u32 total, expected_total, i;
> const struct btf_header *hdr;
> - struct btf *btf = env->btf;
> - u32 meta_left;
> + const struct btf *btf;
> +
> + BUILD_BUG_ON(ARRAY_SIZE(btf_sec_info_offset) != NR_SECS);
If we define NR_SECS depending on the size of btf_sec_info_offset, this
BUILD_BUG_ON is not needed.
> +
> + btf = env->btf;
> + hdr = &btf->hdr;
> +
> + /* Populate the secs from hdr */
> + for (i = 0; i < NR_SECS; i++)
> + secs[i] = *(struct btf_sec_info *)((void *)hdr +
> + btf_sec_info_offset[i]);
> +
> + sort(secs, NR_SECS, sizeof(struct btf_sec_info),
> + btf_sec_info_cmp, NULL);
> +
> + /* Check for gaps and overlap among sections */
> + total = 0;
> + expected_total = btf_data_size - hdr->hdr_len;
> + for (i = 0; i < NR_SECS; i++) {
> + if (expected_total < secs[i].off) {
> + btf_verifier_log(env, "Invalid section offset");
> + return -EINVAL;
> + }
> + if (total < secs[i].off) {
> + /* gap */
> + btf_verifier_log(env, "Unsupported section found");
> + return -EINVAL;
> + }
> + if (total > secs[i].off) {
> + btf_verifier_log(env, "Section overlap found");
> + return -EINVAL;
> + }
> + if (expected_total - total < secs[i].len) {
> + btf_verifier_log(env,
> + "Total section length too long");
> + return -EINVAL;
> + }
> + total += secs[i].len;
> + }
> +
> + /* There is data other than hdr and known sections */
> + if (expected_total != total) {
> + btf_verifier_log(env, "Unsupported section found");
> + return -EINVAL;
> + }
Does this patch try to address compatibility issue? That is, the old btf
format with 2 sections should still work with future kernel with more
than 2 sections? The above comparision seems suggesting that newer
kernel will not support non-compatible number of sections?
> +
> + return 0;
> +}
> +
> +static int btf_parse_hdr(struct btf_verifier_env *env, void __user *btf_data,
> + u32 btf_data_size)
> +{
> + const struct btf_header *hdr;
> + u32 hdr_len, hdr_copy;
> + struct btf_min_header {
> + u16 magic;
> + u8 version;
> + u8 flags;
> + u32 hdr_len;
> + } __user *min_hdr;
This is the partial structure with the first four fields
for struct btf_header.
It is okay, but I feel a little bit duplication here.
Looks like only sizeof(*min_hdr) and min_hdr->hdr_len is used.
Maybe we can just cast bpf_data to bpf_header and
sizeof(*min_hdr) being offsetof(bpf_data, type_off), and
min_hdr->hdr_len being bpf_data->hdr_len.
Do this work?
> + struct btf *btf;
> + int err;
> +
> + btf = env->btf;
> + min_hdr = btf_data;
> +
> + if (btf_data_size < sizeof(*min_hdr)) {
> + btf_verifier_log(env, "hdr_len not found");
> + return -EINVAL;
> + }
>
> - if (btf->data_size < sizeof(*hdr)) {
> + if (get_user(hdr_len, &min_hdr->hdr_len))
> + return -EFAULT;
> +
> + if (btf_data_size < hdr_len) {
> btf_verifier_log(env, "btf_header not found");
> return -EINVAL;
> }
>
> - btf_verifier_log_hdr(env);
> + err = bpf_check_uarg_tail_zero(btf_data, sizeof(btf->hdr), hdr_len);
> + if (err) {
> + if (err == -E2BIG)
> + btf_verifier_log(env, "Unsupported btf_header");
> + return err;
> + }
> +
> + hdr_copy = min_t(u32, hdr_len, sizeof(btf->hdr));
> + if (copy_from_user(&btf->hdr, btf_data, hdr_copy))
> + return -EFAULT;
> +
> + hdr = &btf->hdr;
> +
> + btf_verifier_log_hdr(env, btf_data_size);
>
> - hdr = btf->hdr;
> if (hdr->magic != BTF_MAGIC) {
> btf_verifier_log(env, "Invalid magic");
> return -EINVAL;
> @@ -1925,26 +2044,14 @@ static int btf_parse_hdr(struct btf_verifier_env *env)
> return -ENOTSUPP;
> }
>
> - meta_left = btf->data_size - sizeof(*hdr);
> - if (!meta_left) {
> + if (btf_data_size == hdr->hdr_len) {
> btf_verifier_log(env, "No data");
> return -EINVAL;
> }
>
> - if (meta_left < hdr->type_off || hdr->str_off <= hdr->type_off ||
> - /* Type section must align to 4 bytes */
> - hdr->type_off & (sizeof(u32) - 1)) {
> - btf_verifier_log(env, "Invalid type_off");
> - return -EINVAL;
> - }
> -
> - if (meta_left < hdr->str_off ||
> - meta_left - hdr->str_off < hdr->str_len) {
> - btf_verifier_log(env, "Invalid str_off or str_len");
> - return -EINVAL;
> - }
> -
> - btf->nohdr_data = btf->hdr + 1;
> + err = btf_check_sec_info(env, btf_data_size);
> + if (err)
> + return err;
>
> return 0;
> }
> @@ -1987,6 +2094,11 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
> err = -ENOMEM;
> goto errout;
> }
> + env->btf = btf;
> +
> + err = btf_parse_hdr(env, btf_data, btf_data_size);
> + if (err)
> + goto errout;
>
> data = kvmalloc(btf_data_size, GFP_KERNEL | __GFP_NOWARN);
> if (!data) {
> @@ -1996,18 +2108,13 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
>
> btf->data = data;
> btf->data_size = btf_data_size;
> + btf->nohdr_data = btf->data + btf->hdr.hdr_len;
>
> if (copy_from_user(data, btf_data, btf_data_size)) {
> err = -EFAULT;
> goto errout;
> }
>
> - env->btf = btf;
> -
> - err = btf_parse_hdr(env);
> - if (err)
> - goto errout;
> -
> err = btf_parse_str_sec(env);
> if (err)
> goto errout;
> @@ -2016,16 +2123,14 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
> if (err)
> goto errout;
>
> - if (!err && log->level && bpf_verifier_log_full(log)) {
> + if (log->level && bpf_verifier_log_full(log)) {
> err = -ENOSPC;
> goto errout;
> }
>
> - if (!err) {
> - btf_verifier_env_free(env);
> - refcount_set(&btf->refcnt, 1);
> - return btf;
> - }
> + btf_verifier_env_free(env);
> + refcount_set(&btf->refcnt, 1);
> + return btf;
>
> errout:
> btf_verifier_env_free(env);
>
Powered by blists - more mailing lists