On Mon, Nov 2, 2020 at 4:51 PM Song Liu [off-list ref] wrote:

quoted

On Oct 28, 2020, at 5:58 PM, Andrii Nakryiko [off-list ref] 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.

quoted

-     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).

[...]