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;
Date:   Mon, 18 Jan 2021 16:18:27 +0100
From:   Jessica Yu <jeyu@kernel.org>
To:     Frank van der Linden <fllinden@amazon.com>
Cc:     linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] module: harden ELF info handling
Message-ID: <20210118151827.GA22792@linux-8ccs>
References: <20210114222146.25762-1-fllinden@amazon.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20210114222146.25762-1-fllinden@amazon.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

+++ Frank van der Linden [14/01/21 22:21 +0000]:
>5fdc7db644 ("module: setup load info before module_sig_check()")
>moved the ELF setup, so that it was done before the signature
>check. This made the module name available to signature error
>messages.
>
>However, the checks for ELF correctness in setup_load_info
>are not sufficient to prevent bad memory references due to
>corrupted offset fields, indices, etc.
>
>So, there's a regression in behavior here: a corrupt and unsigned
>(or badly signed) module, which might previously have been rejected
>immediately, can now cause an oops/crash.
>
>Harden ELF handling for module loading by doing the following:
>
>- Move the signature check back up so that it comes before ELF
>  initialization. It's best to do the signature check to see
>  if we can trust the module, before using the ELF structures
>  inside it. This also makes checks against info->len
>  more accurate again, as this field will be reduced by the
>  length of the signature in mod_check_sig().
>
>  The module name is now once again not available for error
>  messages during the signature check, but that seems like
>  a fair tradeoff.
>
>- Check if sections have offset / size fields that at least don't
>  exceed the length of the module.
>
>- Check if sections have section name offsets that don't fall
>  outside the section name table.
>
>- Add a few other sanity checks against invalid section indices,
>  etc.
>
>This is not an exhaustive consistency check, but the idea is to
>at least get through the signature and blacklist checks without
>crashing because of corrupted ELF info, and to error out gracefully
>for most issues that would have caused problems later on.
>
>Fixes: 5fdc7db644 ("module: setup load info before module_sig_check()")
>Signed-off-by: Frank van der Linden <fllinden@amazon.com>

Queued on modules-next.

Thanks Frank!

Jessica