On 4/22/21 11:20 AM, Andrii Nakryiko wrote:
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.
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;
+ }
[...]
