diff mbox series

[v2,bpf-next,09/17] libbpf: extend sanity checking ELF symbols with externs validation

Message ID 20210416202404.3443623-10-andrii@kernel.org (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series BPF static linker: support externs | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count fail Series longer than 15 patches
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 6 maintainers not CCed: yhs@fb.com kpsingh@kernel.org kafai@fb.com ast@kernel.org john.fastabend@gmail.com songliubraving@fb.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning CHECK: Logical continuations should be on the previous line WARNING: line length of 85 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns WARNING: line length of 94 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Andrii Nakryiko April 16, 2021, 8:23 p.m. UTC 
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(-)

Comments

Yonghong Song April 22, 2021, 4:35 p.m. UTC | #1 
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) {
>
Andrii Nakryiko April 22, 2021, 6:20 p.m. UTC | #2 
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) {
> >
Yonghong Song April 22, 2021, 11:13 p.m. UTC | #3 
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 mbox series

Patch

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