| Message ID | 20201029005902.1706310-8-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: split BTF support | expand |
> On Oct 28, 2020, at 5:58 PM, Andrii Nakryiko <andrii@kernel.org> wrote: > > Make data section layout checks stricter, disallowing overlap of types and > strings data. > > Additionally, allow BTFs with no type data. There is nothing inherently wrong > with having BTF with no types (put potentially with some strings). This could > be a situation with kernel module BTFs, if module doesn't introduce any new > type information. > > Also fix invalid offset alignment check for btf->hdr->type_off. > > Fixes: 8a138aed4a80 ("bpf: btf: Add BTF support to libbpf") > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > tools/lib/bpf/btf.c | 16 ++++++---------- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > index 20c64a8441a8..9b0ef71a03d0 100644 > --- a/tools/lib/bpf/btf.c > +++ b/tools/lib/bpf/btf.c > @@ -245,22 +245,18 @@ static int btf_parse_hdr(struct btf *btf) > return -EINVAL; > } > > - if (meta_left < hdr->type_off) { > - pr_debug("Invalid BTF type section offset:%u\n", hdr->type_off); > + if (meta_left < hdr->str_off + hdr->str_len) { > + pr_debug("Invalid BTF total size:%u\n", btf->raw_size); > return -EINVAL; > } Can we make this one as if (meta_left != hdr->str_off + hdr->str_len) { > > - if (meta_left < hdr->str_off) { > - pr_debug("Invalid BTF string section offset:%u\n", hdr->str_off); > + if (hdr->type_off + hdr->type_len > hdr->str_off) { > + pr_debug("Invalid BTF data sections layout: type data at %u + %u, strings data at %u + %u\n", > + hdr->type_off, hdr->type_len, hdr->str_off, hdr->str_len); > return -EINVAL; > } And this one if (hdr->type_off + hdr->type_len != hdr->str_off) { ? [...]
On Mon, Nov 2, 2020 at 4:51 PM Song Liu <songliubraving@fb.com> wrote: > > > > > On Oct 28, 2020, at 5:58 PM, Andrii Nakryiko <andrii@kernel.org> wrote: > > > > Make data section layout checks stricter, disallowing overlap of types and > > strings data. > > > > Additionally, allow BTFs with no type data. There is nothing inherently wrong > > with having BTF with no types (put potentially with some strings). This could > > be a situation with kernel module BTFs, if module doesn't introduce any new > > type information. > > > > Also fix invalid offset alignment check for btf->hdr->type_off. > > > > Fixes: 8a138aed4a80 ("bpf: btf: Add BTF support to libbpf") > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > tools/lib/bpf/btf.c | 16 ++++++---------- > > 1 file changed, 6 insertions(+), 10 deletions(-) > > > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > > index 20c64a8441a8..9b0ef71a03d0 100644 > > --- a/tools/lib/bpf/btf.c > > +++ b/tools/lib/bpf/btf.c > > @@ -245,22 +245,18 @@ static int btf_parse_hdr(struct btf *btf) > > return -EINVAL; > > } > > > > - if (meta_left < hdr->type_off) { > > - pr_debug("Invalid BTF type section offset:%u\n", hdr->type_off); > > + if (meta_left < hdr->str_off + hdr->str_len) { > > + pr_debug("Invalid BTF total size:%u\n", btf->raw_size); > > return -EINVAL; > > } > > Can we make this one as > if (meta_left != hdr->str_off + hdr->str_len) { That would be not forward-compatible. I.e., old libbpf loading new BTF with extra stuff after the string section. Kernel is necessarily more strict, but I'd like to keep libbpf more permissive with this. > > > > > - if (meta_left < hdr->str_off) { > > - pr_debug("Invalid BTF string section offset:%u\n", hdr->str_off); > > + if (hdr->type_off + hdr->type_len > hdr->str_off) { > > + pr_debug("Invalid BTF data sections layout: type data at %u + %u, strings data at %u + %u\n", > > + hdr->type_off, hdr->type_len, hdr->str_off, hdr->str_len); > > return -EINVAL; > > } > > And this one > if (hdr->type_off + hdr->type_len != hdr->str_off) { > > ? Similarly, libbpf could be a bit more permissive here without sacrificing correctness (at least for read-only BTF, when rewriting BTF extra data will be discarded, of course). > > [...]
> On Nov 2, 2020, at 9:18 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Nov 2, 2020 at 4:51 PM Song Liu <songliubraving@fb.com> wrote: >> >> >> >>> On Oct 28, 2020, at 5:58 PM, Andrii Nakryiko <andrii@kernel.org> wrote: >>> >>> Make data section layout checks stricter, disallowing overlap of types and >>> strings data. >>> >>> Additionally, allow BTFs with no type data. There is nothing inherently wrong >>> with having BTF with no types (put potentially with some strings). This could >>> be a situation with kernel module BTFs, if module doesn't introduce any new >>> type information. >>> >>> Also fix invalid offset alignment check for btf->hdr->type_off. >>> >>> Fixes: 8a138aed4a80 ("bpf: btf: Add BTF support to libbpf") >>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> >>> --- >>> tools/lib/bpf/btf.c | 16 ++++++---------- >>> 1 file changed, 6 insertions(+), 10 deletions(-) >>> >>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c >>> index 20c64a8441a8..9b0ef71a03d0 100644 >>> --- a/tools/lib/bpf/btf.c >>> +++ b/tools/lib/bpf/btf.c >>> @@ -245,22 +245,18 @@ static int btf_parse_hdr(struct btf *btf) >>> return -EINVAL; >>> } >>> >>> - if (meta_left < hdr->type_off) { >>> - pr_debug("Invalid BTF type section offset:%u\n", hdr->type_off); >>> + if (meta_left < hdr->str_off + hdr->str_len) { >>> + pr_debug("Invalid BTF total size:%u\n", btf->raw_size); >>> return -EINVAL; >>> } >> >> Can we make this one as >> if (meta_left != hdr->str_off + hdr->str_len) { > > That would be not forward-compatible. I.e., old libbpf loading new BTF > with extra stuff after the string section. Kernel is necessarily more > strict, but I'd like to keep libbpf more permissive with this. Yeah, this makes sense. Let's keep both checks AS-IS. Thanks, Song
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c index 20c64a8441a8..9b0ef71a03d0 100644 --- a/tools/lib/bpf/btf.c +++ b/tools/lib/bpf/btf.c @@ -245,22 +245,18 @@ static int btf_parse_hdr(struct btf *btf) return -EINVAL; } - if (meta_left < hdr->type_off) { - pr_debug("Invalid BTF type section offset:%u\n", hdr->type_off); + if (meta_left < hdr->str_off + hdr->str_len) { + pr_debug("Invalid BTF total size:%u\n", btf->raw_size); return -EINVAL; } - if (meta_left < hdr->str_off) { - pr_debug("Invalid BTF string section offset:%u\n", hdr->str_off); + if (hdr->type_off + hdr->type_len > hdr->str_off) { + pr_debug("Invalid BTF data sections layout: type data at %u + %u, strings data at %u + %u\n", + hdr->type_off, hdr->type_len, hdr->str_off, hdr->str_len); return -EINVAL; } - if (hdr->type_off >= hdr->str_off) { - pr_debug("BTF type section offset >= string section offset. No type?\n"); - return -EINVAL; - } - - if (hdr->type_off & 0x02) { + if (hdr->type_off % 4) { pr_debug("BTF type section is not aligned to 4 bytes\n"); return -EINVAL; }
Make data section layout checks stricter, disallowing overlap of types and strings data. Additionally, allow BTFs with no type data. There is nothing inherently wrong with having BTF with no types (put potentially with some strings). This could be a situation with kernel module BTFs, if module doesn't introduce any new type information. Also fix invalid offset alignment check for btf->hdr->type_off. Fixes: 8a138aed4a80 ("bpf: btf: Add BTF support to libbpf") Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- tools/lib/bpf/btf.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)