[PATCH] Fix a use after free bug in kernel->userspace relay file support

[PATCH] Fix a use after free bug in kernel->userspace relay file support

[Jesper Juhl]

Hi,

Coverity spotted what looks like a real possible case of using a 
variable after it has been freed.
The problem is in kernel/relay.c::relay_open_buf()

If the code hits "goto free_buf;" it ends up in this code :

  free_buf:
    	relay_destroy_buf(buf);	<--- calls kfree() on 'buf'.
  free_name:
   	kfree(tmpname);
  end:
  	return buf;		<-- use after free of 'buf'.

I read through the callers and they all handle a NULL return 
from this function as an error (and hitting the 'free_buf' label 
only happens on failure to chan->cb->create_buf_file(), so that 
looks like a clear error to me). 

The patch simply sets 'buf' to NULL after the call to 
relay_destroy_buf(buf); - as far as I can see that should take 
care of the problem.

The patch also corrects a reference to a documentation file while 
I was at it.

Warning: I don't know this code at all and I've only read through 
it quickly to try and work out what to do here, so I'd really 
apreciate it if someone who actually knows how this code should 
behave could ACK or NACK the patch before it gets merged anywhere 
since I did this pretty blind.

Patch has been compile tested only.


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

diff --git a/kernel/relay.c b/kernel/relay.c
index a615a8f..c55e399 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -1,7 +1,7 @@
 /*
  * Public API and common code for kernel->userspace relay file support.
  *
- * See Documentation/filesystems/relayfs.txt for an overview of relayfs.
+ * See Documentation/filesystems/relay.txt for an overview.
  *
  * Copyright (C) 2002-2005 - Tom Zanussi (zanussi@us.ibm.com), IBM Corp
  * Copyright (C) 1999-2005 - Karim Yaghmour (karim@opersys.com)
@@ -427,6 +427,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
 
 free_buf:
  	relay_destroy_buf(buf);
+ 	buf = NULL;
 free_name:
  	kfree(tmpname);
 end:

Re: [PATCH] Fix a use after free bug in kernel->userspace relay file support

[David J. Wilder]

ACK
Thanks for catching this. Your patch looks fine. I tested for 
regression, no problems. I also tested the error path and had the 
expected results.
Thanks
Dave

Jesper Juhl wrote:
> Hi,
>
> Coverity spotted what looks like a real possible case of using a 
> variable after it has been freed.
> The problem is in kernel/relay.c::relay_open_buf()
>
> If the code hits "goto free_buf;" it ends up in this code :
>
>   free_buf:
>     	relay_destroy_buf(buf);	<--- calls kfree() on 'buf'.
>   free_name:
>    	kfree(tmpname);
>   end:
>   	return buf;		<-- use after free of 'buf'.
>
> I read through the callers and they all handle a NULL return 
> from this function as an error (and hitting the 'free_buf' label 
> only happens on failure to chan->cb->create_buf_file(), so that 
> looks like a clear error to me). 
>
> The patch simply sets 'buf' to NULL after the call to 
> relay_destroy_buf(buf); - as far as I can see that should take 
> care of the problem.
>
> The patch also corrects a reference to a documentation file while 
> I was at it.
>
> Warning: I don't know this code at all and I've only read through 
> it quickly to try and work out what to do here, so I'd really 
> apreciate it if someone who actually knows how this code should 
> behave could ACK or NACK the patch before it gets merged anywhere 
> since I did this pretty blind.
>
> Patch has been compile tested only.
>
>
> Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> ---
>
> diff --git a/kernel/relay.c b/kernel/relay.c
> index a615a8f..c55e399 100644
> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -1,7 +1,7 @@
>  /*
>   * Public API and common code for kernel->userspace relay file support.
>   *
> - * See Documentation/filesystems/relayfs.txt for an overview of relayfs.
> + * See Documentation/filesystems/relay.txt for an overview.
>   *
>   * Copyright (C) 2002-2005 - Tom Zanussi (zanussi@us.ibm.com), IBM Corp
>   * Copyright (C) 1999-2005 - Karim Yaghmour (karim@opersys.com)
> @@ -427,6 +427,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
>
>  free_buf:
>   	relay_destroy_buf(buf);
> + 	buf = NULL;
>  free_name:
>   	kfree(tmpname);
>  end:
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>   

Re: [PATCH] Fix a use after free bug in kernel->userspace relay file support

[Mathieu Desnoyers]

* Jesper Juhl (jesper.juhl@gmail.com) wrote:
> Hi,
> 
> Coverity spotted what looks like a real possible case of using a 
> variable after it has been freed.
> The problem is in kernel/relay.c::relay_open_buf()
> 
> If the code hits "goto free_buf;" it ends up in this code :
> 
>   free_buf:
>     	relay_destroy_buf(buf);	<--- calls kfree() on 'buf'.
>   free_name:
>    	kfree(tmpname);
>   end:
>   	return buf;		<-- use after free of 'buf'.
> 
> I read through the callers and they all handle a NULL return 
> from this function as an error (and hitting the 'free_buf' label 
> only happens on failure to chan->cb->create_buf_file(), so that 
> looks like a clear error to me). 
> 
> The patch simply sets 'buf' to NULL after the call to 
> relay_destroy_buf(buf); - as far as I can see that should take 
> care of the problem.
> 
> The patch also corrects a reference to a documentation file while 
> I was at it.
> 
> Warning: I don't know this code at all and I've only read through 
> it quickly to try and work out what to do here, so I'd really 
> apreciate it if someone who actually knows how this code should 
> behave could ACK or NACK the patch before it gets merged anywhere 
> since I did this pretty blind.
> 
> Patch has been compile tested only.
> 
> 
> Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>

Yes, the fix seems good. Thanks.

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>

> ---
> 
> diff --git a/kernel/relay.c b/kernel/relay.c
> index a615a8f..c55e399 100644
> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -1,7 +1,7 @@
>  /*
>   * Public API and common code for kernel->userspace relay file support.
>   *
> - * See Documentation/filesystems/relayfs.txt for an overview of relayfs.
> + * See Documentation/filesystems/relay.txt for an overview.
>   *
>   * Copyright (C) 2002-2005 - Tom Zanussi (zanussi@us.ibm.com), IBM Corp
>   * Copyright (C) 1999-2005 - Karim Yaghmour (karim@opersys.com)
> @@ -427,6 +427,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
>  
>  free_buf:
>   	relay_destroy_buf(buf);
> + 	buf = NULL;
>  free_name:
>   	kfree(tmpname);
>  end:
> 
> 

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH] Fix a use after free bug in kernel->userspace relay file support

[Jesper Juhl]

On 19/07/07, David J. Wilder <wilder@us.ibm.com> wrote:
> ACK
> Thanks for catching this. Your patch looks fine. I tested for
> regression, no problems. I also tested the error path and had the
> expected results.

Ok, thank you for testing. I see that Mathieu Desnoyers also ack'ed
the patch (thank you Mathieu). Will you guys take care of getting it
merged upstream or should I resubmit it myself to Linus or Andrew with
your ACKs?


> Thanks
> Dave
>
> Jesper Juhl wrote:
> > Hi,
> >
> > Coverity spotted what looks like a real possible case of using a
> > variable after it has been freed.
> > The problem is in kernel/relay.c::relay_open_buf()
> >
[snip]

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

[PATCH] Fix a use after free bug in kernel->userspace relay file support

[Mathieu Desnoyers]

Coverity spotted what looks like a real possible case of using a 
variable after it has been freed.
The problem is in kernel/relay.c::relay_open_buf()

If the code hits "goto free_buf;" it ends up in this code :

  free_buf:
    	relay_destroy_buf(buf);	<--- calls kfree() on 'buf'.
  free_name:
   	kfree(tmpname);
  end:
  	return buf;		<-- use after free of 'buf'.

I read through the callers and they all handle a NULL return 
from this function as an error (and hitting the 'free_buf' label 
only happens on failure to chan->cb->create_buf_file(), so that 
looks like a clear error to me). 

The patch simply sets 'buf' to NULL after the call to 
relay_destroy_buf(buf); - as far as I can see that should take 
care of the problem.

The patch also corrects a reference to a documentation file while 
I was at it.

Note from Mathieu: the documentation reference change should have been
done in a separate patch, but I guess no one will really care.

Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
Acked-by: "David J. Wilder" <wilder@us.ibm.com>
Tested-by: "David J. Wilder" <wilder@us.ibm.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
---

diff --git a/kernel/relay.c b/kernel/relay.c
index a615a8f..c55e399 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -1,7 +1,7 @@
 /*
  * Public API and common code for kernel->userspace relay file support.
  *
- * See Documentation/filesystems/relayfs.txt for an overview of relayfs.
+ * See Documentation/filesystems/relay.txt for an overview.
  *
  * Copyright (C) 2002-2005 - Tom Zanussi (zanussi@us.ibm.com), IBM Corp
  * Copyright (C) 1999-2005 - Karim Yaghmour (karim@opersys.com)
@@ -427,6 +427,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
 
 free_buf:
  	relay_destroy_buf(buf);
+ 	buf = NULL;
 free_name:
  	kfree(tmpname);
 end:
-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH] Fix a use after free bug in kernel->userspace relay file support

[Andrew Morton]

On Thu, 19 Jul 2007 21:09:09 -0400
Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:

> Coverity spotted what looks like a real possible case of using a 
> variable after it has been freed.
> The problem is in kernel/relay.c::relay_open_buf()
> 
> If the code hits "goto free_buf;" it ends up in this code :
> 
>   free_buf:
>     	relay_destroy_buf(buf);	<--- calls kfree() on 'buf'.
>   free_name:
>    	kfree(tmpname);
>   end:
>   	return buf;		<-- use after free of 'buf'.
> 
> I read through the callers and they all handle a NULL return 
> from this function as an error (and hitting the 'free_buf' label 
> only happens on failure to chan->cb->create_buf_file(), so that 
> looks like a clear error to me). 
> 
> The patch simply sets 'buf' to NULL after the call to 
> relay_destroy_buf(buf); - as far as I can see that should take 
> care of the problem.
> 
> The patch also corrects a reference to a documentation file while 
> I was at it.
> 
> Note from Mathieu: the documentation reference change should have been
> done in a separate patch, but I guess no one will really care.
> 
> Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> Acked-by: "David J. Wilder" <wilder@us.ibm.com>
> Tested-by: "David J. Wilder" <wilder@us.ibm.com>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>

I'm going to infer from all this that the patch was actually written
by Jesper.  Correct?

If so, it shuld have had From: Jesper Juhl <jesper.juhl@gmail.com>
right at the start of the changelog.

Re: [PATCH] Fix a use after free bug in kernel->userspace relay file support

[Mathieu Desnoyers]

* Andrew Morton (akpm@linux-foundation.org) wrote:
> On Thu, 19 Jul 2007 21:09:09 -0400
> Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:
> 
> > Coverity spotted what looks like a real possible case of using a 
> > variable after it has been freed.
> > The problem is in kernel/relay.c::relay_open_buf()
> > 
> > If the code hits "goto free_buf;" it ends up in this code :
> > 
> >   free_buf:
> >     	relay_destroy_buf(buf);	<--- calls kfree() on 'buf'.
> >   free_name:
> >    	kfree(tmpname);
> >   end:
> >   	return buf;		<-- use after free of 'buf'.
> > 
> > I read through the callers and they all handle a NULL return 
> > from this function as an error (and hitting the 'free_buf' label 
> > only happens on failure to chan->cb->create_buf_file(), so that 
> > looks like a clear error to me). 
> > 
> > The patch simply sets 'buf' to NULL after the call to 
> > relay_destroy_buf(buf); - as far as I can see that should take 
> > care of the problem.
> > 
> > The patch also corrects a reference to a documentation file while 
> > I was at it.
> > 
> > Note from Mathieu: the documentation reference change should have been
> > done in a separate patch, but I guess no one will really care.
> > 
> > Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> > Acked-by: "David J. Wilder" <wilder@us.ibm.com>
> > Tested-by: "David J. Wilder" <wilder@us.ibm.com>
> > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> 
> I'm going to infer from all this that the patch was actually written
> by Jesper.  Correct?
> 
> If so, it shuld have had From: Jesper Juhl <jesper.juhl@gmail.com>
> right at the start of the changelog.

Yes, it's the case. Thanks.

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68