Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200829111437.96334-1-wqu@suse.com>
In-Reply-To: <20200829111437.96334-1-wqu@suse.com>
From:   Lucas De Marchi <lucas.de.marchi@gmail.com>
Date:   Tue, 1 Sep 2020 11:50:58 -0700
Message-ID: <CAKi4VAJ_nZ9DT1Tm=uufbJPGgZJu+4j+DVVA9otv6oGXuD3jxg@mail.gmail.com>
Subject: Re: [PATCH] module: Add more error message for failed kernel module loading
To:     Qu Wenruo <wqu@suse.com>
Cc:     lkml <linux-kernel@vger.kernel.org>,
        linux-modules <linux-modules@vger.kernel.org>,
        Rusty Russell <rusty@rustcorp.com.au>,
        Prarit Bhargava <prarit@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Aug 29, 2020 at 4:15 AM Qu Wenruo <wqu@suse.com> wrote:
>
> When kernel module loading failed, user space only get one of the
> following error messages:
> - -ENOEXEC
>   This is the most confusing one. From corrupted ELF header to bad
>   WRITE|EXEC flags check introduced by in module_enforce_rwx_sections()
>   all returns this error number.
>
> - -EPERM
>   This is for blacklisted modules. But mod doesn't do extra explain
>   on this error either.
>
> - -ENOMEM
>   The only error which needs no explain.
>
> This means, if a user got "Exec format error" from modprobe, it provides
> no meaningful way for the user to debug, and will take extra time
> communicating to get extra info.
>
> So this patch will add extra error messages for -ENOEXEC and -EPERM
> errors, allowing user to do better debugging and reporting.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  kernel/module.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 1c5cff34d9f2..9f748c6eeb48 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2096,8 +2096,12 @@ static int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
>         int i;
>
>         for (i = 0; i < hdr->e_shnum; i++) {
> -               if ((sechdrs[i].sh_flags & shf_wx) == shf_wx)
> +               if ((sechdrs[i].sh_flags & shf_wx) == shf_wx) {
> +                       pr_err(
> +                       "Module %s section %d has invalid WRITE|EXEC flags\n",
> +                               mod->name, i);
>                         return -ENOEXEC;
> +               }
>         }
>
>         return 0;
> @@ -3825,8 +3829,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
>         char *after_dashes;
>
>         err = elf_header_check(info);
> -       if (err)
> +       if (err) {
> +               pr_err("Module has invalid ELF header\n");
>                 goto free_copy;
> +       }
>
>         err = setup_load_info(info, flags);
>         if (err)
> @@ -3834,6 +3840,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>
>         if (blacklisted(info->name)) {
>                 err = -EPERM;
> +               pr_err("Module %s is blacklisted\n", info->name);

I wonder why would anyone actually add the blacklist to the command
line like this and have no
way to revert that back. This was introduced in
be7de5f91fdc modules: Add kernel parameter to blacklist modules
as a way to overcome broken initrd generation afaics. Either kernel
command line (using modprobe.blacklist)
or /etc/modprobe.d options are honoured by libkmod and allow a
sufficiently privileged user to bypass it.

+Rusty, +Prarit: is there anything this module parameter is covering
that I'm missing?

For the changes here,

Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

>                 goto free_copy;
>         }
>
> --
> 2.27.0
>