On Fri, Jun 10, 2022 at 11:34 AM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
>
> There are 5 callsites of sec_name(). In all the places, sec_name() is
> used together with get_secindex().
>
> So, it is simpler to merge two function calls
>
> sec_name(elf, get_secindex(elf, sym))
>
> into one call:
>
> sec_name_of_symbol(elf, sym)
>
> While I was here, I also inserted this array range check:
>
> if (secindex >= info->num_sections)
> return "";
>
> This will make the code robust against info->sechdrs[] overrun.
>
> sym->st_shndx is 2 bytes (for both 32 and 64 bit systems), and the
> range 0xff00..0xffff is reserved for special sections.
>
> For example, a symbol specifies an absolute value, sym->st_shndx==0xfff1.
> get_secindex() remaps it to 0xfffffff1.
>
> There is no corresponding section header for such special sections.
>
> The existing code does not hit this issue, but it is better to check
> the array range.
>
> Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> ---
>
> scripts/mod/modpost.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 620dc8c4c814..b9f2a040f185 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -339,8 +339,19 @@ static const char *sech_name(const struct elf_info *info, Elf_Shdr *sechdr)
> sechdr->sh_name);
> }
>
> -static const char *sec_name(const struct elf_info *info, int secindex)
> +static const char *sec_name_of_symbol(const struct elf_info *info,
> + const Elf_Sym *sym)
> {
> + unsigned int secindex = get_secindex(info, sym);
> +
> + /*
> + * If sym->st_shndx is within the special section range, get_secindex()
> + * will remapit to a big number.
> + * Bail out here, otherwise info->sechdrs[secindex] would overrun.
> + */
> + if (secindex >= info->num_sections)
> + return "";
> +
> return sech_name(info, &info->sechdrs[secindex]);
> }
>
> @@ -649,7 +660,7 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
> const char *name, *secname;
>
> name = symname + strlen("__ksymtab_");
> - secname = sec_name(info, get_secindex(info, sym));
> + secname = sec_name_of_symbol(info, sym);
>
> if (strstarts(secname, "___ksymtab_gpl+"))
> sym_add_exported(name, mod, true);
> @@ -1217,7 +1228,7 @@ static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
>
> if (is_shndx_special(sym->st_shndx))
> continue;
> - symsec = sec_name(elf, get_secindex(elf, sym));
> + symsec = sec_name_of_symbol(elf, sym);
> if (strcmp(symsec, sec) != 0)
> continue;
> if (!is_valid_name(elf, sym))
> @@ -1457,7 +1468,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> if (strstarts(fromsym, "reference___initcall"))
> return;
>
> - tosec = sec_name(elf, get_secindex(elf, sym));
> + tosec = sec_name_of_symbol(elf, sym);
> to = find_elf_symbol(elf, r->r_addend, sym);
> tosym = sym_name(elf, to);
>
> @@ -1559,7 +1570,7 @@ static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
> Elf_Rela* r, Elf_Sym* sym,
> const char *fromsec)
> {
> - const char* tosec = sec_name(elf, get_secindex(elf, sym));
> + const char *tosec = sec_name_of_symbol(elf, sym);
>
> sec_mismatch_count++;
>
> @@ -1593,7 +1604,7 @@ static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
> static void check_section_mismatch(const char *modname, struct elf_info *elf,
> Elf_Rela *r, Elf_Sym *sym, const char *fromsec)
> {
> - const char *tosec = sec_name(elf, get_secindex(elf, sym));
> + const char *tosec = sec_name_of_symbol(elf, sym);
> const struct sectioncheck *mismatch = section_mismatch(fromsec, tosec);
>
> if (mismatch) {
> --
> 2.32.0
>
--
Thanks,
~Nick Desaulniers
