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: <20220523164626.858340-1-masahiroy@kernel.org> <20220523164626.858340-3-masahiroy@kernel.org>
In-Reply-To: <20220523164626.858340-3-masahiroy@kernel.org>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Tue, 24 May 2022 13:47:55 -0700
Message-ID: <CAKwvOdnCQUSOUHqL20nAFJ1FcUGQOSz9DWkyS5UU-8C9M-wcNA@mail.gmail.com>
Subject: Re: [PATCH 3/5] modpost: simplify mod->name allocation
To:     Masahiro Yamada <masahiroy@kernel.org>
Cc:     linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org,
        Michal Marek <michal.lkml@markovi.net>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, May 23, 2022 at 9:48 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> mod->name is set to the ELF filename with the suffix ".o" stripped.
>
> The current code calls strdup() and free() to manipulate the string,
> but a simpler approach is to pass new_module() with the name length
> subtracted by 2.
>
> Also, check if the passed filename ends with ".o" before stripping it.
>
> The current code blindly chops the suffix
>
>     tmp[strlen(tmp) - 2] = '\0'
>
> but it will cause buffer under-run if strlen(tmp) < 2;
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

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

> ---
>
>  scripts/mod/modpost.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 843c64eebe8b..77c315dea1a3 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -172,11 +172,11 @@ static struct module *find_module(const char *modname)
>         return NULL;
>  }
>
> -static struct module *new_module(const char *modname)
> +static struct module *new_module(const char *name, size_t namelen)
>  {
>         struct module *mod;
>
> -       mod = NOFAIL(malloc(sizeof(*mod) + strlen(modname) + 1));
> +       mod = NOFAIL(malloc(sizeof(*mod) + namelen + 1));
>         memset(mod, 0, sizeof(*mod));
>
>         INIT_LIST_HEAD(&mod->exported_symbols);
> @@ -184,8 +184,9 @@ static struct module *new_module(const char *modname)
>         INIT_LIST_HEAD(&mod->missing_namespaces);
>         INIT_LIST_HEAD(&mod->imported_namespaces);
>
> -       strcpy(mod->name, modname);
> -       mod->is_vmlinux = (strcmp(modname, "vmlinux") == 0);
> +       memcpy(mod->name, name, namelen);
> +       mod->name[namelen] = '\0';
> +       mod->is_vmlinux = (strcmp(mod->name, "vmlinux") == 0);
>
>         /*
>          * Set mod->is_gpl_compatible to true by default. If MODULE_LICENSE()
> @@ -2022,16 +2023,14 @@ static void read_symbols(const char *modname)
>         if (!parse_elf(&info, modname))
>                 return;
>
> -       {
> -               char *tmp;
> -
> -               /* strip trailing .o */
> -               tmp = NOFAIL(strdup(modname));
> -               tmp[strlen(tmp) - 2] = '\0';
> -               mod = new_module(tmp);
> -               free(tmp);
> +       if (!strends(modname, ".o")) {
> +               error("%s: filename must be suffixed with .o\n", modname);
> +               return;
>         }
>
> +       /* strip trailing .o */
> +       mod = new_module(modname, strlen(modname) - strlen(".o"));
> +
>         if (!mod->is_vmlinux) {
>                 license = get_modinfo(&info, "license");
>                 if (!license)
> @@ -2493,7 +2492,7 @@ static void read_dump(const char *fname)
>
>                 mod = find_module(modname);
>                 if (!mod) {
> -                       mod = new_module(modname);
> +                       mod = new_module(modname, strlen(modname));
>                         mod->from_dump = true;
>                 }
>                 s = sym_add_exported(symname, mod, gpl_only);
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers