linux-kernel - Re: [PATCH] Mark thread stack correctly in proc/<pid>/maps

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120116112802.GB7180@jl-vm1.vm.bytemark.co.uk>
Date:	Mon, 16 Jan 2012 11:28:02 +0000
From:	Jamie Lokier <jamie@...reable.org>
To:	Siddhesh Poyarekar <siddhesh.poyarekar@...il.com>
Cc:	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org,
	Michael Kerrisk <mtk.manpages@...il.com>,
	linux-man@...r.kernel.org
Subject: Re: [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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/