module: Harden STRICT_MODULE_RWX

PATCH Superseded Peter Zijlstra 4 participants 12 messages Apr 3, 2020 – Apr 10, 2020

lore

Share & tools

lore

Peter Zijlstra

author

2020-04-03 16:37 UTC #0 lore raw

PATCH 1/1 module: Harden STRICT_MODULE_RWX

We're very close to enforcing W^X memory, refuse to load modules that violate this principle per construction.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Cc Jessica Yu <[email protected]>

Cc Kees Cook <[email protected]>

---
 kernel/module.c |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

kernel/module.c +24 -0

@@ -2044,9 +2044,28 @@ static void module_enable_x(const struct
 	frob_text(&mod->core_layout, set_memory_x);
 	frob_text(&mod->init_layout, set_memory_x);
 }
+
+static int module_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
+			       char *secstrings, struct module *mod)
+{
+	int i;
+
+	for (i = 0; i < hdr->e_shnum; i++) {
+		if (sechdrs[i].sh_flags & (SHF_EXECINSTR|SHF_WRITE))
+			return -ENOEXEC;
+	}
+
+	return 0;
+}
+
 #else /* !CONFIG_ARCH_HAS_STRICT_MODULE_RWX */
 static void module_enable_nx(const struct module *mod) { }
 static void module_enable_x(const struct module *mod) { }
+static int module_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
+			       char *secstrings, struct module *mod)
+{
+	return 0;
+}
 #endif /* CONFIG_ARCH_HAS_STRICT_MODULE_RWX */

@@ -3378,6 +3397,11 @@ static struct module *layout_and_allocat
 	if (err < 0)
 		return ERR_PTR(err);
 
+	err = module_rwx_sections(info->hdr, info->sechdrs,
+				  info->secstrings, info->mod);
+	if (err < 0)
+		return ERR_PTR(err);
+
 	/* We will do a special allocation for per-cpu sections later. */
 	info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;

Josh Poimboeuf

2020-04-03 16:56 UTC #1 lore raw

Quoted 14 lines · Peter Zijlstra

We're very close to enforcing W^X memory, refuse to load modules that violate this principle per construction.
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Cc Jessica Yu <[email protected]>
Cc Kees Cook <[email protected]>
---
 kernel/module.c |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

kernel/module.c static void module_enable_x(const struct · line 2049

Quoted 6 lines

kernel/module.c +3 -0

@@ -2044,9 +2044,28 @@ static void module_enable_x(const struct
 	frob_text(&mod->core_layout, set_memory_x);
 	frob_text(&mod->init_layout, set_memory_x);
 }
+
+static int module_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
+			       char *secstrings, struct module *mod)

A verb would be nice: "module_enforce_rwx_sections"?

Shouldn't this be under STRICT_MODULE_RWX instead of ARCH_HAS_STRICT_MODULE_RWX?

