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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 1 Sep 2020 16:05:38 +0200
From:   Jessica Yu <jeyu@...nel.org>
To:     Qu Wenruo <wqu@...e.com>
Cc:     linux-kernel@...r.kernel.org, jeyu@...e.de
Subject: Re: [PATCH] module: Add more error message for failed kernel module
 loading

+++ Qu Wenruo [31/08/20 16:37 +0800]:
>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@...e.com>

Thanks for your patch, agreed that there should be more descriptive
error messages to help debug module loading issues.

>---
> kernel/module.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index 8fa2600bde6a..204bf29437b8 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -2068,8 +2068,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);

I think it's OK to put pr_err and the format string on the same line.
IMO the line break doesn't add much readability value in this case.

Also, we have access to secstrings in this function. We can print out
the section name with secstrings + sechdrs[i].sh_name in addition to
the section number, I think that would be helpful.

And can we reformat the message to start with the module name, similar
to other pr_err() sites? i.e.,

pr_err("%s: section %s (index %d) has invalid WRITE|EXEC flags",
		mod->name, secstrings + sechdrs[i].sh_name, i)

The rest looks fine to me. Thanks!

Jessica

Powered by blists - more mailing lists