On Thu, Dec 7, 2023 at 5:50 AM Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> wrote: > > > > 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? 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. -- Ondrej Mosnacek Senior Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
