All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] module: Harden STRICT_MODULE_RWX
@ 2020-04-03 16:37 Peter Zijlstra
  2020-04-03 16:56 ` Josh Poimboeuf
  2020-04-03 16:59 ` Josh Poimboeuf
  0 siblings, 2 replies; 12+ messages in thread
From: Peter Zijlstra @ 2020-04-03 16:37 UTC (permalink / raw)
  To: jeyu
  Cc: linux-kernel, Thomas Gleixner, keescook, Josh Poimboeuf, Miroslav Benes


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) <peterz@infradead.org>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
---
 kernel/module.c |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

--- a/kernel/module.c
+++ b/kernel/module.c
@@ -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;
 

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] module: Harden STRICT_MODULE_RWX
  2020-04-03 16:37 [PATCH] module: Harden STRICT_MODULE_RWX Peter Zijlstra
@ 2020-04-03 16:56 ` Josh Poimboeuf
  2020-04-03 17:08   ` Peter Zijlstra
  2020-04-06  9:55   ` Miroslav Benes
  2020-04-03 16:59 ` Josh Poimboeuf
  1 sibling, 2 replies; 12+ messages in thread
From: Josh Poimboeuf @ 2020-04-03 16:56 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: jeyu, linux-kernel, Thomas Gleixner, keescook, Miroslav Benes

On Fri, Apr 03, 2020 at 06:37:16PM +0200, Peter Zijlstra wrote:
> 
> 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) <peterz@infradead.org>
> Cc: Jessica Yu <jeyu@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> ---
>  kernel/module.c |   24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -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?

> +{
> +	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))

-- 
Josh


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] module: Harden STRICT_MODULE_RWX
  2020-04-03 16:37 [PATCH] module: Harden STRICT_MODULE_RWX Peter Zijlstra
  2020-04-03 16:56 ` Josh Poimboeuf
@ 2020-04-03 16:59 ` Josh Poimboeuf
  1 sibling, 0 replies; 12+ messages in thread
From: Josh Poimboeuf @ 2020-04-03 16:59 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: jeyu, linux-kernel, Thomas Gleixner, keescook, Miroslav Benes

On Fri, Apr 03, 2020 at 06:37:16PM +0200, Peter Zijlstra wrote:
> 
> 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.

-- 
Josh


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] module: Harden STRICT_MODULE_RWX
  2020-04-03 16:56 ` Josh Poimboeuf
@ 2020-04-03 17:08   ` Peter Zijlstra
  2020-04-06  9:55   ` Miroslav Benes
  1 sibling, 0 replies; 12+ messages in thread
From: Peter Zijlstra @ 2020-04-03 17:08 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: jeyu, linux-kernel, Thomas Gleixner, keescook, Miroslav Benes

On Fri, Apr 03, 2020 at 11:56:31AM -0500, Josh Poimboeuf wrote:
> On Fri, Apr 03, 2020 at 06:37:16PM +0200, Peter Zijlstra wrote:
> > 
> > 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) <peterz@infradead.org>
> > Cc: Jessica Yu <jeyu@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > ---
> >  kernel/module.c |   24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -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?
> 
> > +{
> > +	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))
> 

Duh. yes. Let me respin.

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] module: Harden STRICT_MODULE_RWX
  2020-04-03 16:56 ` Josh Poimboeuf
  2020-04-03 17:08   ` Peter Zijlstra
@ 2020-04-06  9:55   ` Miroslav Benes
  2020-04-06 10:46     ` Jessica Yu
  1 sibling, 1 reply; 12+ messages in thread
From: Miroslav Benes @ 2020-04-06  9:55 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Peter Zijlstra, jeyu, linux-kernel, Thomas Gleixner, keescook

On Fri, 3 Apr 2020, Josh Poimboeuf wrote:

> On Fri, Apr 03, 2020 at 06:37:16PM +0200, Peter Zijlstra wrote:
> > +{
> > +	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

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] module: Harden STRICT_MODULE_RWX
  2020-04-06  9:55   ` Miroslav Benes
@ 2020-04-06 10:46     ` Jessica Yu
  2020-04-06 11:27       ` Peter Zijlstra
  0 siblings, 1 reply; 12+ messages in thread
From: Jessica Yu @ 2020-04-06 10:46 UTC (permalink / raw)
  To: Miroslav Benes
  Cc: Josh Poimboeuf, Peter Zijlstra, linux-kernel, Thomas Gleixner, keescook

+++ Miroslav Benes [06/04/20 11:55 +0200]:
>On Fri, 3 Apr 2020, Josh Poimboeuf wrote:
>
>> On Fri, Apr 03, 2020 at 06:37:16PM +0200, Peter Zijlstra wrote:
>> > +{
>> > +	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.

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..

>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.

>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() :-/

>Am I missing something?

Nope, thanks for double checking everything!

Jessica

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] module: Harden STRICT_MODULE_RWX
  2020-04-06 10:46     ` Jessica Yu
@ 2020-04-06 11:27       ` Peter Zijlstra
  2020-04-06 12:53         ` Jessica Yu
  2020-04-07  7:43         ` Miroslav Benes
  0 siblings, 2 replies; 12+ messages in thread
