* [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation [not found] <1492088223-98232-1-git-send-email-zhangshaokun@hisilicon.com> @ 2017-04-13 12:33 ` dongbo (E) 2017-04-18 17:01 ` Catalin Marinas 2017-04-25 6:58 ` [PATCH REPOST] " dongbo (E) 0 siblings, 2 replies; 13+ messages in thread From: dongbo (E) @ 2017-04-13 12:33 UTC (permalink / raw) To: viro, linux-fsdevel, linux-kernel Cc: Mark Rutland, Linuxarm, linux-arm-kernel, catalin.marinas, will.deacon From: Dong Bo <dongbo4@huawei.com> In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set, the flag is propagated to its child processes, even the elf files are marked as not requiring executable stack. It may cause superfluous operations on some arch, e.g. __sync_icache_dcache on aarch64 due to a PROT_READ mmap is also marked as PROT_EXEC. Signed-off-by: Dong Bo <dongbo4@huawei.com> --- fs/binfmt_elf.c | 2 ++ fs/binfmt_elf_fdpic.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 5075fd5..c52e670 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -863,6 +863,8 @@ static int load_elf_binary(struct linux_binprm *bprm) SET_PERSONALITY2(loc->elf_ex, &arch_state); if (elf_read_implies_exec(loc->elf_ex, executable_stack)) current->personality |= READ_IMPLIES_EXEC; + else + current->personality &= ~READ_IMPLIES_EXEC; if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space) current->flags |= PF_RANDOMIZE; diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c index cf93a4f..c4bc4d0 100644 --- a/fs/binfmt_elf_fdpic.c +++ b/fs/binfmt_elf_fdpic.c @@ -354,6 +354,8 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) set_personality(PER_LINUX); if (elf_read_implies_exec(&exec_params.hdr, executable_stack)) current->personality |= READ_IMPLIES_EXEC; + else + current->personality &= ~READ_IMPLIES_EXEC; setup_new_exec(bprm); -- 1.9.1 . ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation 2017-04-13 12:33 ` [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation dongbo (E) @ 2017-04-18 17:01 ` Catalin Marinas 2017-04-18 20:01 ` Peter Maydell 2017-04-25 6:58 ` [PATCH REPOST] " dongbo (E) 1 sibling, 1 reply; 13+ messages in thread From: Catalin Marinas @ 2017-04-18 17:01 UTC (permalink / raw) To: dongbo (E) Cc: viro, linux-fsdevel, linux-kernel, Mark Rutland, will.deacon, Linuxarm, linux-arm-kernel, Peter Maydell On Thu, Apr 13, 2017 at 08:33:52PM +0800, dongbo (E) wrote: > From: Dong Bo <dongbo4@huawei.com> > > In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set, > the flag is propagated to its child processes, even the elf > files are marked as not requiring executable stack. It may > cause superfluous operations on some arch, e.g. > __sync_icache_dcache on aarch64 due to a PROT_READ mmap is > also marked as PROT_EXEC. > > Signed-off-by: Dong Bo <dongbo4@huawei.com> > --- > fs/binfmt_elf.c | 2 ++ > fs/binfmt_elf_fdpic.c | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index 5075fd5..c52e670 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -863,6 +863,8 @@ static int load_elf_binary(struct linux_binprm *bprm) > SET_PERSONALITY2(loc->elf_ex, &arch_state); > if (elf_read_implies_exec(loc->elf_ex, executable_stack)) > current->personality |= READ_IMPLIES_EXEC; > + else > + current->personality &= ~READ_IMPLIES_EXEC; > if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space) > current->flags |= PF_RANDOMIZE; > diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c > index cf93a4f..c4bc4d0 100644 > --- a/fs/binfmt_elf_fdpic.c > +++ b/fs/binfmt_elf_fdpic.c > @@ -354,6 +354,8 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) > set_personality(PER_LINUX); > if (elf_read_implies_exec(&exec_params.hdr, executable_stack)) > current->personality |= READ_IMPLIES_EXEC; > + else > + current->personality &= ~READ_IMPLIES_EXEC; > setup_new_exec(bprm); That's affecting most architectures with a risk of ABI breakage. We could do it on arm64 only, though I'm not yet clear on the ABI implications (at a first look, there shouldn't be any). This follows the x86_64 approach but unfortunately we haven't done it on arm64 from the start: diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h index 5d1700425efe..5941e7f6ae60 100644 --- a/arch/arm64/include/asm/elf.h +++ b/arch/arm64/include/asm/elf.h @@ -142,6 +142,7 @@ typedef struct user_fpsimd_state elf_fpregset_t; ({ \ clear_bit(TIF_32BIT, ¤t->mm->context.flags); \ clear_thread_flag(TIF_32BIT); \ + current->personality &= ~READ_IMPLIES_EXEC; \ }) /* update AT_VECTOR_SIZE_ARCH if the number of NEW_AUX_ENT entries changes */ -- Catalin ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation 2017-04-18 17:01 ` Catalin Marinas @ 2017-04-18 20:01 ` Peter Maydell 2017-04-19 10:33 ` Catalin Marinas 0 siblings, 1 reply; 13+ messages in thread From: Peter Maydell @ 2017-04-18 20:01 UTC (permalink / raw) To: Catalin Marinas Cc: dongbo (E), Mark Rutland, Peter Maydell, Will Deacon, Linuxarm, linux-kernel, Al Viro, linux-fsdevel, arm-mail-list On 18 April 2017 at 18:01, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Thu, Apr 13, 2017 at 08:33:52PM +0800, dongbo (E) wrote: >> From: Dong Bo <dongbo4@huawei.com> >> >> In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set, >> the flag is propagated to its child processes, even the elf >> files are marked as not requiring executable stack. It may >> cause superfluous operations on some arch, e.g. >> __sync_icache_dcache on aarch64 due to a PROT_READ mmap is >> also marked as PROT_EXEC. > That's affecting most architectures with a risk of ABI breakage. We > could do it on arm64 only, though I'm not yet clear on the ABI > implications (at a first look, there shouldn't be any). Is there a reason why it isn't just straightforwardly a bug (which we could fix) to make READ_IMPLIES_EXEC propagate to child processes? AFAICT this should be per-process: just because init happens not to have been (re)compiled to permit non-executable stacks doesn't mean every process on the system needs to have an executable stack. Behaviour shouldn't be variable across architectures either, I would hope. thanks -- PMM ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation 2017-04-18 20:01 ` Peter Maydell @ 2017-04-19 10:33 ` Catalin Marinas 2017-04-19 10:45 ` Peter Maydell ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Catalin Marinas @ 2017-04-19 10:33 UTC (permalink / raw) To: Peter Maydell Cc: Mark Rutland, dongbo (E), Peter Maydell, Will Deacon, Linuxarm, linux-kernel, Al Viro, linux-fsdevel, arm-mail-list On Tue, Apr 18, 2017 at 09:01:52PM +0100, Peter Maydell wrote: > On 18 April 2017 at 18:01, Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Thu, Apr 13, 2017 at 08:33:52PM +0800, dongbo (E) wrote: > >> From: Dong Bo <dongbo4@huawei.com> > >> > >> In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set, > >> the flag is propagated to its child processes, even the elf > >> files are marked as not requiring executable stack. It may > >> cause superfluous operations on some arch, e.g. > >> __sync_icache_dcache on aarch64 due to a PROT_READ mmap is > >> also marked as PROT_EXEC. > > > That's affecting most architectures with a risk of ABI breakage. We > > could do it on arm64 only, though I'm not yet clear on the ABI > > implications (at a first look, there shouldn't be any). > > Is there a reason why it isn't just straightforwardly a bug > (which we could fix) to make READ_IMPLIES_EXEC propagate to > child processes? While I agree that it looks like a bug, if there are user programs relying on such bug we call it "ABI". On arm64, I don't think there is anything relying on inheriting READ_IMPLIES_EXEC but I wouldn't change the compat task handling without the corresponding change in arch/arm. > AFAICT this should be per-process: just because > init happens not to have been (re)compiled to permit non-executable > stacks doesn't mean every process on the system needs to have > an executable stack. I think this also affects the heap if brk(2) is used (via VM_DATA_DEFAULT_FLAGS though I guess malloc mostly uses mmap these days). > Behaviour shouldn't be variable across architectures either, I would > hope. The behaviour has already been variable for a long time. Even on x86, AFAICT x86_32 differs from x86_64 in this respect. Anyway, the patch should be posted to linux-arch for a cross-arch discussion. -- Catalin ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation 2017-04-19 10:33 ` Catalin Marinas @ 2017-04-19 10:45 ` Peter Maydell 2017-04-20 3:50 ` dongbo (E) 2017-04-24 15:40 ` Will Deacon 2 siblings, 0 replies; 13+ messages in thread From: Peter Maydell @ 2017-04-19 10:45 UTC (permalink / raw) To: Catalin Marinas Cc: Mark Rutland, dongbo (E), Peter Maydell, Will Deacon, Linuxarm, linux-kernel, Al Viro, linux-fsdevel, arm-mail-list On 19 April 2017 at 11:33, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Tue, Apr 18, 2017 at 09:01:52PM +0100, Peter Maydell wrote: >> >> > That's affecting most architectures with a risk of ABI breakage. We >> > could do it on arm64 only, though I'm not yet clear on the ABI >> > implications (at a first look, there shouldn't be any). >> >> Is there a reason why it isn't just straightforwardly a bug >> (which we could fix) to make READ_IMPLIES_EXEC propagate to >> child processes? > > While I agree that it looks like a bug, if there are user programs > relying on such bug we call it "ABI". Can there be any? Such a program would behave differently depending on how the program that spawned it happened to have been compiled, and for instance could break when the OS happened to have its init binary updated even if the kernel didn't change. >> Behaviour shouldn't be variable across architectures either, I would >> hope. > > The behaviour has already been variable for a long time. Even on x86, > AFAICT x86_32 differs from x86_64 in this respect. That also sounds like a bug to me. > Anyway, the patch should be posted to linux-arch for a cross-arch > discussion. Agreed -- there may be something I'm missing, since it looks like this behaviour of inheriting READ_IMPLIES_EXEC has always been there. thanks -- PMM ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation 2017-04-19 10:33 ` Catalin Marinas 2017-04-19 10:45 ` Peter Maydell @ 2017-04-20 3:50 ` dongbo (E) 2017-04-24 15:40 ` Will Deacon 2 siblings, 0 replies; 13+ messages in thread From: dongbo (E) @ 2017-04-20 3:50 UTC (permalink / raw) To: Catalin Marinas, Peter Maydell, linux Cc: Mark Rutland, Peter Maydell, Will Deacon, Linuxarm, linux-kernel, Al Viro, linux-fsdevel, arm-mail-list, linux-arch On 2017/4/19 18:33, Catalin Marinas wrote: > On Tue, Apr 18, 2017 at 09:01:52PM +0100, Peter Maydell wrote: >> On 18 April 2017 at 18:01, Catalin Marinas <catalin.marinas@arm.com> wrote: >>> On Thu, Apr 13, 2017 at 08:33:52PM +0800, dongbo (E) wrote: >>>> From: Dong Bo <dongbo4@huawei.com> >>>> >>>> In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set, >>>> the flag is propagated to its child processes, even the elf >>>> files are marked as not requiring executable stack. It may >>>> cause superfluous operations on some arch, e.g. >>>> __sync_icache_dcache on aarch64 due to a PROT_READ mmap is >>>> also marked as PROT_EXEC. >>>> >>>> Signed-off-by: Dong Bo <dongbo4@huawei.com> >>>> --- >>>> fs/binfmt_elf.c | 2 ++ >>>> fs/binfmt_elf_fdpic.c | 2 ++ >>>> 2 files changed, 4 insertions(+) >>>> >>>> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c >>>> index 5075fd5..c52e670 100644 >>>> --- a/fs/binfmt_elf.c >>>> +++ b/fs/binfmt_elf.c >>>> @@ -863,6 +863,8 @@ static int load_elf_binary(struct linux_binprm *bprm) >>>> SET_PERSONALITY2(loc->elf_ex, &arch_state); >>>> if (elf_read_implies_exec(loc->elf_ex, executable_stack)) >>>> current->personality |= READ_IMPLIES_EXEC; >>>> + else >>>> + current->personality &= ~READ_IMPLIES_EXEC; >>>> if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space) >>>> current->flags |= PF_RANDOMIZE; >>>> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c >>>> index cf93a4f..c4bc4d0 100644 >>>> --- a/fs/binfmt_elf_fdpic.c >>>> +++ b/fs/binfmt_elf_fdpic.c >>>> @@ -354,6 +354,8 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) >>>> set_personality(PER_LINUX); >>>> if (elf_read_implies_exec(&exec_params.hdr, executable_stack)) >>>> current->personality |= READ_IMPLIES_EXEC; >>>> + else >>>> + current->personality &= ~READ_IMPLIES_EXEC; >>>> setup_new_exec(bprm); >> >>> That's affecting most architectures with a risk of ABI breakage. We >>> could do it on arm64 only, though I'm not yet clear on the ABI >>> implications (at a first look, there shouldn't be any). >> >> Is there a reason why it isn't just straightforwardly a bug >> (which we could fix) to make READ_IMPLIES_EXEC propagate to >> child processes? > > While I agree that it looks like a bug, if there are user programs > relying on such bug we call it "ABI". On arm64, I don't think there is > anything relying on inheriting READ_IMPLIES_EXEC but I wouldn't change > the compat task handling without the corresponding change in arch/arm. > With READ_IMPLIES_EXEC propagation, several hundreds times of __sync_icache_dcache operations shows up than not READ_IMPLIES_EXEC propagation, which degenerating the system performance. Changing arm64 only would settle our problem down, thanks for figuring out previously. Seems that arch/arm had discussed the propagation of READ_IMPLIES_EXEC: http://lists.infradead.org/pipermail/linux-arm-kernel/2012-February/086490.html But the READ_IMPLIES_EXEC is still not cleared in elf_set_personality(). >> AFAICT this should be per-process: just because >> init happens not to have been (re)compiled to permit non-executable >> stacks doesn't mean every process on the system needs to have >> an executable stack. > > I think this also affects the heap if brk(2) is used (via > VM_DATA_DEFAULT_FLAGS though I guess malloc mostly uses mmap these > days). > >> Behaviour shouldn't be variable across architectures either, I would >> hope. > > The behaviour has already been variable for a long time. Even on x86, > AFAICT x86_32 differs from x86_64 in this respect. > > Anyway, the patch should be posted to linux-arch for a cross-arch > discussion. > OK, this mail Cc to linux-arch. Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation 2017-04-19 10:33 ` Catalin Marinas 2017-04-19 10:45 ` Peter Maydell 2017-04-20 3:50 ` dongbo (E) @ 2017-04-24 15:40 ` Will Deacon 2017-04-24 15:58 ` Catalin Marinas 2 siblings, 1 reply; 13+ messages in thread From: Will Deacon @ 2017-04-24 15:40 UTC (permalink / raw) To: Catalin Marinas Cc: Peter Maydell, Mark Rutland, dongbo (E), Peter Maydell, Linuxarm, linux-kernel, Al Viro, linux-fsdevel, arm-mail-list On Wed, Apr 19, 2017 at 11:33:14AM +0100, Catalin Marinas wrote: > On Tue, Apr 18, 2017 at 09:01:52PM +0100, Peter Maydell wrote: > > On 18 April 2017 at 18:01, Catalin Marinas <catalin.marinas@arm.com> wrote: > > > On Thu, Apr 13, 2017 at 08:33:52PM +0800, dongbo (E) wrote: > > >> From: Dong Bo <dongbo4@huawei.com> > > >> > > >> In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set, > > >> the flag is propagated to its child processes, even the elf > > >> files are marked as not requiring executable stack. It may > > >> cause superfluous operations on some arch, e.g. > > >> __sync_icache_dcache on aarch64 due to a PROT_READ mmap is > > >> also marked as PROT_EXEC. > > > > > That's affecting most architectures with a risk of ABI breakage. We > > > could do it on arm64 only, though I'm not yet clear on the ABI > > > implications (at a first look, there shouldn't be any). > > > > Is there a reason why it isn't just straightforwardly a bug > > (which we could fix) to make READ_IMPLIES_EXEC propagate to > > child processes? > > While I agree that it looks like a bug, if there are user programs > relying on such bug we call it "ABI". On arm64, I don't think there is > anything relying on inheriting READ_IMPLIES_EXEC but I wouldn't change > the compat task handling without the corresponding change in arch/arm. > > > AFAICT this should be per-process: just because > > init happens not to have been (re)compiled to permit non-executable > > stacks doesn't mean every process on the system needs to have > > an executable stack. > > I think this also affects the heap if brk(2) is used (via > VM_DATA_DEFAULT_FLAGS though I guess malloc mostly uses mmap these > days). I think it also affects mprotect, which is more worrying imo, particularly for things like JIT code that is ported from 32-bit (although a quick look at v8, ionmonkey and art suggests they all pass PROT_EXEC when needed). Will ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation 2017-04-24 15:40 ` Will Deacon @ 2017-04-24 15:58 ` Catalin Marinas 2017-04-24 16:05 ` Will Deacon ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Catalin Marinas @ 2017-04-24 15:58 UTC (permalink / raw) To: Will Deacon Cc: Mark Rutland, Peter Maydell, dongbo (E), Peter Maydell, Linuxarm, linux-kernel, Al Viro, linux-fsdevel, arm-mail-list On Mon, Apr 24, 2017 at 04:40:23PM +0100, Will Deacon wrote: > On Wed, Apr 19, 2017 at 11:33:14AM +0100, Catalin Marinas wrote: > > On Tue, Apr 18, 2017 at 09:01:52PM +0100, Peter Maydell wrote: > > > On 18 April 2017 at 18:01, Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > On Thu, Apr 13, 2017 at 08:33:52PM +0800, dongbo (E) wrote: > > > >> From: Dong Bo <dongbo4@huawei.com> > > > >> > > > >> In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set, > > > >> the flag is propagated to its child processes, even the elf > > > >> files are marked as not requiring executable stack. It may > > > >> cause superfluous operations on some arch, e.g. > > > >> __sync_icache_dcache on aarch64 due to a PROT_READ mmap is > > > >> also marked as PROT_EXEC. > > > > > > > That's affecting most architectures with a risk of ABI breakage. We > > > > could do it on arm64 only, though I'm not yet clear on the ABI > > > > implications (at a first look, there shouldn't be any). > > > > > > Is there a reason why it isn't just straightforwardly a bug > > > (which we could fix) to make READ_IMPLIES_EXEC propagate to > > > child processes? > > > > While I agree that it looks like a bug, if there are user programs > > relying on such bug we call it "ABI". On arm64, I don't think there is > > anything relying on inheriting READ_IMPLIES_EXEC but I wouldn't change > > the compat task handling without the corresponding change in arch/arm. > > > > > AFAICT this should be per-process: just because > > > init happens not to have been (re)compiled to permit non-executable > > > stacks doesn't mean every process on the system needs to have > > > an executable stack. > > > > I think this also affects the heap if brk(2) is used (via > > VM_DATA_DEFAULT_FLAGS though I guess malloc mostly uses mmap these > > days). > > I think it also affects mprotect, which is more worrying imo, particularly > for things like JIT code that is ported from 32-bit (although a quick look > at v8, ionmonkey and art suggests they all pass PROT_EXEC when needed). As Peter said, the default behaviour is READ_IMPLIES_EXEC off, so JIT code must already pass PROT_EXEC if it wants executable permission. The question is whether any user code relies on READ_IMPLIES_EXEC being passed down to child processes. I don't think so but I would be reluctant to make an such cross-arch change (happy to do it for arm64 though). Since linux-arch was cc'ed in the middle of this thread, I doubt people would reply. I suggest that the original patch is re-posted to linux-arch directly. -- Catalin ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation 2017-04-24 15:58 ` Catalin Marinas @ 2017-04-24 16:05 ` Will Deacon 2017-04-24 16:14 ` Catalin Marinas 2017-04-25 7:04 ` dongbo (E) 2 siblings, 0 replies; 13+ messages in thread From: Will Deacon @ 2017-04-24 16:05 UTC (permalink / raw) To: Catalin Marinas Cc: Mark Rutland, Peter Maydell, dongbo (E), Peter Maydell, Linuxarm, linux-kernel, Al Viro, linux-fsdevel, arm-mail-list On Mon, Apr 24, 2017 at 04:58:41PM +0100, Catalin Marinas wrote: > On Mon, Apr 24, 2017 at 04:40:23PM +0100, Will Deacon wrote: > > On Wed, Apr 19, 2017 at 11:33:14AM +0100, Catalin Marinas wrote: > > > On Tue, Apr 18, 2017 at 09:01:52PM +0100, Peter Maydell wrote: > > > > On 18 April 2017 at 18:01, Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > > On Thu, Apr 13, 2017 at 08:33:52PM +0800, dongbo (E) wrote: > > > > >> From: Dong Bo <dongbo4@huawei.com> > > > > >> > > > > >> In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set, > > > > >> the flag is propagated to its child processes, even the elf > > > > >> files are marked as not requiring executable stack. It may > > > > >> cause superfluous operations on some arch, e.g. > > > > >> __sync_icache_dcache on aarch64 due to a PROT_READ mmap is > > > > >> also marked as PROT_EXEC. > > > > > > > > > That's affecting most architectures with a risk of ABI breakage. We > > > > > could do it on arm64 only, though I'm not yet clear on the ABI > > > > > implications (at a first look, there shouldn't be any). > > > > > > > > Is there a reason why it isn't just straightforwardly a bug > > > > (which we could fix) to make READ_IMPLIES_EXEC propagate to > > > > child processes? > > > > > > While I agree that it looks like a bug, if there are user programs > > > relying on such bug we call it "ABI". On arm64, I don't think there is > > > anything relying on inheriting READ_IMPLIES_EXEC but I wouldn't change > > > the compat task handling without the corresponding change in arch/arm. > > > > > > > AFAICT this should be per-process: just because > > > > init happens not to have been (re)compiled to permit non-executable > > > > stacks doesn't mean every process on the system needs to have > > > > an executable stack. > > > > > > I think this also affects the heap if brk(2) is used (via > > > VM_DATA_DEFAULT_FLAGS though I guess malloc mostly uses mmap these > > > days). > > > > I think it also affects mprotect, which is more worrying imo, particularly > > for things like JIT code that is ported from 32-bit (although a quick look > > at v8, ionmonkey and art suggests they all pass PROT_EXEC when needed). > > As Peter said, the default behaviour is READ_IMPLIES_EXEC off, so JIT > code must already pass PROT_EXEC if it wants executable permission. The > question is whether any user code relies on READ_IMPLIES_EXEC being > passed down to child processes. I don't think so but I would be > reluctant to make an such cross-arch change (happy to do it for arm64 > though). I was thinking of the case where init has an executable stack, but that's not the common case and such code would be broken under a different init anyway, so I agree that we should make this change for arm64 (but not compat without arch/arm/). Will ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation 2017-04-24 15:58 ` Catalin Marinas 2017-04-24 16:05 ` Will Deacon @ 2017-04-24 16:14 ` Catalin Marinas 2017-04-25 7:04 ` dongbo (E) 2 siblings, 0 replies; 13+ messages in thread From: Catalin Marinas @ 2017-04-24 16:14 UTC (permalink / raw) To: Will Deacon Cc: Mark Rutland, Peter Maydell, dongbo (E), Peter Maydell, Linuxarm, linux-kernel, Al Viro, linux-fsdevel, arm-mail-list On Mon, Apr 24, 2017 at 04:58:41PM +0100, Catalin Marinas wrote: > On Mon, Apr 24, 2017 at 04:40:23PM +0100, Will Deacon wrote: > > On Wed, Apr 19, 2017 at 11:33:14AM +0100, Catalin Marinas wrote: > > > On Tue, Apr 18, 2017 at 09:01:52PM +0100, Peter Maydell wrote: > > > > On 18 April 2017 at 18:01, Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > > On Thu, Apr 13, 2017 at 08:33:52PM +0800, dongbo (E) wrote: > > > > >> From: Dong Bo <dongbo4@huawei.com> > > > > >> > > > > >> In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set, > > > > >> the flag is propagated to its child processes, even the elf > > > > >> files are marked as not requiring executable stack. It may > > > > >> cause superfluous operations on some arch, e.g. > > > > >> __sync_icache_dcache on aarch64 due to a PROT_READ mmap is > > > > >> also marked as PROT_EXEC. > > > > > > > > > That's affecting most architectures with a risk of ABI breakage. We > > > > > could do it on arm64 only, though I'm not yet clear on the ABI > > > > > implications (at a first look, there shouldn't be any). > > > > > > > > Is there a reason why it isn't just straightforwardly a bug > > > > (which we could fix) to make READ_IMPLIES_EXEC propagate to > > > > child processes? > > > > > > While I agree that it looks like a bug, if there are user programs > > > relying on such bug we call it "ABI". On arm64, I don't think there is > > > anything relying on inheriting READ_IMPLIES_EXEC but I wouldn't change > > > the compat task handling without the corresponding change in arch/arm. > > > > > > > AFAICT this should be per-process: just because > > > > init happens not to have been (re)compiled to permit non-executable > > > > stacks doesn't mean every process on the system needs to have > > > > an executable stack. > > > > > > I think this also affects the heap if brk(2) is used (via > > > VM_DATA_DEFAULT_FLAGS though I guess malloc mostly uses mmap these > > > days). > > > > I think it also affects mprotect, which is more worrying imo, particularly > > for things like JIT code that is ported from 32-bit (although a quick look > > at v8, ionmonkey and art suggests they all pass PROT_EXEC when needed). > > As Peter said, the default behaviour is READ_IMPLIES_EXEC off, For the record, just to clarify the "default" behaviour: what I meant is that the (newish) toolchain always generates the GNU_STACK header which disables the executable stack (and therefore READ_IMPLIES_EXEC is off). -- Catalin ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation 2017-04-24 15:58 ` Catalin Marinas 2017-04-24 16:05 ` Will Deacon 2017-04-24 16:14 ` Catalin Marinas @ 2017-04-25 7:04 ` dongbo (E) 2 siblings, 0 replies; 13+ messages in thread From: dongbo (E) @ 2017-04-25 7:04 UTC (permalink / raw) To: Catalin Marinas, Will Deacon Cc: Mark Rutland, Peter Maydell, Peter Maydell, Linuxarm, linux-kernel, Al Viro, linux-fsdevel, arm-mail-list On 2017/4/24 23:58, Catalin Marinas wrote: > On Mon, Apr 24, 2017 at 04:40:23PM +0100, Will Deacon wrote: >> On Wed, Apr 19, 2017 at 11:33:14AM +0100, Catalin Marinas wrote: >>> On Tue, Apr 18, 2017 at 09:01:52PM +0100, Peter Maydell wrote: >>>> On 18 April 2017 at 18:01, Catalin Marinas <catalin.marinas@arm.com> wrote: >>>>> On Thu, Apr 13, 2017 at 08:33:52PM +0800, dongbo (E) wrote: >>>>>> From: Dong Bo <dongbo4@huawei.com> >>>>>> >>>>>> In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set, >>>>>> the flag is propagated to its child processes, even the elf >>>>>> files are marked as not requiring executable stack. It may >>>>>> cause superfluous operations on some arch, e.g. >>>>>> __sync_icache_dcache on aarch64 due to a PROT_READ mmap is >>>>>> also marked as PROT_EXEC. >>>> >>>>> That's affecting most architectures with a risk of ABI breakage. We >>>>> could do it on arm64 only, though I'm not yet clear on the ABI >>>>> implications (at a first look, there shouldn't be any). >>>> >>>> Is there a reason why it isn't just straightforwardly a bug >>>> (which we could fix) to make READ_IMPLIES_EXEC propagate to >>>> child processes? >>> >>> While I agree that it looks like a bug, if there are user programs >>> relying on such bug we call it "ABI". On arm64, I don't think there is >>> anything relying on inheriting READ_IMPLIES_EXEC but I wouldn't change >>> the compat task handling without the corresponding change in arch/arm. >>> >>>> AFAICT this should be per-process: just because >>>> init happens not to have been (re)compiled to permit non-executable >>>> stacks doesn't mean every process on the system needs to have >>>> an executable stack. >>> >>> I think this also affects the heap if brk(2) is used (via >>> VM_DATA_DEFAULT_FLAGS though I guess malloc mostly uses mmap these >>> days). >> >> I think it also affects mprotect, which is more worrying imo, particularly >> for things like JIT code that is ported from 32-bit (although a quick look >> at v8, ionmonkey and art suggests they all pass PROT_EXEC when needed). > > As Peter said, the default behaviour is READ_IMPLIES_EXEC off, so JIT > code must already pass PROT_EXEC if it wants executable permission. The > question is whether any user code relies on READ_IMPLIES_EXEC being > passed down to child processes. I don't think so but I would be > reluctant to make an such cross-arch change (happy to do it for arm64 > though). > OK, I have re-built a patch for arm64 as you suggested. Thanks. > Since linux-arch was cc'ed in the middle of this thread, I doubt people > would reply. I suggest that the original patch is re-posted to > linux-arch directly. > Re-posted. Bo Dong . ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH REPOST] fs: Preventing READ_IMPLIES_EXEC Propagation 2017-04-13 12:33 ` [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation dongbo (E) 2017-04-18 17:01 ` Catalin Marinas @ 2017-04-25 6:58 ` dongbo (E) 2017-06-12 13:41 ` Catalin Marinas 1 sibling, 1 reply; 13+ messages in thread From: dongbo (E) @ 2017-04-25 6:58 UTC (permalink / raw) To: linux-arch Cc: linux-fsdevel, Al Viro, linux-kernel, catalin.marinas, will.deacon, arm-mail-list, Mark Rutland From: Dong Bo <dongbo4@huawei.com> In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set, the flag is propagated to its child processes, even the elf files are marked as not requiring executable stack. It may cause superfluous operations on some arch, e.g. __sync_icache_dcache on aarch64 due to a PROT_READ mmap is also marked as PROT_EXEC. This patch was originally posted and discussed here: https://patchwork.kernel.org/patch/9685891/ Signed-off-by: Dong Bo <dongbo4@huawei.com> --- fs/binfmt_elf.c | 2 ++ fs/binfmt_elf_fdpic.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 5075fd5..c52e670 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -863,6 +863,8 @@ static int load_elf_binary(struct linux_binprm *bprm) SET_PERSONALITY2(loc->elf_ex, &arch_state); if (elf_read_implies_exec(loc->elf_ex, executable_stack)) current->personality |= READ_IMPLIES_EXEC; + else + current->personality &= ~READ_IMPLIES_EXEC; if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space) current->flags |= PF_RANDOMIZE; diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c index cf93a4f..c4bc4d0 100644 --- a/fs/binfmt_elf_fdpic.c +++ b/fs/binfmt_elf_fdpic.c @@ -354,6 +354,8 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) set_personality(PER_LINUX); if (elf_read_implies_exec(&exec_params.hdr, executable_stack)) current->personality |= READ_IMPLIES_EXEC; + else + current->personality &= ~READ_IMPLIES_EXEC; setup_new_exec(bprm); -- 1.9.1 . ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH REPOST] fs: Preventing READ_IMPLIES_EXEC Propagation 2017-04-25 6:58 ` [PATCH REPOST] " dongbo (E) @ 2017-06-12 13:41 ` Catalin Marinas 0 siblings, 0 replies; 13+ messages in thread From: Catalin Marinas @ 2017-06-12 13:41 UTC (permalink / raw) To: dongbo (E) Cc: linux-arch, linux-fsdevel, Al Viro, linux-kernel, will.deacon, arm-mail-list, Mark Rutland On Tue, Apr 25, 2017 at 02:58:01PM +0800, dongbo (E) wrote: > From: Dong Bo <dongbo4@huawei.com> > > In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set, > the flag is propagated to its child processes, even the elf > files are marked as not requiring executable stack. It may > cause superfluous operations on some arch, e.g. > __sync_icache_dcache on aarch64 due to a PROT_READ mmap is > also marked as PROT_EXEC. > > This patch was originally posted and discussed here: > https://patchwork.kernel.org/patch/9685891/ > > Signed-off-by: Dong Bo <dongbo4@huawei.com> > --- > fs/binfmt_elf.c | 2 ++ > fs/binfmt_elf_fdpic.c | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index 5075fd5..c52e670 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -863,6 +863,8 @@ static int load_elf_binary(struct linux_binprm *bprm) > SET_PERSONALITY2(loc->elf_ex, &arch_state); > if (elf_read_implies_exec(loc->elf_ex, executable_stack)) > current->personality |= READ_IMPLIES_EXEC; > + else > + current->personality &= ~READ_IMPLIES_EXEC; > if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space) > current->flags |= PF_RANDOMIZE; > diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c > index cf93a4f..c4bc4d0 100644 > --- a/fs/binfmt_elf_fdpic.c > +++ b/fs/binfmt_elf_fdpic.c > @@ -354,6 +354,8 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) > set_personality(PER_LINUX); > if (elf_read_implies_exec(&exec_params.hdr, executable_stack)) > current->personality |= READ_IMPLIES_EXEC; > + else > + current->personality &= ~READ_IMPLIES_EXEC; > setup_new_exec(bprm); > -- 1.9.1 FWIW, we queued the arm64-equivalent patch (commit 48f99c8ec0b25 in linux-next). It doesn't change the behaviour for compat tasks since we want to keep the same behaviour with arch/arm. If the above generic patch gets merged, we'll drop the arm64-specific one. -- Catalin ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-06-12 13:41 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1492088223-98232-1-git-send-email-zhangshaokun@hisilicon.com> 2017-04-13 12:33 ` [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation dongbo (E) 2017-04-18 17:01 ` Catalin Marinas 2017-04-18 20:01 ` Peter Maydell 2017-04-19 10:33 ` Catalin Marinas 2017-04-19 10:45 ` Peter Maydell 2017-04-20 3:50 ` dongbo (E) 2017-04-24 15:40 ` Will Deacon 2017-04-24 15:58 ` Catalin Marinas 2017-04-24 16:05 ` Will Deacon 2017-04-24 16:14 ` Catalin Marinas 2017-04-25 7:04 ` dongbo (E) 2017-04-25 6:58 ` [PATCH REPOST] " dongbo (E) 2017-06-12 13:41 ` Catalin Marinas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).