Messages in this thread | | | Date | Tue, 7 Apr 2020 09:43:17 +0200 (CEST) | From | Miroslav Benes <> | Subject | Re: [PATCH] module: Harden STRICT_MODULE_RWX |
| |
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
| |