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 <[email protected]>
---
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);
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)
+ 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);
+ 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);
+ return -ENOEXEC;
+ }
+ }
+ break;
+ }
+ }
+
return 0;
}
@@ -3095,11 +3186,6 @@ static int rewrite_section_headers(struct load_info *info, int flags)
for (i = 1; i < info->hdr->e_shnum; i++) {
Elf_Shdr *shdr = &info->sechdrs[i];
- if (shdr->sh_type != SHT_NOBITS
- && info->len < shdr->sh_offset + shdr->sh_size) {
- pr_err("Module len %lu truncated\n", info->len);
- return -ENOEXEC;
- }
/*
* Mark all sections sh_addr with their address in the
@@ -3133,11 +3219,6 @@ static int setup_load_info(struct load_info *info, int flags)
{
unsigned int i;
- /* Set up the convenience variables */
- info->sechdrs = (void *)info->hdr + info->hdr->e_shoff;
- info->secstrings = (void *)info->hdr
- + info->sechdrs[info->hdr->e_shstrndx].sh_offset;
-
/* Try to find a name early so we can log errors with a module name */
info->index.info = find_sec(info, ".modinfo");
if (info->index.info)
@@ -3894,26 +3975,50 @@ static int load_module(struct load_info *info, const char __user *uargs,
long err = 0;
char *after_dashes;
- err = elf_header_check(info);
+ /*
+ * Do the signature check (if any) first. All that
+ * the signature check needs is info->len, it does
+ * not need any of the section info. That can be
+ * set up later. This will minimize the chances
+ * of a corrupt module causing problems before
+ * we even get to the signature check.
+ *
+ * The check will also adjust info->len by stripping
+ * off the sig length at the end of the module, making
+ * checks against info->len more correct.
+ */
+ err = module_sig_check(info, flags);
+ if (err)
+ goto free_copy;
+
+ /*
+ * Do basic sanity checks against the ELF header and
+ * sections.
+ */
+ err = elf_validity_check(info);
if (err) {
- pr_err("Module has invalid ELF header\n");
+ pr_err("Module has invalid ELF structures\n");
goto free_copy;
}
+ /*
+ * Everything checks out, so set up the section info
+ * in the info structure.
+ */
err = setup_load_info(info, flags);
if (err)
goto free_copy;
+ /*
+ * Now that we know we have the correct module name, check
+ * if it's blacklisted.
+ */
if (blacklisted(info->name)) {
err = -EPERM;
pr_err("Module %s is blacklisted\n", info->name);
goto free_copy;
}
- err = module_sig_check(info, flags);
- if (err)
- goto free_copy;
-
err = rewrite_section_headers(info, flags);
if (err)
goto free_copy;
diff --git a/kernel/module_signature.c b/kernel/module_signature.c
index 4224a1086b7d..00132d12487c 100644
--- a/kernel/module_signature.c
+++ b/kernel/module_signature.c
@@ -25,7 +25,7 @@ int mod_check_sig(const struct module_signature *ms, size_t file_len,
return -EBADMSG;
if (ms->id_type != PKEY_ID_PKCS7) {
- pr_err("%s: Module is not signed with expected PKCS#7 message\n",
+ pr_err("%s: not signed with expected PKCS#7 message\n",
name);
return -ENOPKG;
}
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index 9d9fc678c91d..9a057c5d1d4d 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -30,7 +30,7 @@ int mod_verify_sig(const void *mod, struct load_info *info)
memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
- ret = mod_check_sig(&ms, modlen, info->name);
+ ret = mod_check_sig(&ms, modlen, info->name ?: "module");
if (ret)
return ret;
--
2.23.3
I tested this patch by writing a little ELF corruption program that messes
up ELF sections, especially those that are accessed early, in various ways.
With the patch, I could no longer cause any problems with corrupted modules.
In the patch I sent in, I moved the signature back up, so that it's done
before any ELF section parsing. This seemed like the best thing to do -
first see if a module can be trusted at all, and only then parse its
ELF sections. That comes at the price of not having the module name available
for error messages during the sig check, but I think that's ok.
However, if there's disagreement on that, here is a different version that
only hardens the ELF check, and does not change the order otherwise. I can
send it in seperately if needed.
- Frank
========
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 ELF validity checks are not enough to prevent a
module with corrupted ELF info from causing a bad memory
reference before the signature and blacklist checks are done.
Harden ELF handling for module loading by doing the following:
- Check if sections have offset / size fields that at least don't
exceed the length of the module or don't overflow.
- 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 safely retrieve the name and get through the signature
and blacklist checks without crashing because of corrupted ELF info.
This should also make things error out more 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 <[email protected]>
---
kernel/module.c | 109 +++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 95 insertions(+), 14 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index 4bf30e4b3eaa..5d9b336d1952 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -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)
+ 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_debug("Invalid ELF section in module num %u type %u\n",
+ i, shdr->sh_type);
+ return err;
+ }
+
+ if (shdr->sh_flags & SHF_ALLOC) {
+ if (shdr->sh_name >= strhdr->sh_size) {
+ pr_debug("Invalid ELF section name in module num %u type %u\n",
+ i, shdr->sh_type);
+ return -ENOEXEC;
+ }
+ }
+ break;
+ }
+ }
+
return 0;
}
@@ -3095,11 +3186,6 @@ static int rewrite_section_headers(struct load_info *info, int flags)
for (i = 1; i < info->hdr->e_shnum; i++) {
Elf_Shdr *shdr = &info->sechdrs[i];
- if (shdr->sh_type != SHT_NOBITS
- && info->len < shdr->sh_offset + shdr->sh_size) {
- pr_err("Module len %lu truncated\n", info->len);
- return -ENOEXEC;
- }
/*
* Mark all sections sh_addr with their address in the
@@ -3133,11 +3219,6 @@ static int setup_load_info(struct load_info *info, int flags)
{
unsigned int i;
- /* Set up the convenience variables */
- info->sechdrs = (void *)info->hdr + info->hdr->e_shoff;
- info->secstrings = (void *)info->hdr
- + info->sechdrs[info->hdr->e_shstrndx].sh_offset;
-
/* Try to find a name early so we can log errors with a module name */
info->index.info = find_sec(info, ".modinfo");
if (info->index.info)
@@ -3894,9 +3975,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
long err = 0;
char *after_dashes;
- err = elf_header_check(info);
+ err = elf_validity_check(info);
if (err) {
- pr_err("Module has invalid ELF header\n");
+ pr_err("Module has invalid ELF structures\n");
goto free_copy;
}
--
2.23.3
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 <[email protected]>
>---
> 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
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 <[email protected]>
> > ---
> > 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