Re: [PATCH] module: harden ELF info handling

[Jessica Yu]

Hi Frank,

Sorry for the delay. I've just gotten back from vacation :-)

+++ Frank van der Linden [21/12/20 23:49 +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.

I vaguely remember that I had made the module name available in
response to a one-off request, IIRC someone had wanted the module name
logged to be able to figure out which module(s) had failed signature
verification. But I do agree with your line of reasoning, that we
should probably not access internal module structures until we have
verified that we can trust the module. It is a chicken and egg problem
unfortunately. Although, it is probably worth it to trade ease of
debugging for a more hardened approach.

> - 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@xxxxxxxxxx>
> ---
> kernel/module.c           | 143 +++++++++++++++++++++++++++++++++-----
> kernel/module_signature.c |   2 +-
> kernel/module_signing.c   |   2 +-
> 3 files changed, 126 insertions(+), 21 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 4bf30e4b3eaa..ef7681a22a1a 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2964,7 +2964,7 @@ static int module_sig_check(struct load_info *info, int flags)
> 	}
>
> 	if (is_module_sig_enforced()) {
> -		pr_notice("%s: loading of %s is rejected\n", info->name, reason);
> +		pr_notice("loading of %s is rejected\n", reason);

Small nit: Let's start with a capital letter perhaps? Just to be
consistent with the other log messages that don't start with a prefix.
Same goes for the other pr_err()s below.

> 		return -EKEYREJECTED;
> 	}
>
> @@ -2977,9 +2977,33 @@ static int module_sig_check(struct load_info *info, int flags)
> }
> #endif /* !CONFIG_MODULE_SIG */
>
> -/* Sanity checks against invalid binaries, wrong arch, weird elf version. */
> -static int elf_header_check(struct load_info *info)
> +static int validate_section_offset(struct load_info *info, Elf_Shdr *shdr)
> {
> +	unsigned long secend;
> +
> +	/*
> +	 * Check for both overflow and offset/size being
> +	 * too large.
> +	 */
> +	secend = shdr->sh_offset + shdr->sh_size;
> +	if (secend < shdr->sh_offset || secend >= info->len)

Should this not be secend > info->len?

> +		return -ENOEXEC;
> +
> +	return 0;
> +}
> +
> +/*
> + * Sanity checks against invalid binaries, wrong arch, weird elf version.
> + *
> + * Also do basic validity checks against section offsets and sizes, the
> + * section name string table, and the indices used for it (sh_name).
> + */
> +static int elf_validity_check(struct load_info *info)
> +{
> +	unsigned int i;
> +	Elf_Shdr *shdr, *strhdr;
> +	int err;
> +
> 	if (info->len < sizeof(*(info->hdr)))
> 		return -ENOEXEC;
>
> @@ -2989,11 +3013,78 @@ static int elf_header_check(struct load_info *info)
> 	    || info->hdr->e_shentsize != sizeof(Elf_Shdr))
> 		return -ENOEXEC;
>
> +	/*
> +	 * e_shnum is 16 bits, and sizeof(Elf_Shdr) is
> +	 * known and small. So e_shnum * sizeof(Elf_Shdr)
> +	 * will not overflow unsigned long on any platform.
> +	 */
> 	if (info->hdr->e_shoff >= info->len
> 	    || (info->hdr->e_shnum * sizeof(Elf_Shdr) >
> 		info->len - info->hdr->e_shoff))
> 		return -ENOEXEC;
>
> +	info->sechdrs = (void *)info->hdr + info->hdr->e_shoff;
> +
> +	/*
> +	 * Verify if the section name table index is valid.
> +	 */
> +	if (info->hdr->e_shstrndx == SHN_UNDEF
> +	    || info->hdr->e_shstrndx >= info->hdr->e_shnum)
> +		return -ENOEXEC;
> +
> +	strhdr = &info->sechdrs[info->hdr->e_shstrndx];
> +	err = validate_section_offset(info, strhdr);
> +	if (err < 0)
> +		return err;
> +
> +	/*
> +	 * The section name table must be NUL-terminated, as required
> +	 * by the spec. This makes strcmp and pr_* calls that access
> +	 * strings in the section safe.
> +	 */
> +	info->secstrings = (void *)info->hdr + strhdr->sh_offset;
> +	if (info->secstrings[strhdr->sh_size - 1] != '\0')
> +		return -ENOEXEC;
> +
> +	/*
> +	 * The code assumes that section 0 has a length of zero and
> +	 * an addr of zero, so check for it.
> +	 */
> +	if (info->sechdrs[0].sh_type != SHT_NULL
> +	    || info->sechdrs[0].sh_size != 0
> +	    || info->sechdrs[0].sh_addr != 0)
> +		return -ENOEXEC;
> +
> +	for (i = 1; i < info->hdr->e_shnum; i++) {
> +		shdr = &info->sechdrs[i];
> +		switch (shdr->sh_type) {
> +		case SHT_NULL:
> +		case SHT_NOBITS:
> +			continue;
> +		case SHT_SYMTAB:
> +			if (shdr->sh_link == SHN_UNDEF
> +			    || shdr->sh_link >= info->hdr->e_shnum)
> +				return -ENOEXEC;
> +			fallthrough;
> +		default:
> +			err = validate_section_offset(info, shdr);
> +			if (err < 0) {
> +				pr_err("invalid ELF section in module num %u type %u\n",
> +					i, shdr->sh_type);

Same as the first comment here. Also, this is personal preference but I
think the "in module num %u type %u" reads a bit awkwardly. Maybe
something like "Invalid ELF section in module (section ndx %u type %u)"?

> +				return err;
> +			}
> +
> +			if (shdr->sh_flags & SHF_ALLOC) {
> +				if (shdr->sh_name >= strhdr->sh_size) {
> +					pr_err("invalid ELF section name in module num %u type %u\n",
> +					       i, shdr->sh_type);

Same here.

Thanks!

Jessica