| Message ID | 20210416202404.3443623-10-andrii@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | BPF static linker: support externs | expand |
On 4/16/21 1:23 PM, Andrii Nakryiko wrote: > Add logic to validate extern symbols, plus some other minor extra checks, like > ELF symbol #0 validation, general symbol visibility and binding validations. > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > tools/lib/bpf/linker.c | 43 +++++++++++++++++++++++++++++++++--------- > 1 file changed, 34 insertions(+), 9 deletions(-) > > diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c > index 1263641e8b97..283249df9831 100644 > --- a/tools/lib/bpf/linker.c > +++ b/tools/lib/bpf/linker.c > @@ -750,14 +750,39 @@ static int linker_sanity_check_elf_symtab(struct src_obj *obj, struct src_sec *s > n = sec->shdr->sh_size / sec->shdr->sh_entsize; > sym = sec->data->d_buf; > for (i = 0; i < n; i++, sym++) { > - if (sym->st_shndx > - && sym->st_shndx < SHN_LORESERVE > - && sym->st_shndx >= obj->sec_cnt) { > + int sym_type = ELF64_ST_TYPE(sym->st_info); > + int sym_bind = ELF64_ST_BIND(sym->st_info); > + > + if (i == 0) { > + if (sym->st_name != 0 || sym->st_info != 0 > + || sym->st_other != 0 || sym->st_shndx != 0 > + || sym->st_value != 0 || sym->st_size != 0) { > + pr_warn("ELF sym #0 is invalid in %s\n", obj->filename); > + return -EINVAL; > + } > + continue; > + } In ELF file, the first entry of symbol table and section table (index 0) is invalid/undefined. Symbol table '.symtab' contains 9 entries: Num: Value Size Type Bind Vis Ndx Name 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND Section Headers: [Nr] Name Type Address Off Size ES Flg Lk Inf Al [ 0] NULL 0000000000000000 000000 000000 00 0 0 0 Instead of validating them, I think we can skip traversal of the index = 0 entry for symbol table and section header table. What do you think? > + if (sym_bind != STB_LOCAL && sym_bind != STB_GLOBAL && sym_bind != STB_WEAK) { > + pr_warn("ELF sym #%d is section #%zu has unsupported symbol binding %d\n", > + i, sec->sec_idx, sym_bind); > + return -EINVAL; > + } > + if (sym->st_shndx == 0) { > + if (sym_type != STT_NOTYPE || sym_bind == STB_LOCAL > + || sym->st_value != 0 || sym->st_size != 0) { > + pr_warn("ELF sym #%d is invalid extern symbol in %s\n", > + i, obj->filename); > + > + return -EINVAL; > + } > + continue; > + } > + if (sym->st_shndx < SHN_LORESERVE && sym->st_shndx >= obj->sec_cnt) { > pr_warn("ELF sym #%d in section #%zu points to missing section #%zu in %s\n", > i, sec->sec_idx, (size_t)sym->st_shndx, obj->filename); > return -EINVAL; > } > - if (ELF64_ST_TYPE(sym->st_info) == STT_SECTION) { > + if (sym_type == STT_SECTION) { > if (sym->st_value != 0) > return -EINVAL; > continue; > @@ -1135,16 +1160,16 @@ static int linker_append_elf_syms(struct bpf_linker *linker, struct src_obj *obj > size_t dst_sym_idx; > int name_off; > > - /* we already have all-zero initial symbol */ > - if (sym->st_name == 0 && sym->st_info == 0 && > - sym->st_other == 0 && sym->st_shndx == SHN_UNDEF && > - sym->st_value == 0 && sym->st_size ==0) > + /* We already validated all-zero symbol #0 and we already > + * appended it preventively to the final SYMTAB, so skip it. > + */ > + if (i == 0) > continue; > > sym_name = elf_strptr(obj->elf, str_sec_idx, sym->st_name); > if (!sym_name) { > pr_warn("can't fetch symbol name for symbol #%d in '%s'\n", i, obj->filename); > - return -1; > + return -EINVAL; > } > > if (sym->st_shndx && sym->st_shndx < SHN_LORESERVE) { >
On Thu, Apr 22, 2021 at 9:35 AM Yonghong Song <yhs@fb.com> wrote: > > > > On 4/16/21 1:23 PM, Andrii Nakryiko wrote: > > Add logic to validate extern symbols, plus some other minor extra checks, like > > ELF symbol #0 validation, general symbol visibility and binding validations. > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > tools/lib/bpf/linker.c | 43 +++++++++++++++++++++++++++++++++--------- > > 1 file changed, 34 insertions(+), 9 deletions(-) > > > > diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c > > index 1263641e8b97..283249df9831 100644 > > --- a/tools/lib/bpf/linker.c > > +++ b/tools/lib/bpf/linker.c > > @@ -750,14 +750,39 @@ static int linker_sanity_check_elf_symtab(struct src_obj *obj, struct src_sec *s > > n = sec->shdr->sh_size / sec->shdr->sh_entsize; > > sym = sec->data->d_buf; > > for (i = 0; i < n; i++, sym++) { > > - if (sym->st_shndx > > - && sym->st_shndx < SHN_LORESERVE > > - && sym->st_shndx >= obj->sec_cnt) { > > + int sym_type = ELF64_ST_TYPE(sym->st_info); > > + int sym_bind = ELF64_ST_BIND(sym->st_info); > > + > > + if (i == 0) { > > + if (sym->st_name != 0 || sym->st_info != 0 > > + || sym->st_other != 0 || sym->st_shndx != 0 > > + || sym->st_value != 0 || sym->st_size != 0) { > > + pr_warn("ELF sym #0 is invalid in %s\n", obj->filename); > > + return -EINVAL; > > + } > > + continue; > > + } > > In ELF file, the first entry of symbol table and section table (index 0) > is invalid/undefined. > > Symbol table '.symtab' contains 9 entries: > Num: Value Size Type Bind Vis Ndx Name > 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND > > Section Headers: > > [Nr] Name Type Address Off Size > ES Flg Lk Inf Al > [ 0] NULL 0000000000000000 000000 000000 > 00 0 0 0 > > Instead of validating them, I think we can skip traversal of the index = > 0 entry for symbol table and section header table. What do you think? In valid ELF yes. But then entire sanity check logic is not needed if we just assume correct ELF. But I don't want to make that potentially dangerous assumption :) Here I'm validating that ELF is sane with minimal efforts. I do skip symbol #0 later because I validated that it's all-zero one, as expected by ELF standard. > > > + if (sym_bind != STB_LOCAL && sym_bind != STB_GLOBAL && sym_bind != STB_WEAK) { > > + pr_warn("ELF sym #%d is section #%zu has unsupported symbol binding %d\n", > > + i, sec->sec_idx, sym_bind); > > + return -EINVAL; > > + } > > + if (sym->st_shndx == 0) { > > + if (sym_type != STT_NOTYPE || sym_bind == STB_LOCAL > > + || sym->st_value != 0 || sym->st_size != 0) { > > + pr_warn("ELF sym #%d is invalid extern symbol in %s\n", > > + i, obj->filename); > > + > > + return -EINVAL; > > + } > > + continue; > > + } > > + if (sym->st_shndx < SHN_LORESERVE && sym->st_shndx >= obj->sec_cnt) { > > pr_warn("ELF sym #%d in section #%zu points to missing section #%zu in %s\n", > > i, sec->sec_idx, (size_t)sym->st_shndx, obj->filename); > > return -EINVAL; > > } > > - if (ELF64_ST_TYPE(sym->st_info) == STT_SECTION) { > > + if (sym_type == STT_SECTION) { > > if (sym->st_value != 0) > > return -EINVAL; > > continue; > > @@ -1135,16 +1160,16 @@ static int linker_append_elf_syms(struct bpf_linker *linker, struct src_obj *obj > > size_t dst_sym_idx; > > int name_off; > > > > - /* we already have all-zero initial symbol */ > > - if (sym->st_name == 0 && sym->st_info == 0 && > > - sym->st_other == 0 && sym->st_shndx == SHN_UNDEF && > > - sym->st_value == 0 && sym->st_size ==0) > > + /* We already validated all-zero symbol #0 and we already > > + * appended it preventively to the final SYMTAB, so skip it. > > + */ > > + if (i == 0) > > continue; > > > > sym_name = elf_strptr(obj->elf, str_sec_idx, sym->st_name); > > if (!sym_name) { > > pr_warn("can't fetch symbol name for symbol #%d in '%s'\n", i, obj->filename); > > - return -1; > > + return -EINVAL; > > } > > > > if (sym->st_shndx && sym->st_shndx < SHN_LORESERVE) { > >
On 4/22/21 11:20 AM, Andrii Nakryiko wrote: > On Thu, Apr 22, 2021 at 9:35 AM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 4/16/21 1:23 PM, Andrii Nakryiko wrote: >>> Add logic to validate extern symbols, plus some other minor extra checks, like >>> ELF symbol #0 validation, general symbol visibility and binding validations. >>> >>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> >>> --- >>> tools/lib/bpf/linker.c | 43 +++++++++++++++++++++++++++++++++--------- >>> 1 file changed, 34 insertions(+), 9 deletions(-) >>> >>> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c >>> index 1263641e8b97..283249df9831 100644 >>> --- a/tools/lib/bpf/linker.c >>> +++ b/tools/lib/bpf/linker.c >>> @@ -750,14 +750,39 @@ static int linker_sanity_check_elf_symtab(struct src_obj *obj, struct src_sec *s >>> n = sec->shdr->sh_size / sec->shdr->sh_entsize; >>> sym = sec->data->d_buf; >>> for (i = 0; i < n; i++, sym++) { >>> - if (sym->st_shndx >>> - && sym->st_shndx < SHN_LORESERVE >>> - && sym->st_shndx >= obj->sec_cnt) { >>> + int sym_type = ELF64_ST_TYPE(sym->st_info); >>> + int sym_bind = ELF64_ST_BIND(sym->st_info); >>> + >>> + if (i == 0) { >>> + if (sym->st_name != 0 || sym->st_info != 0 >>> + || sym->st_other != 0 || sym->st_shndx != 0 >>> + || sym->st_value != 0 || sym->st_size != 0) { >>> + pr_warn("ELF sym #0 is invalid in %s\n", obj->filename); >>> + return -EINVAL; >>> + } >>> + continue; >>> + } >> >> In ELF file, the first entry of symbol table and section table (index 0) >> is invalid/undefined. >> >> Symbol table '.symtab' contains 9 entries: >> Num: Value Size Type Bind Vis Ndx Name >> 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND >> >> Section Headers: >> >> [Nr] Name Type Address Off Size >> ES Flg Lk Inf Al >> [ 0] NULL 0000000000000000 000000 000000 >> 00 0 0 0 >> >> Instead of validating them, I think we can skip traversal of the index = >> 0 entry for symbol table and section header table. What do you think? > > In valid ELF yes. But then entire sanity check logic is not needed if > we just assume correct ELF. But I don't want to make that potentially > dangerous assumption :) Here I'm validating that ELF is sane with > minimal efforts. I do skip symbol #0 later because I validated that > it's all-zero one, as expected by ELF standard. I just feel we can ignore entry 0 regardless of its contents. But I guess this is still useful since libbpf linker generates its own ELF file and it is worthwhile to do an in-depth check for that. So I am okay with this. > >> >>> + if (sym_bind != STB_LOCAL && sym_bind != STB_GLOBAL && sym_bind != STB_WEAK) { >>> + pr_warn("ELF sym #%d is section #%zu has unsupported symbol binding %d\n", >>> + i, sec->sec_idx, sym_bind); >>> + return -EINVAL; >>> + } >>> + if (sym->st_shndx == 0) { >>> + if (sym_type != STT_NOTYPE || sym_bind == STB_LOCAL >>> + || sym->st_value != 0 || sym->st_size != 0) { >>> + pr_warn("ELF sym #%d is invalid extern symbol in %s\n", >>> + i, obj->filename); >>> + >>> + return -EINVAL; >>> + } >>> + continue; >>> + } [...]
diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c index 1263641e8b97..283249df9831 100644 --- a/tools/lib/bpf/linker.c +++ b/tools/lib/bpf/linker.c @@ -750,14 +750,39 @@ static int linker_sanity_check_elf_symtab(struct src_obj *obj, struct src_sec *s n = sec->shdr->sh_size / sec->shdr->sh_entsize; sym = sec->data->d_buf; for (i = 0; i < n; i++, sym++) { - if (sym->st_shndx - && sym->st_shndx < SHN_LORESERVE - && sym->st_shndx >= obj->sec_cnt) { + int sym_type = ELF64_ST_TYPE(sym->st_info); + int sym_bind = ELF64_ST_BIND(sym->st_info); + + if (i == 0) { + if (sym->st_name != 0 || sym->st_info != 0 + || sym->st_other != 0 || sym->st_shndx != 0 + || sym->st_value != 0 || sym->st_size != 0) { + pr_warn("ELF sym #0 is invalid in %s\n", obj->filename); + return -EINVAL; + } + continue; + } + if (sym_bind != STB_LOCAL && sym_bind != STB_GLOBAL && sym_bind != STB_WEAK) { + pr_warn("ELF sym #%d is section #%zu has unsupported symbol binding %d\n", + i, sec->sec_idx, sym_bind); + return -EINVAL; + } + if (sym->st_shndx == 0) { + if (sym_type != STT_NOTYPE || sym_bind == STB_LOCAL + || sym->st_value != 0 || sym->st_size != 0) { + pr_warn("ELF sym #%d is invalid extern symbol in %s\n", + i, obj->filename); + + return -EINVAL; + } + continue; + } + if (sym->st_shndx < SHN_LORESERVE && sym->st_shndx >= obj->sec_cnt) { pr_warn("ELF sym #%d in section #%zu points to missing section #%zu in %s\n", i, sec->sec_idx, (size_t)sym->st_shndx, obj->filename); return -EINVAL; } - if (ELF64_ST_TYPE(sym->st_info) == STT_SECTION) { + if (sym_type == STT_SECTION) { if (sym->st_value != 0) return -EINVAL; continue; @@ -1135,16 +1160,16 @@ static int linker_append_elf_syms(struct bpf_linker *linker, struct src_obj *obj size_t dst_sym_idx; int name_off; - /* we already have all-zero initial symbol */ - if (sym->st_name == 0 && sym->st_info == 0 && - sym->st_other == 0 && sym->st_shndx == SHN_UNDEF && - sym->st_value == 0 && sym->st_size ==0) + /* We already validated all-zero symbol #0 and we already + * appended it preventively to the final SYMTAB, so skip it. + */ + if (i == 0) continue; sym_name = elf_strptr(obj->elf, str_sec_idx, sym->st_name); if (!sym_name) { pr_warn("can't fetch symbol name for symbol #%d in '%s'\n", i, obj->filename); - return -1; + return -EINVAL; } if (sym->st_shndx && sym->st_shndx < SHN_LORESERVE) {
Add logic to validate extern symbols, plus some other minor extra checks, like ELF symbol #0 validation, general symbol visibility and binding validations. Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- tools/lib/bpf/linker.c | 43 +++++++++++++++++++++++++++++++++--------- 1 file changed, 34 insertions(+), 9 deletions(-)