linux-kernel - Re: perf/jit doesn't cope well with mprotect() to jit containing pages

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]

Date:   Thu, 26 Jan 2017 15:04:01 -0800
From:   Andres Freund <andres@...razel.de>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     linux-kernel@...r.kernel.org,
        Stephane Eranian <eranian@...gle.com>, acme@...nel.org,
        jolsa@...hat.com, mingo@...e.hu, anton@...abs.org,
        namhyung@...nel.org, Stephane Eranian <eranian@...il.com>
Subject: Re: perf/jit doesn't cope well with mprotect() to jit containing
 pages

On 2017-01-26 23:15:08 +0100, Peter Zijlstra wrote:
> On Mon, Dec 12, 2016 at 09:49:03AM +0100, Peter Zijlstra wrote:
> 
> > > BTW, it's also a bit weird that those MMAP2 records triggered by
> > > mprotect/mmap, have prot set to 0...
> > 
> > Yes, mprotect() does: vma->vm_flags = newflags; before calling
> > perf_event_mmap(vma); which then looks at VM_{READ,WRITE,EXEC} bits in
> > that word to generate the prot value.
> > 
> > So that being 0 is a bit weird.
> 
> OK, that was a silly bug :/

Heh, that explains that part.  Thanks for looking into it!


> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 896aa53..e67c5ba 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6660,6 +6683,27 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
>  	char *buf = NULL;
>  	char *name;
>  
> +	if (vma->vm_flags & VM_READ)
> +		prot |= PROT_READ;
> +	if (vma->vm_flags & VM_WRITE)
> +		prot |= PROT_WRITE;
> +	if (vma->vm_flags & VM_EXEC)
> +		prot |= PROT_EXEC;
> +
> +	if (vma->vm_flags & VM_MAYSHARE)
> +		flags = MAP_SHARED;
> +	else
> +		flags = MAP_PRIVATE;
> +
> +	if (vma->vm_flags & VM_DENYWRITE)
> +		flags |= MAP_DENYWRITE;
> +	if (vma->vm_flags & VM_MAYEXEC)
> +		flags |= MAP_EXECUTABLE;
> +	if (vma->vm_flags & VM_LOCKED)
> +		flags |= MAP_LOCKED;
> +	if (vma->vm_flags & VM_HUGETLB)
> +		flags |= MAP_HUGETLB;
> +
>  	if (file) {
>  		struct inode *inode;
>  		dev_t dev;
> @@ -6686,27 +6730,6 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
>  		maj = MAJOR(dev);
>  		min = MINOR(dev);
>  
> -		if (vma->vm_flags & VM_READ)
> -			prot |= PROT_READ;
> -		if (vma->vm_flags & VM_WRITE)
> -			prot |= PROT_WRITE;
> -		if (vma->vm_flags & VM_EXEC)
> -			prot |= PROT_EXEC;
> -
> -		if (vma->vm_flags & VM_MAYSHARE)
> -			flags = MAP_SHARED;
> -		else
> -			flags = MAP_PRIVATE;
> -
> -		if (vma->vm_flags & VM_DENYWRITE)
> -			flags |= MAP_DENYWRITE;
> -		if (vma->vm_flags & VM_MAYEXEC)
> -			flags |= MAP_EXECUTABLE;
> -		if (vma->vm_flags & VM_LOCKED)
> -			flags |= MAP_LOCKED;
> -		if (vma->vm_flags & VM_HUGETLB)
> -			flags |= MAP_HUGETLB;
> -
>  		goto got_name;
>  	} else {
>  		if (vma->vm_ops && vma->vm_ops->name) {

I'm not a mm/perf (or kernel in general) expert. But this looks
obviously buggy to me. And the fix makes it look correct. So if that's
helpful (I'm not actually sure):
Signed-off-by: Andres Freund <andres@...razel.de>

Not sure if this on its own has enough consequences to be worth being
put into stable?

Not that it's this fix's responsibility, but I did note that that piece
of code doesn't maintain MAP_GROWSDOWN, nor set the bits controlling the
size of the HUGETLB mapping.  Seems harmless enough, but given that I
already looked I thought it'd bring it up.

Greetings,

Andres Freund