[patch] perfmon ia64: fix file/vma lifetime

[patch] perfmon ia64: fix file/vma lifetime

[Nick Piggin]

Patch is against 2.6.20.

Nick
--
From: Nick Piggin <npiggin@suse.de>

Perfmon associates vmalloc()ed memory with a file descriptor, and installs
a vma mapping that memory. Unfortunately, the vm_file field is not filled in,
so processes with mappings to that memory do not prevent the file from being
closed and the memory freed. This results in use-after-free bugs and multiple
freeing of pages, etc.

I saw this bug on an Altix on SLES9. Haven't reproduced upstream but it looks
like the same issue is there.

Signed-off-by: Nick Piggin <npiggin@suse.de>

Index: linux-2.6/arch/ia64/kernel/perfmon.c
===================================================================
--- linux-2.6.orig/arch/ia64/kernel/perfmon.c
+++ linux-2.6/arch/ia64/kernel/perfmon.c
@@ -2261,7 +2261,7 @@ pfm_remap_buffer(struct vm_area_struct *
  * allocate a sampling buffer and remaps it into the user address space of the task
  */
 static int
-pfm_smpl_buffer_alloc(struct task_struct *task, pfm_context_t *ctx, unsigned long rsize, void **user_vaddr)
+pfm_smpl_buffer_alloc(struct task_struct *task, struct file *filp, pfm_context_t *ctx, unsigned long rsize, void **user_vaddr)
 {
 	struct mm_struct *mm = task->mm;
 	struct vm_area_struct *vma = NULL;
@@ -2312,6 +2312,7 @@ pfm_smpl_buffer_alloc(struct task_struct
 	 * partially initialize the vma for the sampling buffer
 	 */
 	vma->vm_mm	     = mm;
+	vma->vm_file	     = filp;
 	vma->vm_flags	     = VM_READ| VM_MAYREAD |VM_RESERVED;
 	vma->vm_page_prot    = PAGE_READONLY; /* XXX may need to change */
 
@@ -2350,6 +2351,8 @@ pfm_smpl_buffer_alloc(struct task_struct
 		goto error;
 	}
 
+	get_file(filp);
+
 	/*
 	 * now insert the vma in the vm list for the process, must be
 	 * done with mmap lock held
@@ -2427,7 +2430,7 @@ pfarg_is_sane(struct task_struct *task, 
 }
 
 static int
-pfm_setup_buffer_fmt(struct task_struct *task, pfm_context_t *ctx, unsigned int ctx_flags,
+pfm_setup_buffer_fmt(struct task_struct *task, struct file *filp, pfm_context_t *ctx, unsigned int ctx_flags,
 		     unsigned int cpu, pfarg_context_t *arg)
 {
 	pfm_buffer_fmt_t *fmt = NULL;
@@ -2468,7 +2471,7 @@ pfm_setup_buffer_fmt(struct task_struct 
 		/*
 		 * buffer is always remapped into the caller's address space
 		 */
-		ret = pfm_smpl_buffer_alloc(current, ctx, size, &uaddr);
+		ret = pfm_smpl_buffer_alloc(current, filp, ctx, size, &uaddr);
 		if (ret) goto error;
 
 		/* keep track of user address of buffer */
@@ -2679,7 +2682,7 @@ pfm_context_create(pfm_context_t *ctx, v
 	 * does the user want to sample?
 	 */
 	if (pfm_uuid_cmp(req->ctx_smpl_buf_id, pfm_null_uuid)) {
-		ret = pfm_setup_buffer_fmt(current, ctx, ctx_flags, 0, req);
+		ret = pfm_setup_buffer_fmt(current, filp, ctx, ctx_flags, 0, req);
 		if (ret) goto buffer_error;
 	}
 

Re: [patch] perfmon ia64: fix file/vma lifetime

[Stephane Eranian]

nick,

On Tue, Feb 20, 2007 at 03:18:56PM +0100, Nick Piggin wrote:
> From: Nick Piggin <npiggin@suse.de>
> 
> Perfmon associates vmalloc()ed memory with a file descriptor, and installs
> a vma mapping that memory. Unfortunately, the vm_file field is not filled in,
> so processes with mappings to that memory do not prevent the file from being
> closed and the memory freed. This results in use-after-free bugs and multiple
> freeing of pages, etc.
> 
> I saw this bug on an Altix on SLES9. Haven't reproduced upstream but it looks
> like the same issue is there.
> 

I think this is possible for the old perfmon v2.0 codebase that is currently in
mainline for IA-64. I have corrected this with the multi-arch v2.3 code base 
available as a  kernel patch for the moment.

Thanks.

> Signed-off-by: Nick Piggin <npiggin@suse.de>
> 
> Index: linux-2.6/arch/ia64/kernel/perfmon.c
> ===================================================================
> --- linux-2.6.orig/arch/ia64/kernel/perfmon.c
> +++ linux-2.6/arch/ia64/kernel/perfmon.c
> @@ -2261,7 +2261,7 @@ pfm_remap_buffer(struct vm_area_struct *
>   * allocate a sampling buffer and remaps it into the user address space of the task
>   */
>  static int
> -pfm_smpl_buffer_alloc(struct task_struct *task, pfm_context_t *ctx, unsigned long rsize, void **user_vaddr)
> +pfm_smpl_buffer_alloc(struct task_struct *task, struct file *filp, pfm_context_t *ctx, unsigned long rsize, void **user_vaddr)
>  {
>  	struct mm_struct *mm = task->mm;
>  	struct vm_area_struct *vma = NULL;
> @@ -2312,6 +2312,7 @@ pfm_smpl_buffer_alloc(struct task_struct
>  	 * partially initialize the vma for the sampling buffer
>  	 */
>  	vma->vm_mm	     = mm;
> +	vma->vm_file	     = filp;
>  	vma->vm_flags	     = VM_READ| VM_MAYREAD |VM_RESERVED;
>  	vma->vm_page_prot    = PAGE_READONLY; /* XXX may need to change */
>  
> @@ -2350,6 +2351,8 @@ pfm_smpl_buffer_alloc(struct task_struct
>  		goto error;
>  	}
>  
> +	get_file(filp);
> +
>  	/*
>  	 * now insert the vma in the vm list for the process, must be
>  	 * done with mmap lock held
> @@ -2427,7 +2430,7 @@ pfarg_is_sane(struct task_struct *task, 
>  }
>  
>  static int
> -pfm_setup_buffer_fmt(struct task_struct *task, pfm_context_t *ctx, unsigned int ctx_flags,
> +pfm_setup_buffer_fmt(struct task_struct *task, struct file *filp, pfm_context_t *ctx, unsigned int ctx_flags,
>  		     unsigned int cpu, pfarg_context_t *arg)
>  {
>  	pfm_buffer_fmt_t *fmt = NULL;
> @@ -2468,7 +2471,7 @@ pfm_setup_buffer_fmt(struct task_struct 
>  		/*
>  		 * buffer is always remapped into the caller's address space
>  		 */
> -		ret = pfm_smpl_buffer_alloc(current, ctx, size, &uaddr);
> +		ret = pfm_smpl_buffer_alloc(current, filp, ctx, size, &uaddr);
>  		if (ret) goto error;
>  
>  		/* keep track of user address of buffer */
> @@ -2679,7 +2682,7 @@ pfm_context_create(pfm_context_t *ctx, v
>  	 * does the user want to sample?
>  	 */
>  	if (pfm_uuid_cmp(req->ctx_smpl_buf_id, pfm_null_uuid)) {
> -		ret = pfm_setup_buffer_fmt(current, ctx, ctx_flags, 0, req);
> +		ret = pfm_setup_buffer_fmt(current, filp, ctx, ctx_flags, 0, req);
>  		if (ret) goto buffer_error;
>  	}
>  

-- 

-Stephane

Re: [patch] perfmon ia64: fix file/vma lifetime

[Nick Piggin]

On Tue, Feb 20, 2007 at 06:34:54AM -0800, Stephane Eranian wrote:
> nick,
> 
> On Tue, Feb 20, 2007 at 03:18:56PM +0100, Nick Piggin wrote:
> > From: Nick Piggin <npiggin@suse.de>
> > 
> > Perfmon associates vmalloc()ed memory with a file descriptor, and installs
> > a vma mapping that memory. Unfortunately, the vm_file field is not filled in,
> > so processes with mappings to that memory do not prevent the file from being
> > closed and the memory freed. This results in use-after-free bugs and multiple
> > freeing of pages, etc.
> > 
> > I saw this bug on an Altix on SLES9. Haven't reproduced upstream but it looks
> > like the same issue is there.
> > 
> 
> I think this is possible for the old perfmon v2.0 codebase that is currently in
> mainline for IA-64.

OK, I take that as an Ack? (the patch definitely applies, I just can't
get the Altix to boot 2.6.20 to verify it).

> I have corrected this with the multi-arch v2.3 code base 
> available as a  kernel patch for the moment.

Not that I've looked at the code, but can I be hopeful that v2.3
using the traditional mmap file operation to set up the vma and map
in pages, rather than the way that v2.0 works?

Or is there some particular reason why you avoided that?

Thanks,
Nick

Re: [patch] perfmon ia64: fix file/vma lifetime

[Stephane Eranian]

Nick,

On Tue, Feb 20, 2007 at 04:08:45PM +0100, Nick Piggin wrote:
> On Tue, Feb 20, 2007 at 06:34:54AM -0800, Stephane Eranian wrote:
> > nick,
> > 
> > On Tue, Feb 20, 2007 at 03:18:56PM +0100, Nick Piggin wrote:
> > > From: Nick Piggin <npiggin@suse.de>
> > > 
> > > Perfmon associates vmalloc()ed memory with a file descriptor, and installs
> > > a vma mapping that memory. Unfortunately, the vm_file field is not filled in,
> > > so processes with mappings to that memory do not prevent the file from being
> > > closed and the memory freed. This results in use-after-free bugs and multiple
> > > freeing of pages, etc.
> > > 
> > > I saw this bug on an Altix on SLES9. Haven't reproduced upstream but it looks
> > > like the same issue is there.
> > > 
> > 
> > I think this is possible for the old perfmon v2.0 codebase that is currently in
> > mainline for IA-64.
> 
> OK, I take that as an Ack? (the patch definitely applies, I just can't
> get the Altix to boot 2.6.20 to verify it).
> 
> > I have corrected this with the multi-arch v2.3 code base 
> > available as a  kernel patch for the moment.
> 
> Not that I've looked at the code, but can I be hopeful that v2.3
> using the traditional mmap file operation to set up the vma and map
> in pages, rather than the way that v2.0 works?
> 

It does. You need an explicit mmap()  call.

-- 

-Stephane

Re: [patch] perfmon ia64: fix file/vma lifetime

[Nick Piggin]

On Tue, Feb 20, 2007 at 07:14:08AM -0800, Stephane Eranian wrote:
> Nick,
> 
> > Not that I've looked at the code, but can I be hopeful that v2.3
> > using the traditional mmap file operation to set up the vma and map
> > in pages, rather than the way that v2.0 works?
> > 
> 
> It does. You need an explicit mmap()  call.

Good to hear, thanks Stephane.