mm: convert to vma_is_initial_heap/stack()

I believe this patch yields a semantic change in the SELinux execheap permission check. That said, I think the change is for the better.

Quoted 10 lines · Stephen Smalley

Quoted 8 lines · Paul Moore

Quoted 2 lines · Stephen Smalley

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.

Quoted 6 lines

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.

(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/[email protected]/)

Quoted 19 lines · Paul Moore

Quoted 10 lines · Stephen Smalley

Quoted 8 lines · Paul Moore

Quoted 2 lines · Stephen Smalley

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.

Quoted 6 lines

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.

Quoted 6 lines

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")).

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 < & >. 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

Quoted 43 lines · Ondrej Mosnacek

(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/[email protected]/)

Quoted 19 lines · Paul Moore

Quoted 10 lines · Stephen Smalley

Quoted 8 lines · Paul Moore

Quoted 2 lines · Stephen Smalley

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.

Quoted 6 lines

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.

Quoted 6 lines

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]

Quoted 4 lines

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

Quoted 8 lines

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")).

Quoted 4 lines

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 < & >.

Quoted 6 lines

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

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.

Quoted 44 lines · Paul Moore

Quoted 43 lines · Ondrej Mosnacek

(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/[email protected]/)

Quoted 19 lines · Paul Moore

Quoted 10 lines · Stephen Smalley

Quoted 8 lines · Paul Moore

Quoted 2 lines · Stephen Smalley

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.

Quoted 6 lines

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.

Quoted 6 lines

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]

Quoted 12 lines

Quoted 4 lines

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

Quoted 8 lines

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.

Quoted 4 lines

Quoted 4 lines

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

Quoted 6 lines

Quoted 6 lines

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?

Quoted 95 lines · Kefeng Wang

Quoted 44 lines · Paul Moore

Quoted 43 lines · Ondrej Mosnacek

(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/[email protected]/)

Quoted 19 lines · Paul Moore

Quoted 10 lines · Stephen Smalley

Quoted 8 lines · Paul Moore

Quoted 2 lines · Stephen Smalley

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.

Quoted 6 lines

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.

Quoted 6 lines

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]

Quoted 12 lines

Quoted 4 lines

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

Quoted 8 lines

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.

Quoted 4 lines

Quoted 4 lines

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

Quoted 4 lines

Quoted 4 lines

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...)

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

