[lkml]   [2012]   [Jan]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] Mark thread stack correctly in proc/<pid>/maps
    Siddhesh Poyarekar wrote:
    > Memory mmaped by glibc for a thread stack currently shows up as a simple
    > anonymous map, which makes it difficult to differentiate between memory
    > usage of the thread on stack and other dynamic allocation. Since glibc
    > already uses MAP_STACK to request this mapping, the attached patch
    > uses this flag to add additional VM_STACK_FLAGS to the resulting vma
    > so that the mapping is treated as a stack and not any regular
    > anonymous mapping. Also, one may use vm_flags to decide if a vma is a
    > stack.

    I think this is fine.

    > There is an additional complication with posix threads where the stack
    > guard for a thread stack may be larger than a page, unlike the case
    > for process stack where the stack guard is a page long. glibc
    > implements these guards by calling mprotect on the beginning page(s)
    > to remove all permissions. I have used this to remove vmas that have
    > the thread stack guard, from the /proc/maps output.

    > - /* We don't show the stack guard page in /proc/maps */
    > + /* We don't show the stack guard pages in /proc/maps */
    > + if (thread_stack_guard(vma))
    > + return;
    > +
    > start = vma->vm_start;
    > if (stack_guard_page_start(vma, start))
    > start += PAGE_SIZE;

    Hmm, I see why you did this. The current code already hides one guard
    page, which is already dubious for programs that do things like read
    /proc/pid/maps to decide if MAP_FIXED would be not clobber an existing
    mapping. At least those programs _could_ know about the stack guard
    page address

    I wonder if it's a potential security hole: You've just allowed
    programs to use two MAP_GROWSUP/DOWN|PROT_NONE to hide vmas from the
    user. Sure, the memory isn't accessible, but it can still store data
    and be ephemerally made visible using mprotect() then hidden again.

    I would prefer a label like "[stack guard]" or just "[guard]",
    both for the thread stacks and the process stack.

    With a label like "[guard]" it needn't be limited to stacks; heap
    guard pages used by some programs would also be labelled.

    > +static inline int vma_is_stack(struct vm_area_struct *vma)
    > +{
    > + return vma && (vma->vm_flags & (VM_GROWSUP | VM_GROWSDOWN));
    > +}
    > +
    > +/*
    > + * POSIX thread stack guards may be more than a page long and access to it
    > + * should return an error (possibly a SIGSEGV). The glibc implementation does
    > + * an mprotect(..., ..., PROT_NONE), so our guard vma has no permissions.
    > + */
    > +static inline int thread_stack_guard(struct vm_area_struct *vma)

    Is there a reason the names aren't consistent - i.e. not vma_is_stack_guard()?

    > +{
    > + return vma_is_stack(vma) &&
    > + ((vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_MAYSHARE)) == 0) &&
    > + vma_is_stack((vma->vm_flags & VM_GROWSDOWN)?vma->vm_next:vma->vm_prev);
    > +}
    > +

    That doesn't check if ->vm_next/prev is adjacent in address space.

    You can't assume the program is using Glibc, or that MAP_STACK
    mappings are all from Glibc, or that they are in the pattern you expect.

    How about simply calling it vma_is_guard(), return 1 if it's PROT_NONE
    without checking vma_is_stack() or ->vm_next/prev, and annotate the
    maps output like this:

    is_stack => "[stack]"
    is_guard & is_stack => "[stack guard]"
    is_guard & !is_stack => "[guard]"

    What do you think?

    -- Jamie

     \ /
      Last update: 2012-01-16 12:31    [W:0.023 / U:4.236 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site