netdev - Re: [PATCH net-next v2 4/4] kernel/module: Hide vermagic header file from general use

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK7LNASdOf0inF_-f8Gn7_mn1QSdXEi1HTR2zj3DEs38sf96xA@mail.gmail.com>
Date:   Mon, 20 Apr 2020 07:59:35 +0900
From:   Masahiro Yamada <masahiroy@...nel.org>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Leon Romanovsky <leonro@...lanox.com>,
        Borislav Petkov <bp@...e.de>, Jessica Yu <jeyu@...nel.org>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Michal Marek <michal.lkml@...kovi.net>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 4/4] kernel/module: Hide vermagic header file
 from general use

Hi,

On Mon, Apr 20, 2020 at 12:55 AM Leon Romanovsky <leon@...nel.org> wrote:
>
> From: Leon Romanovsky <leonro@...lanox.com>
>
> VERMAGIC* definitions are not supposed to be used by the drivers,
> see this [1] bug report, so introduce special define to guard inclusion
> of this header file and define it in kernel/modules.h and in internal
> script that generates *.mod.c files.
>
> In-tree module build:
> ➜  kernel git:(vermagic) ✗ make clean
> ➜  kernel git:(vermagic) ✗ make M=drivers/infiniband/hw/mlx5
> ➜  kernel git:(vermagic) ✗ modinfo drivers/infiniband/hw/mlx5/mlx5_ib.ko
> filename:       /images/leonro/src/kernel/drivers/infiniband/hw/mlx5/mlx5_ib.ko
> <...>
> vermagic:       5.6.0+ SMP mod_unload modversions
>
> Out-of-tree module build:
> ➜  mlx5 make -C /images/leonro/src/kernel clean M=/tmp/mlx5
> ➜  mlx5 make -C /images/leonro/src/kernel M=/tmp/mlx5
> ➜  mlx5 modinfo /tmp/mlx5/mlx5_ib.ko
> filename:       /tmp/mlx5/mlx5_ib.ko
> <...>
> vermagic:       5.6.0+ SMP mod_unload modversions
>
> [1] https://lore.kernel.org/lkml/20200411155623.GA22175@zn.tnic
> Reported-by: Borislav Petkov <bp@...e.de>
> Acked-by: Borislav Petkov <bp@...e.de>
> Acked-by: Jessica Yu <jeyu@...nel.org>
> Co-developed-by: Masahiro Yamada <masahiroy@...nel.org>
> Signed-off-by: Masahiro Yamada <masahiroy@...nel.org>
> Signed-off-by: Leon Romanovsky <leonro@...lanox.com>
> ---


I did not read the full thread of [1],
and perhaps may miss something.

But, this series is trying to solve a different problem
"driver code should not include <linux/vermagic.h>"
isn't it?


IIUC, Borislav reported conflict of MODULE_ARCH_VERMAGIC
if <linux/vermagic.h> is included before <linux/module.h>.

With your cleanups, the include site of <linux/vermagic.h>
will be limited to kernel/module.c and scripts/mod/module.c

Assuming those two files include them in the *correct* order,
this problem will be suppressed.

But, I do not think it addresses the problem properly.


If
  #include <foo.h>
  #include <bar.h>

works, but

  #include <bar.h>
  #include <foo.h>

does not, the root cause is very likely
that <bar.h> is not self-contained.
The problem is solved by including <foo.h> from <bar.h>


Please see my thoughts in this:
https://lore.kernel.org/patchwork/patch/1227024/


Of course, we are solving different issues, so I think
we can merge both.


What do you think?




>  include/linux/vermagic.h | 5 +++++
>  kernel/module.c          | 3 +++
>  scripts/mod/modpost.c    | 1 +
>  3 files changed, 9 insertions(+)
>
> diff --git a/include/linux/vermagic.h b/include/linux/vermagic.h
> index 9aced11e9000..7768d20ada39 100644
> --- a/include/linux/vermagic.h
> +++ b/include/linux/vermagic.h
> @@ -1,4 +1,9 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef INCLUDE_VERMAGIC
> +#error "This header can be included from kernel/module.c or *.mod.c only"
> +#endif
> +
>  #include <generated/utsrelease.h>
>
>  /* Simply sanity version stamp for modules. */
> diff --git a/kernel/module.c b/kernel/module.c
> index 646f1e2330d2..8833e848b73c 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -4,6 +4,9 @@
>     Copyright (C) 2001 Rusty Russell, 2002, 2010 Rusty Russell IBM.
>
>  */
> +
> +#define INCLUDE_VERMAGIC
> +
>  #include <linux/export.h>
>  #include <linux/extable.h>
>  #include <linux/moduleloader.h>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 5c3c50c5ec52..7f7d4ee7b652 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2251,6 +2251,7 @@ static void add_header(struct buffer *b, struct module *mod)
>          * Include build-salt.h after module.h in order to
>          * inherit the definitions.
>          */
> +       buf_printf(b, "#define INCLUDE_VERMAGIC\n");
>         buf_printf(b, "#include <linux/build-salt.h>\n");
>         buf_printf(b, "#include <linux/vermagic.h>\n");
>         buf_printf(b, "#include <linux/compiler.h>\n");
> --
> 2.25.2
>


--
Best Regards
Masahiro Yamada