kernel/module.c static void module_enable_x(const struct · line 2055

Quoted 6 lines

kernel/module.c +6 -0

+{
+	int i;
+
+	for (i = 0; i < hdr->e_shnum; i++) {
+		if (sechdrs[i].sh_flags & (SHF_EXECINSTR|SHF_WRITE))
+			return -ENOEXEC;

I think you only want the error when both are set?

		if (sechdrs[i].sh_flags & (SHF_EXECINSTR|SHF_WRITE) == (SHF_EXECINSTR|SHF_WRITE))

Signature · 1 line

Josh

Miroslav Benes

2020-04-06 09:55 UTC #3 lore raw

In reply to Josh Poimboeuf

Quoted 11 lines · Josh Poimboeuf

kernel/module.c static void module_enable_x(const struct · line 2055
Quoted 6 lines · Peter Zijlstra
kernel/module.c +6 -0
+{
+	int i;
+
+	for (i = 0; i < hdr->e_shnum; i++) {
+		if (sechdrs[i].sh_flags & (SHF_EXECINSTR|SHF_WRITE))
+			return -ENOEXEC;
I think you only want the error when both are set?
		if (sechdrs[i].sh_flags & (SHF_EXECINSTR|SHF_WRITE) == (SHF_EXECINSTR|SHF_WRITE))

A section with SHF_EXECINSTR and SHF_WRITE but without SHF_ALLOC would be strange though, no? It wouldn't be copied to the final module later anyway.

Looking at layout_sections()... a section with SHF_EXECINSTR|SHF_WRITE|SHF_ALLOC would not be counted at all. However, move_module() later copies everything with SHF_ALLOC flag to the final module. If there is WXA section, there would be a bug because the allocation there would not get the correct size. In that case it is important to error out early as you're proposing.

Am I missing something?

Miroslav

Jessica Yu

2020-04-06 10:46 UTC #4 lore raw

+++ Miroslav Benes [06/04/20 11:55 +0200]:

Quoted 12 lines · Josh Poimboeuf

Quoted 11 lines · Peter Zijlstra
kernel/module.c static void module_enable_x(const struct · line 2055
Quoted 6 lines
kernel/module.c +6 -0
+{
+	int i;
+
+	for (i = 0; i < hdr->e_shnum; i++) {
+		if (sechdrs[i].sh_flags & (SHF_EXECINSTR|SHF_WRITE))
+			return -ENOEXEC;
I think you only want the error when both are set?

Quoted 5 lines

Quoted 1 line
		if (sechdrs[i].sh_flags & (SHF_EXECINSTR|SHF_WRITE) == (SHF_EXECINSTR|SHF_WRITE))
A section with SHF_EXECINSTR and SHF_WRITE but without SHF_ALLOC would be strange though, no? It wouldn't be copied to the final module later anyway.

That's right - move_module() ignores !SHF_ALLOC sections and does not copy them over to their final location. So I think we want to look for SHF_EXECINSTR|SHF_WRITE|SHF_ALLOC here..

Quoted 2 lines

Looking at layout_sections()... a section with SHF_EXECINSTR|SHF_WRITE|SHF_ALLOC would not be counted at all.

Also correct, a section with SHF_EXECINSTR|SHF_WRITE|SHF_ALLOC would be ignored as it matches none of the masks listed in layout_sections() - its section->sh_entsize will stay ~0UL.

Quoted 5 lines

However,
move_module() later copies everything with SHF_ALLOC flag to the final module. If there is WXA section, there would be a bug because the allocation there would not get the correct size. In that case it is important to error out early as you're proposing.

That would be a bug indeed, - we'd get a completely wrong offset to copy into since sh_entsize was never initialized. Actually, there should probably be a check for that in move_module() :-/

Quoted 1 line

Am I missing something?

Nope, thanks for double checking everything!

Jessica

Peter Zijlstra

author

2020-04-06 11:27 UTC #5 lore raw

Quoted 15 lines · Jessica Yu

+++ Miroslav Benes [06/04/20 11:55 +0200]:
Quoted 14 lines · Josh Poimboeuf
Quoted 12 lines · Peter Zijlstra
kernel/module.c static void module_enable_x(const struct · line 2055
Quoted 6 lines
kernel/module.c +6 -0
+{
+	int i;
+
+	for (i = 0; i < hdr->e_shnum; i++) {
+		if (sechdrs[i].sh_flags & (SHF_EXECINSTR|SHF_WRITE))
+			return -ENOEXEC;
I think you only want the error when both are set?
		if (sechdrs[i].sh_flags & (SHF_EXECINSTR|SHF_WRITE) == (SHF_EXECINSTR|SHF_WRITE))

Quoted 7 lines

Quoted 3 lines
A section with SHF_EXECINSTR and SHF_WRITE but without SHF_ALLOC would be strange though, no? It wouldn't be copied to the final module later anyway.
That's right - move_module() ignores !SHF_ALLOC sections and does not copy them over to their final location. So I think we want to look for SHF_EXECINSTR|SHF_WRITE|SHF_ALLOC here..

So I did notice that !SHF_ALLOC sections get ignored, but since this check is about W^X we don't strictly care about SHF_ALLOC. What we care about it never allowing a writable and executable map.

Adding ALLOC to the test only allows for future mistakes and doesn't make the check any better.

Jessica Yu

2020-04-06 12:53 UTC #6 lore raw

+++ Peter Zijlstra [06/04/20 13:27 +0200]:

Quoted 24 lines · Jessica Yu

Quoted 15 lines
+++ Miroslav Benes [06/04/20 11:55 +0200]:
Quoted 14 lines · Josh Poimboeuf
Quoted 12 lines · Peter Zijlstra
kernel/module.c static void module_enable_x(const struct · line 2055
Quoted 6 lines
kernel/module.c +6 -0
+{
+	int i;
+
+	for (i = 0; i < hdr->e_shnum; i++) {
+		if (sechdrs[i].sh_flags & (SHF_EXECINSTR|SHF_WRITE))
+			return -ENOEXEC;
I think you only want the error when both are set?
		if (sechdrs[i].sh_flags & (SHF_EXECINSTR|SHF_WRITE) == (SHF_EXECINSTR|SHF_WRITE))
Quoted 7 lines
Quoted 3 lines
A section with SHF_EXECINSTR and SHF_WRITE but without SHF_ALLOC would be strange though, no? It wouldn't be copied to the final module later anyway.
That's right - move_module() ignores !SHF_ALLOC sections and does not copy them over to their final location. So I think we want to look for SHF_EXECINSTR|SHF_WRITE|SHF_ALLOC here..

Quoted 6 lines

So I did notice that !SHF_ALLOC sections get ignored, but since this check is about W^X we don't strictly care about SHF_ALLOC. What we care about it never allowing a writable and executable map.
Adding ALLOC to the test only allows for future mistakes and doesn't make the check any better.

Ugh sorry, my brain shorted out and for some reason I mistakenly thought the check excluded SHF_WRITE|SHF_EXECINSTR|SHF_ALLOC sections. It doesn't obviously. Sorry for the noise.

Peter Zijlstra

author

2020-04-06 14:11 UTC #7 lore raw

Quoted 3 lines · Jessica Yu

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
kernel/module.c static void module_enable_x(const struct · line 2055
Quoted 3 lines
kernel/module.c +3 -0
+	for (i = 0; i < hdr->e_shnum; i++) {
+		if (sechdrs[i].sh_flags & (SHF_EXECINSTR|SHF_WRITE))
+			return -ENOEXEC;

Hehe, I'm well familiar with the brain going funny, as evidenced by the above... :facepalm:

Quoted 3 lines

Ugh sorry, my brain shorted out and for some reason I mistakenly thought the check excluded SHF_WRITE|SHF_EXECINSTR|SHF_ALLOC sections. It doesn't obviously. Sorry for the noise.

Miroslav Benes

2020-04-07 07:43 UTC #8 lore raw

In reply to Peter Zijlstra

Quoted 24 lines · Peter Zijlstra

Quoted 15 lines · Jessica Yu
+++ Miroslav Benes [06/04/20 11:55 +0200]:
Quoted 14 lines · Josh Poimboeuf
Quoted 12 lines · Peter Zijlstra
kernel/module.c static void module_enable_x(const struct · line 2055
Quoted 6 lines
kernel/module.c +6 -0
+{
+	int i;
+
+	for (i = 0; i < hdr->e_shnum; i++) {
+		if (sechdrs[i].sh_flags & (SHF_EXECINSTR|SHF_WRITE))
+			return -ENOEXEC;
I think you only want the error when both are set?
		if (sechdrs[i].sh_flags & (SHF_EXECINSTR|SHF_WRITE) == (SHF_EXECINSTR|SHF_WRITE))
Quoted 7 lines
Quoted 3 lines
A section with SHF_EXECINSTR and SHF_WRITE but without SHF_ALLOC would be strange though, no? It wouldn't be copied to the final module later anyway.
That's right - move_module() ignores !SHF_ALLOC sections and does not copy them over to their final location. So I think we want to look for SHF_EXECINSTR|SHF_WRITE|SHF_ALLOC here..

Quoted 6 lines

So I did notice that !SHF_ALLOC sections get ignored, but since this check is about W^X we don't strictly care about SHF_ALLOC. What we care about it never allowing a writable and executable map.
Adding ALLOC to the test only allows for future mistakes and doesn't make the check any better.

Ok, fair enough.

I am still wondering if there are modules out there with sections flags combination which would cause the same problem with layout_sections() and move_module() logic I described earlier. But that it is a separate issue.

Thanks
Miroslav

Miroslav Benes

2020-04-09 16:55 UTC #9 lore raw

Quoted 3 lines

I am still wondering if there are modules out there with sections flags combination which would cause the same problem with layout_sections() and move_module() logic I described earlier. But that it is a separate issue.

And of course I misread the condition in layout_sections() and all should be fine. Oh well...

Miroslav

Jessica Yu

2020-04-10 09:04 UTC #10 lore raw

+++ Miroslav Benes [09/04/20 18:55 +0200]:

Quoted 6 lines

Quoted 3 lines
I am still wondering if there are modules out there with sections flags combination which would cause the same problem with layout_sections() and move_module() logic I described earlier. But that it is a separate issue.
And of course I misread the condition in layout_sections() and all should be fine. Oh well...

Me too :-( For some reason I misread it as an exact mask match, ugh. In any case, it looks like we are fine since we'd catch all SHF_ALLOC sections at the minimum and they would have sh_entsize set, and we appropriately ignore non-SHF_ALLOC sections in move_module(), so the hypothetical problem I described earlier was incorrect.

Josh Poimboeuf

2020-04-03 16:59 UTC #11 lore raw

In reply to Peter Zijlstra

Quoted 2 lines · Peter Zijlstra

We're very close to enforcing W^X memory

Oh, and I haven't forgotten ;-) Will bump it up the TODO list and finish it soon.

Signature · 1 line

Josh