Re: [PATCH v3 3/4] selinux: use vma_is_initial_stack() and vma_is_initial_heap()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 2023/12/7 4:47, Paul Moore wrote:
On Wed, Dec 6, 2023 at 9:22 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:

(Restoring part of the Cc list to include more relevant lists &
people... If you are lost, the original email is here:
https://lore.kernel.org/selinux/20230728050043.59880-4-wangkefeng.wang@xxxxxxxxxx/)

On Tue, Aug 1, 2023 at 1:08 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
On Mon, Jul 31, 2023 at 4:02 PM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
On Mon, Jul 31, 2023 at 12:19 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:

On Mon, Jul 31, 2023 at 10:26 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
I believe this patch yields a semantic change in the SELinux execheap
permission check. That said, I think the change is for the better.

Agreed.  I'm also in favor of using a helper which is maintained by
the VM folks over open coded logic in the SELinux code.

Yes, only caveat is in theory it could trigger new execheap denials
under existing policies.
Trying to construct an example based on the
selinux-testsuite/tests/mmap/mprot_heap.c example but coming up empty
so far on something that both works and yields different results
before and after this patch.

My gut feeling is that this will not be an issue, but I could very
well be wrong.  If it becomes a significant issue we can revert the
SELinux portion of the patch.

Of course, if you have any luck demonstrating this with reasonable
code, that would be good input too.

So, it turns out this does affect actual code. Thus far, we know about
gcl [1] and wine [2]. The gcl case is easy to reproduce (just install
gcl on Fedora and run gcl without arguments), so I was able to dig a
bit deeper.

gcl has the following relevant memory mappings as captured by gdb:
Start Addr           End Addr       Size     Offset  Perms  objfile
   0x413000           0xf75000   0xb62000   0x3fa000  rw-p
/usr/lib/gcl-2.6.14/unixport/saved_ansi_gcl
   0xf75000           0xf79000     0x4000        0x0  rwxp   [heap]

It tries to call mprotect(0x883000, 7282688,
PROT_READ|PROT_WRITE|PROT_EXEC), i.e. it tries to make the region
0x883000 - 0xf75000 executable. Before this patch it was allowed,
whereas now it triggers an execheap SELinux denial. But this seems
wrong - the affected region is merely adjacent to the [heap] region,
it does not actually overlap with it. So even if we accept that the
correct semantics is to catch any region that overlaps with the heap
(before only subregions of the heap were subject to the execheap
check), this corner case doesn't seem to be handled correctly by the
new check (and the same bug seems to have been in fs/proc/task_mmu.c
before commit 11250fd12eb8 ("mm: factor out VMA stack and heap
checks")).

Yes, the heap check exists for a long time,

                         [start_brk        brk]
case1:                     [vm_start,vm_end]
case2:             [vm_start,vm_end]
case3:                               [vm_start,vm_end]
case4:         [vm_start,                      vm_end]

case5:   [vm_start,vm_end]
case6:                                          [vm_start,vm_end]

old check:
vma->vm_start >= vma->vm_mm->start_brk && vma->vm_end <= vma->vm_mm->brk

Only include case1, vma range must be within heap

new check:
vma->vm_start <= vma->vm_mm->brk && vma->vm_end >= vma->vm_mm->start_brk

Include case1~case6, but case5(vm_end=start_brk) and case6(vm_start=brk)
are the corner cases, gcl issue matchs the case5.


I didn't analyze the wine case ([2]), but it may be the same situation.

Unless I'm mistaken, those <= & >= in should in fact be just < & >.

I support this change.
 vma->vm_start < vma->vm_mm->brk && vma->vm_end > vma->vm_mm->start_brk

And the expression in vma_is_initial_stack() is also suspicious (but
I'm not going to make any assumption on what is the intended semantics
there...)

[1] https://bugzilla.redhat.com/show_bug.cgi?id=2252391
[2] https://bugzilla.redhat.com/show_bug.cgi?id=2247299

Could you quickly verify after the above change?

Thanks Ondrej.

I'm hoping the mm folks will comment on this as it looks like this is
an issue with the helper functions, but just in case I'm going to prep
a revert for just the SELinux changes.  If we don't hear anything in
the next couple of days I'll send the revert up to Linus with the idea
that we can eventually shift back to the helpers when this is sorted.





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux