linux-kernel - Re: [PATCH] module: fix invalid ELF structure error to print error code

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 22 Sep 2021 19:59:09 -0700
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Shuah Khan <skhan@...uxfoundation.org>
Cc:     jeyu@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] module: fix invalid ELF structure error to print error
 code

On Wed, Sep 22, 2021 at 06:14:42PM -0600, Shuah Khan wrote:
> When elf_validity_check() returns error, load_module() prints an
> error message without error code. It is hard to determine why the
> module ELF structure is invalid without this information. Fix the
> error message to print the error code.
> 
> Signed-off-by: Shuah Khan <skhan@...uxfoundation.org>
> ---
>  kernel/module.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 40ec9a030eec..a0d412d396d6 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3941,7 +3941,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	 */
>  	err = elf_validity_check(info);
>  	if (err) {
> -		pr_err("Module has invalid ELF structures\n");
> +		pr_err("Module has invalid ELF structures:errno(%ld)\n", err);
>  		goto free_copy;

The subject seems to indicate a fix is being made, and I think that the
bots that pick up fixes through language might find that this is a
candidate because of that. It is not, and so if you can change the
subject to indicate that this is just expanding the error print message
with the actual error code passed it would be better.

Now, if you look at elf_validity_check() and how it can fail, it would
seem tons of errors are due to -ENOEXEC. Even a function it calls,
validate_section_offset() also uses that return code. So on failure,
you likely won't still know exactly where this failed as you likely
will juse see the -ENOEXEC value, but that won't tell you much I think.

So, it would seem a bit more useful instead to add some pr_debug()s
on the elf_validity_check() and friends so that dynamic debug could
be used to debug and figure out where this did fail, when needed?
Thoughts?

  Luis