[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?

Yes, changing the operators as suggested fixes the gcl case.

BTW, the vma_is_*() helpers introduced by your patch also have wrong indentation - the "return" lines are indented by 7 spaces instead of a tab. If you are going to submit a fixing patch you might want to fix that, too.

It looks like this issue is still not fixed. There has been some investigation on going in this Bugzilla for Fedora:

https://bugzilla.redhat.com/show_bug.cgi?id=2254434

The behavior we are seeing is that when a process has no heap and mmap(2) is called with MAP_PRIVATE | MAP_ANONYMOUS, it allocates memory on the heap.

If the address space returned by mmap(2) is later on made executable with mprotect(2), that triggers an execheap avc.

This has been reproduced on kernels >= 6.6.

In reviewing the code, my best guess is that this is caused by the scenario where brk == start_brk not being handled, though I am not expert enough in kernel code to know. If the start address allocated by mmap is before the starting program break, and the end address is after the starting program break, then the avc will trigger. However, I don't know how mmap deals with determining an address and if it takes into account the program break, or if calling brk(2) later on will just pick a new location.

Quoted 5 lines · Marc Reisner

It looks like this issue is still not fixed. There has been some investigation on going in this Bugzilla for Fedora:

https://bugzilla.redhat.com/show_bug.cgi?id=2254434

FWIW, I recently learned there was also a report in the kernel bugzilla about the same issue:

https://bugzilla.kernel.org/show_bug.cgi?id=218258

Quoted 6 lines

The behavior we are seeing is that when a process has no heap and mmap(2) is called with MAP_PRIVATE | MAP_ANONYMOUS, it allocates memory on the heap.

If the address space returned by mmap(2) is later on made executable with mprotect(2), that triggers an execheap avc.

...

Quoted 1 line

This has been reproduced on kernels >= 6.6.

Quoted 3 lines

In reviewing the code, my best guess is that this is caused by the scenario where brk == start_brk not being handled, though I am not

Quoted 6 lines

expert enough in kernel code to know. If the start address allocated by mmap is before the starting program break, and the end address is after the starting program break, then the avc will trigger. However, I don't know how mmap deals with determining an address and if it takes into account the program break, or if calling brk(2) later on will just pick a new location.

I'm not a mm expert, but thankfully we have some on the To/CC line so I'm hopeful they will be able to take a look and provide some insight.

To bring the relevant code into this email, prior to using the vma_is_initial_heap() helper the SELinux execheap logic looked like this:

   /* WORKING */
   if (vma->vm_start >= vma->vm_mm->start_brk &&
       vma->vm_end <= vma->vm_mm->brk)
     /* execheap denial */

... while the current vma_is_initial_heap() helper has logic that looks like this:

   /* BUGGY */
   if (vma->vm_start < vma->vm_mm->brk &&
       vma->vm_end > vma->vm_mm->start_brk)
     /* execheap denial */

Quoted 16 lines · Paul Moore

Quoted 5 lines · Marc Reisner

It looks like this issue is still not fixed. There has been some investigation on going in this Bugzilla for Fedora:

https://bugzilla.redhat.com/show_bug.cgi?id=2254434

FWIW, I recently learned there was also a report in the kernel bugzilla about the same issue:

https://bugzilla.kernel.org/show_bug.cgi?id=218258

Quoted 4 lines

The behavior we are seeing is that when a process has no heap and mmap(2) is called with MAP_PRIVATE | MAP_ANONYMOUS, it allocates memory on the heap.

Quoted 6 lines

Quoted 2 lines

If the address space returned by mmap(2) is later on made executable with mprotect(2), that triggers an execheap avc.

...

Quoted 1 line

This has been reproduced on kernels >= 6.6.

I try the reproducer, but fails to reproduce on my 6.6

Quoted 3 lines

Quoted 3 lines

In reviewing the code, my best guess is that this is caused by the scenario where brk == start_brk not being handled, though I am not

Is there vma_start,end and start_brk,brk infos, adding some debug print.

security/selinux/hooks.c +3 -0

index 81fbfa5b80d4..b66c381c558e 100644
@@ -3848,6 +3848,9 @@ static int selinux_file_mprotect(struct 
 vm_area_struct *vma,
                 if (vma_is_initial_heap(vma)) {
                         rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
                                           PROCESS__EXECHEAP, NULL);
+                       if (rc)
+                               pr_err("DEBUG: vma_start,end=%lx,%lx, 
 start_brk,brk=%lx,%lx\n",
+                                       vma->vm_start, vma->vm_end, 
 vma->vm_mm->start_brk, vma->vm_mm->brk);

                } else if (!vma->vm_file && (vma_is_initial_stack(vma) || vma_is_stack_for_current(vma))) { rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,

Quoted 8 lines

Quoted 7 lines

expert enough in kernel code to know. If the start address allocated by mmap is before the starting program break, and the end address is after the starting program break, then the avc will trigger. However, I don't know how mmap deals with determining an address and if it takes into account the program break, or if calling brk(2) later on will just pick a new location.

Quoted 2 lines

I'm not a mm expert, but thankfully we have some on the To/CC line so I'm hopeful they will be able to take a look and provide some insight.

+Cc mmap maintainers too.

Quoted 10 lines

To bring the relevant code into this email, prior to using the vma_is_initial_heap() helper the SELinux execheap logic looked like this:

   /* WORKING */
   if (vma->vm_start >= vma->vm_mm->start_brk &&
       vma->vm_end <= vma->vm_mm->brk)
     /* execheap denial */

Quoted 7 lines

... while the current vma_is_initial_heap() helper has logic that looks like this:

   /* BUGGY */
   if (vma->vm_start < vma->vm_mm->brk &&
       vma->vm_end > vma->vm_mm->start_brk)
     /* execheap denial */

Quoted 19 lines · Kefeng Wang

Quoted 17 lines · Paul Moore

Quoted 5 lines · Marc Reisner

It looks like this issue is still not fixed. There has been some investigation on going in this Bugzilla for Fedora:

https://bugzilla.redhat.com/show_bug.cgi?id=2254434

FWIW, I recently learned there was also a report in the kernel bugzilla about the same issue:

https://bugzilla.kernel.org/show_bug.cgi?id=218258

Quoted 4 lines

The behavior we are seeing is that when a process has no heap and mmap(2) is called with MAP_PRIVATE | MAP_ANONYMOUS, it allocates memory on the heap.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

If the address space returned by mmap(2) is later on made executable with mprotect(2), that triggers an execheap avc.

...

Quoted 1 line

This has been reproduced on kernels >= 6.6.

I try the reproducer, but fails to reproduce on my 6.6

After enable selinux then trigger the issue.

Quoted 20 lines

Quoted 3 lines

Quoted 3 lines

In reviewing the code, my best guess is that this is caused by the scenario where brk == start_brk not being handled, though I am not

Is there vma_start,end and start_brk,brk infos, adding some debug print.

security/selinux/hooks.c +3 -0

index 81fbfa5b80d4..b66c381c558e 100644
@@ -3848,6 +3848,9 @@ static int selinux_file_mprotect(struct
 vm_area_struct *vma,
                 if (vma_is_initial_heap(vma)) {
                         rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
                                           PROCESS__EXECHEAP, NULL);
+                       if (rc)
+                               pr_err("DEBUG: vma_start,end=%lx,%lx,
 start_brk,brk=%lx,%lx\n",
+                                       vma->vm_start, vma->vm_end,
 vma->vm_mm->start_brk, vma->vm_mm->brk);

Quoted 3 lines

                 } else if (!vma->vm_file && (vma_is_initial_stack(vma) ||
                             vma_is_stack_for_current(vma))) {
                         rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,

[ 3520.913952] DEBUG: pid=769674,comm=test-vma,

vma:start,end=1e9c000,21e9c000, start_brk,brk=2310000,2310000

vma_start < start_brk = brk < vma_end

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

expert enough in kernel code to know. If the start address allocated by mmap is before the starting program break, and the end address is after the starting program break, then the avc will trigger. However, I don't know how mmap deals with determining an address and if it takes into account the program break, or if calling brk(2) later on will just pick a new location.

Quoted 15 lines

Quoted 2 lines

I'm not a mm expert, but thankfully we have some on the To/CC line so I'm hopeful they will be able to take a look and provide some insight.

+Cc mmap maintainers too.

Quoted 10 lines

To bring the relevant code into this email, prior to using the vma_is_initial_heap() helper the SELinux execheap logic looked like this:

   /* WORKING */
   if (vma->vm_start >= vma->vm_mm->start_brk &&
       vma->vm_end <= vma->vm_mm->brk)
     /* execheap denial */

vma range must be within heap.

Quoted 3 lines

Quoted 3 lines

... while the current vma_is_initial_heap() helper has logic that looks like this:

Quoted 4 lines

Quoted 4 lines

   /* BUGGY */
   if (vma->vm_start < vma->vm_mm->brk &&
       vma->vm_end > vma->vm_mm->start_brk)
     /* execheap denial */

vma range intersection with brk range will be rejected, and there's a cross in above case, so execheap denial, I not sure it should be allowed or not, hope maintainers give some comments here.

Quoted 99 lines · Kefeng Wang

Quoted 19 lines · Kefeng Wang

Quoted 17 lines · Paul Moore

Quoted 5 lines · Marc Reisner

It looks like this issue is still not fixed. There has been some investigation on going in this Bugzilla for Fedora:

https://bugzilla.redhat.com/show_bug.cgi?id=2254434

FWIW, I recently learned there was also a report in the kernel bugzilla about the same issue:

https://bugzilla.kernel.org/show_bug.cgi?id=218258

Quoted 4 lines

The behavior we are seeing is that when a process has no heap and mmap(2) is called with MAP_PRIVATE | MAP_ANONYMOUS, it allocates memory on the heap.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

If the address space returned by mmap(2) is later on made executable with mprotect(2), that triggers an execheap avc.

...

Quoted 1 line

This has been reproduced on kernels >= 6.6.

I try the reproducer, but fails to reproduce on my 6.6

After enable selinux then trigger the issue.

Quoted 20 lines

Quoted 3 lines

Quoted 3 lines

In reviewing the code, my best guess is that this is caused by the scenario where brk == start_brk not being handled, though I am not

Is there vma_start,end and start_brk,brk infos, adding some debug print.

security/selinux/hooks.c +3 -0

index 81fbfa5b80d4..b66c381c558e 100644
@@ -3848,6 +3848,9 @@ static int selinux_file_mprotect(struct
 vm_area_struct *vma,
                  if (vma_is_initial_heap(vma)) {
                          rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
                                            PROCESS__EXECHEAP, NULL);
+                       if (rc)
+                               pr_err("DEBUG: vma_start,end=%lx,%lx,
 start_brk,brk=%lx,%lx\n",
+                                       vma->vm_start, vma->vm_end,
 vma->vm_mm->start_brk, vma->vm_mm->brk);

Quoted 3 lines

                  } else if (!vma->vm_file && (vma_is_initial_stack(vma) ||
                              vma_is_stack_for_current(vma))) {
                          rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,

[ 3520.913952] DEBUG: pid=769674,comm=test-vma,

vma:start,end=1e9c000,21e9c000, start_brk,brk=2310000,2310000

vma_start < start_brk = brk < vma_end

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

expert enough in kernel code to know. If the start address allocated by mmap is before the starting program break, and the end address is after the starting program break, then the avc will trigger. However, I don't know how mmap deals with determining an address and if it takes into account the program break, or if calling brk(2) later on will just pick a new location.

Quoted 15 lines

Quoted 2 lines

I'm not a mm expert, but thankfully we have some on the To/CC line so I'm hopeful they will be able to take a look and provide some insight.

+Cc mmap maintainers too.

Quoted 10 lines

To bring the relevant code into this email, prior to using the vma_is_initial_heap() helper the SELinux execheap logic looked like this:

    /* WORKING */
    if (vma->vm_start >= vma->vm_mm->start_brk &&
        vma->vm_end <= vma->vm_mm->brk)
      /* execheap denial */

vma range must be within heap.

Quoted 7 lines

Quoted 7 lines

... while the current vma_is_initial_heap() helper has logic that looks like this:

    /* BUGGY */
    if (vma->vm_start < vma->vm_mm->brk &&
        vma->vm_end > vma->vm_mm->start_brk)
      /* execheap denial */

Quoted 3 lines

vma range intersection with brk range will be rejected, and there's a cross in above case, so execheap denial, I not sure it should be allowed or not, hope maintainers give some comments here.

It's a kernel regression so we need to revert the change to SELinux at least - can't start denying permission without a new/updated policy (e.g. making the change in the check conditional on a new policy capability).

OK，revert patch is sent, but I am also curious about it.

https://lore.kernel.org/all/[email protected]/

Quoted 4 lines · Kefeng Wang

OK，revert patch is sent, but I am also curious about it.

https://lore.kernel.org/all/[email protected]/

I am also curious. It seems like the "real" fix would be in mmap - my understanding is that it should not intersect with heap, even when heap is empty (start_brk == brk).

It looks like start_brk is fixed in place when the ELF is loaded in fs/binfmt_elf.c:load_elf_binary (line 1288).

        if ((current->flags & PF_RANDOMIZE) && (snapshot_randomize_va_space > 1)) {
                /*
                 * For architectures with ELF randomization, when executing
                 * a loader directly (i.e. no interpreter listed in ELF
                 * headers), move the brk area out of the mmap region
                 * (since it grows up, and may collide early with the stack
                 * growing down), and into the unused ELF_ET_DYN_BASE region.
                 */
                if (IS_ENABLED(CONFIG_ARCH_HAS_ELF_RANDOMIZE) &&
                    elf_ex->e_type == ET_DYN && !interpreter) {
                        mm->brk = mm->start_brk = ELF_ET_DYN_BASE;
                } else {
                        /* Otherwise leave a gap between .bss and brk. */

                        mm->brk = mm->start_brk = mm->brk + PAGE_SIZE;
                }

                mm->brk = mm->start_brk = arch_randomize_brk(mm);
#ifdef compat_brk_randomized
                current->brk_randomized = 1;
#endif
        }

Have a look at the mmapstress 3 test in ltp [1]. The tests pokes holes and mmaps into those holes throughout the brk range.

[1]. https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/mem/mmapstress/mmapstress03.c

Quoted 4 lines · Liam R. Howlett

Have a look at the mmapstress 3 test in ltp [1]. The tests pokes holes and mmaps into those holes throughout the brk range.

[1]. https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/mem/mmapstress/mmapstress03.c

In investigating this further, with additional reproducers, I believe that the whole bug is in vma_is_initial_heap().

What do you all think about this patch? If it doesn't have any obvious flaws I can submit it (along with a revert for the revert).

Kefeng Wang (4):
mm: factor out VMA stack and heap checks
drm/amdkfd: use vma_is_initial_stack() and vma_is_initial_heap()
selinux: use vma_is_initial_stack() and vma_is_initial_heap() perf/core: use vma_is_initial_stack() and vma_is_initial_heap()

 drivers/gpu/drm/amd/amdkfd/kfd_svm.c |  5 +----
 fs/proc/task_mmu.c                   | 24 ++++----------------
 fs/proc/task_nommu.c                 | 15 +------------
 include/linux/mm.h                   | 25 +++++++++++++++++++++
 kernel/events/core.c                 | 33 ++++++++++------------------
 security/selinux/hooks.c             |  7 ++----
 6 files changed, 44 insertions(+), 65 deletions(-)