From: Peter Zijlstra @ 2020-04-06 11:27 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Miroslav Benes, Josh Poimboeuf, linux-kernel, Thomas Gleixner, keescook

On Mon, Apr 06, 2020 at 12:46:17PM +0200, Jessica Yu wrote:
> +++ Miroslav Benes [06/04/20 11:55 +0200]:
> > On Fri, 3 Apr 2020, Josh Poimboeuf wrote:
> > 
> > > On Fri, Apr 03, 2020 at 06:37:16PM +0200, Peter Zijlstra wrote:
> > > > +{
> > > > +	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.
> 
> 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.


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] module: Harden STRICT_MODULE_RWX
  2020-04-06 11:27       ` Peter Zijlstra
@ 2020-04-06 12:53         ` Jessica Yu
  2020-04-06 14:11           ` Peter Zijlstra
  2020-04-07  7:43         ` Miroslav Benes
  1 sibling, 1 reply; 12+ messages in thread
From: Jessica Yu @ 2020-04-06 12:53 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Miroslav Benes, Josh Poimboeuf, linux-kernel, Thomas Gleixner, keescook

+++ Peter Zijlstra [06/04/20 13:27 +0200]:
>On Mon, Apr 06, 2020 at 12:46:17PM +0200, Jessica Yu wrote:
>> +++ Miroslav Benes [06/04/20 11:55 +0200]:
>> > On Fri, 3 Apr 2020, Josh Poimboeuf wrote:
>> >
>> > > On Fri, Apr 03, 2020 at 06:37:16PM +0200, Peter Zijlstra wrote:
>> > > > +{
>> > > > +	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.
>>
>> 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.

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] module: Harden STRICT_MODULE_RWX
  2020-04-06 12:53         ` Jessica Yu
@ 2020-04-06 14:11           ` Peter Zijlstra
  0 siblings, 0 replies; 12+ messages in thread
From: Peter Zijlstra @ 2020-04-06 14:11 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Miroslav Benes, Josh Poimboeuf, linux-kernel, Thomas Gleixner, keescook

On Mon, Apr 06, 2020 at 02:53:37PM +0200, Jessica Yu wrote:

> > > > > > +	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:

> 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.

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] module: Harden STRICT_MODULE_RWX
  2020-04-06 11:27       ` Peter Zijlstra
  2020-04-06 12:53         ` Jessica Yu
@ 2020-04-07  7:43         ` Miroslav Benes
  2020-04-09 16:55           ` Miroslav Benes
  1 sibling, 1 reply; 12+ messages in thread
From: Miroslav Benes @ 2020-04-07  7:43 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Jessica Yu, Josh Poimboeuf, linux-kernel, Thomas Gleixner, keescook

On Mon, 6 Apr 2020, Peter Zijlstra wrote:

> On Mon, Apr 06, 2020 at 12:46:17PM +0200, Jessica Yu wrote:
> > +++ Miroslav Benes [06/04/20 11:55 +0200]:
> > > On Fri, 3 Apr 2020, Josh Poimboeuf wrote:
> > > 
> > > > On Fri, Apr 03, 2020 at 06:37:16PM +0200, Peter Zijlstra wrote:
> > > > > +{
> > > > > +	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.
> > 
> > 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.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] module: Harden STRICT_MODULE_RWX
  2020-04-07  7:43         ` Miroslav Benes
@ 2020-04-09 16:55           ` Miroslav Benes
  2020-04-10  9:04             ` Jessica Yu
  0 siblings, 1 reply; 12+ messages in thread
From: Miroslav Benes @ 2020-04-09 16:55 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Jessica Yu, Josh Poimboeuf, linux-kernel, Thomas Gleixner, keescook

> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] module: Harden STRICT_MODULE_RWX
  2020-04-09 16:55           ` Miroslav Benes
@ 2020-04-10  9:04             ` Jessica Yu
  0 siblings, 0 replies; 12+ messages in thread
From: Jessica Yu @ 2020-04-10  9:04 UTC (permalink / raw)
  To: Miroslav Benes
  Cc: Peter Zijlstra, Josh Poimboeuf, linux-kernel, Thomas Gleixner, keescook

+++ Miroslav Benes [09/04/20 18:55 +0200]:
>> 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.


^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-04-10  9:04 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-03 16:37 [PATCH] module: Harden STRICT_MODULE_RWX Peter Zijlstra
2020-04-03 16:56 ` Josh Poimboeuf
2020-04-03 17:08   ` Peter Zijlstra
2020-04-06  9:55   ` Miroslav Benes
2020-04-06 10:46     ` Jessica Yu
2020-04-06 11:27       ` Peter Zijlstra
2020-04-06 12:53         ` Jessica Yu
2020-04-06 14:11           ` Peter Zijlstra
2020-04-07  7:43         ` Miroslav Benes
2020-04-09 16:55           ` Miroslav Benes
2020-04-10  9:04             ` Jessica Yu
2020-04-03 16:59 ` Josh Poimboeuf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.