| Message ID | 20240808203353.202352-2-paul@paul-moore.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: revert our use of vma_is_initial_heap() | expand |
On Thu, Aug 8, 2024 at 4:34 PM Paul Moore <paul@paul-moore.com> wrote: > > Unfortunately it appears that vma_is_initial_heap() is currently broken > for applications that do not currently have any heap allocated, e.g. > brk == start_brk. The breakage is such that it will cause SELinux to > check for the process/execheap permission on memory regions that cross > brk/start_brk even when there is no heap. > > The proper fix would be to correct vma_is_initial_heap(), but as there > are multiple callers I am hesitant to unilaterally modify the helper > out of concern that I would end up breaking some other subsystem. The > mm developers have been made aware of the situation and hopefully they > will have a fix at some point in the future, but we need a fix soon so > we are simply going to revert our use of vma_is_initial_heap() in favor > of our old logic/code which works as expected, even in the face of a > zero size heap. We can return to using vma_is_initial_heap() at some > point in the future when it is fixed. > > Cc: stable@vger.kernel.org > Reported-by: Marc Reisner <reisner.marc@gmail.com> > Closes: https://lore.kernel.org/all/ZrPmoLKJEf1wiFmM@marcreisner.com > Fixes: 68df1baf158f ("selinux: use vma_is_initial_stack() and vma_is_initial_heap()") > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/selinux/hooks.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) As a FYI, this passes the selinux-testsuite and the execheap reproducer.
On Thu, Aug 8, 2024 at 4:34 PM Paul Moore <paul@paul-moore.com> wrote: > > Unfortunately it appears that vma_is_initial_heap() is currently broken > for applications that do not currently have any heap allocated, e.g. > brk == start_brk. The breakage is such that it will cause SELinux to > check for the process/execheap permission on memory regions that cross > brk/start_brk even when there is no heap. > > The proper fix would be to correct vma_is_initial_heap(), but as there > are multiple callers I am hesitant to unilaterally modify the helper > out of concern that I would end up breaking some other subsystem. The > mm developers have been made aware of the situation and hopefully they > will have a fix at some point in the future, but we need a fix soon so > we are simply going to revert our use of vma_is_initial_heap() in favor > of our old logic/code which works as expected, even in the face of a > zero size heap. We can return to using vma_is_initial_heap() at some > point in the future when it is fixed. > > Cc: stable@vger.kernel.org > Reported-by: Marc Reisner <reisner.marc@gmail.com> > Closes: https://lore.kernel.org/all/ZrPmoLKJEf1wiFmM@marcreisner.com > Fixes: 68df1baf158f ("selinux: use vma_is_initial_stack() and vma_is_initial_heap()") > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/selinux/hooks.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) Merged into selinux/stable-6.11
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 55c78c318ccd..bfa61e005aac 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3852,7 +3852,17 @@ static int selinux_file_mprotect(struct vm_area_struct *vma, if (default_noexec && (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) { int rc = 0; - if (vma_is_initial_heap(vma)) { + /* + * We don't use the vma_is_initial_heap() helper as it has + * a history of problems and is currently broken on systems + * where there is no heap, e.g. brk == start_brk. Before + * replacing the conditional below with vma_is_initial_heap(), + * or something similar, please ensure that the logic is the + * same as what we have below or you have tested every possible + * corner case you can think to test. + */ + if (vma->vm_start >= vma->vm_mm->start_brk && + vma->vm_end <= vma->vm_mm->brk) { rc = avc_has_perm(sid, sid, SECCLASS_PROCESS, PROCESS__EXECHEAP, NULL); } else if (!vma->vm_file && (vma_is_initial_stack(vma) ||
Unfortunately it appears that vma_is_initial_heap() is currently broken for applications that do not currently have any heap allocated, e.g. brk == start_brk. The breakage is such that it will cause SELinux to check for the process/execheap permission on memory regions that cross brk/start_brk even when there is no heap. The proper fix would be to correct vma_is_initial_heap(), but as there are multiple callers I am hesitant to unilaterally modify the helper out of concern that I would end up breaking some other subsystem. The mm developers have been made aware of the situation and hopefully they will have a fix at some point in the future, but we need a fix soon so we are simply going to revert our use of vma_is_initial_heap() in favor of our old logic/code which works as expected, even in the face of a zero size heap. We can return to using vma_is_initial_heap() at some point in the future when it is fixed. Cc: stable@vger.kernel.org Reported-by: Marc Reisner <reisner.marc@gmail.com> Closes: https://lore.kernel.org/all/ZrPmoLKJEf1wiFmM@marcreisner.com Fixes: 68df1baf158f ("selinux: use vma_is_initial_stack() and vma_is_initial_heap()") Signed-off-by: Paul Moore <paul@paul-moore.com> --- security/selinux/hooks.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)