On Thu, Apr 22, 2021 at 9:35 AM Yonghong Song <yhs@xxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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) {
> >
