On Mon, Nov 2, 2020 at 4:51 PM Song Liu <songliubraving@xxxxxx> wrote:
>
>
>
> > On Oct 28, 2020, at 5:58 PM, Andrii Nakryiko <andrii@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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).
>
> [...]
