Messages in this thread | | | Date | Mon, 6 Apr 2020 13:27:32 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH] module: Harden STRICT_MODULE_RWX |
| |
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.
| |