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(+)
--- 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;
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) <[email protected]>
> Cc: Jessica Yu <[email protected]>
> Cc: Kees Cook <[email protected]>
> ---
> 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
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
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) <[email protected]>
> > Cc: Jessica Yu <[email protected]>
> > Cc: Kees Cook <[email protected]>
> > ---
> > 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.
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
+++ 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
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.
+++ 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.
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.
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
> 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
+++ 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.