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

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2023/12/7 4:47, Paul Moore wrote:
> On Wed, Dec 6, 2023 at 9:22 AM Ondrej Mosnacek <omosnace@redhat.com> 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@huawei.com/)
>>
>> On Tue, Aug 1, 2023 at 1:08 AM Paul Moore <paul@paul-moore.com> wrote:
>>> On Mon, Jul 31, 2023 at 4:02 PM Stephen Smalley
>>> <stephen.smalley.work@gmail.com> wrote:
>>>> On Mon, Jul 31, 2023 at 12:19 PM Paul Moore <paul@paul-moore.com> wrote:
>>>>>
>>>>> On Mon, Jul 31, 2023 at 10:26 AM Stephen Smalley
>>>>> <stephen.smalley.work@gmail.com> 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.
>