Messages in this thread | | | Subject | Re: [PATCH] module: harden ELF info handling | Date | Wed, 6 Jan 2021 19:24:26 +0000 | From | Frank van der Linden <> |
| |
On Tue, Jan 05, 2021 at 02:39:28PM +0100, Jessica Yu wrote: > Hi Frank, > > Sorry for the delay. I've just gotten back from vacation :-)
No problem - I figured you were :-)
Comments inline - > > +++ 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.
Cool, thanks, I'm glad you agree/ > > > - 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> > > --- > > 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.
Sure, will do.
> > > 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?
Yep, good catch, will fix that.
> > > + 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.
Yup, will change that.
Thanks - I'll send v2 today or tomorrow.
- Frank
| |