Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220610183236.1272216-1-masahiroy@kernel.org> <20220610183236.1272216-3-masahiroy@kernel.org>
In-Reply-To: <20220610183236.1272216-3-masahiroy@kernel.org>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Fri, 10 Jun 2022 14:41:52 -0700
Message-ID: <CAKwvOdn+fYiiU=x7tqxp68Zkb3E88suEg6oNNucom3AehqayJA@mail.gmail.com>
Subject: Re: [PATCH 2/7] modpost: put get_secindex() call inside sec_name()
To:     Masahiro Yamada <masahiroy@kernel.org>
Cc:     Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Nicolas Pitre <npitre@baylibre.com>,
        Luis Chamberlain <mcgrof@kernel.org>,
        linux-modules <linux-modules@vger.kernel.org>,
        Ard Biesheuvel <ardb@kernel.org>,
        Michal Marek <michal.lkml@markovi.net>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Jun 10, 2022 at 11:34 AM Masahiro Yamada <masahiroy@kernel.org> 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@kernel.org>

Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

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