Patchwork [1/1] net: ax25: fix information leak to userland

login
register
mail settings
Submitter Andy Whitcroft
Date Jan. 31, 2011, 4:20 p.m.
Message ID <1296490834-14335-2-git-send-email-apw@canonical.com>
Download mbox | patch
Permalink /patch/81176/
State Accepted
Commit 38f1c9ea96ac3917583aa727389f5958d34738be
Headers show

Comments

Andy Whitcroft - Jan. 31, 2011, 4:20 p.m.
From: Vasiliy Kulikov <segooon@gmail.com>

Sometimes ax25_getname() doesn't initialize all members of fsa_digipeater
field of fsa struct, also the struct has padding bytes between
sax25_call and sax25_ndigis fields.  This structure is then copied to
userland.  It leads to leaking of contents of kernel stack memory.

Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

CVE-2010-3875
BugLink: http://bugs.launchpad.net/bugs/710714
(cherry picked from commit fe10ae53384e48c51996941b7720ee16995cbcb7 upstream)
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 net/ax25/af_ax25.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Tim Gardner - Jan. 31, 2011, 4:25 p.m.
On 01/31/2011 09:20 AM, Andy Whitcroft wrote:
> From: Vasiliy Kulikov<segooon@gmail.com>
>
> Sometimes ax25_getname() doesn't initialize all members of fsa_digipeater
> field of fsa struct, also the struct has padding bytes between
> sax25_call and sax25_ndigis fields.  This structure is then copied to
> userland.  It leads to leaking of contents of kernel stack memory.
>
> Signed-off-by: Vasiliy Kulikov<segooon@gmail.com>
> Signed-off-by: David S. Miller<davem@davemloft.net>
>
> CVE-2010-3875
> BugLink: http://bugs.launchpad.net/bugs/710714
> (cherry picked from commit fe10ae53384e48c51996941b7720ee16995cbcb7 upstream)
> Signed-off-by: Andy Whitcroft<apw@canonical.com>
> ---
>   net/ax25/af_ax25.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> index cfdfd7e..6e2371a 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -1392,6 +1392,7 @@ static int ax25_getname(struct socket *sock, struct sockaddr *uaddr,
>   	ax25_cb *ax25;
>   	int err = 0;
>
> +	memset(fsa, 0, sizeof(fsa));
>   	lock_sock(sk);
>   	ax25 = ax25_sk(sk);
>
> @@ -1403,7 +1404,6 @@ static int ax25_getname(struct socket *sock, struct sockaddr *uaddr,
>
>   		fsa->fsa_ax25.sax25_family = AF_AX25;
>   		fsa->fsa_ax25.sax25_call   = ax25->dest_addr;
> -		fsa->fsa_ax25.sax25_ndigis = 0;
>
>   		if (ax25->digipeat != NULL) {
>   			ndigi = ax25->digipeat->ndigi;

shouldn't that be 'memset(fsa, 0, sizeof(*fsa));' ?
Andy Whitcroft - Jan. 31, 2011, 4:35 p.m.
On Mon, Jan 31, 2011 at 09:25:26AM -0700, Tim Gardner wrote:
> On 01/31/2011 09:20 AM, Andy Whitcroft wrote:
> >From: Vasiliy Kulikov<segooon@gmail.com>
> >
> >Sometimes ax25_getname() doesn't initialize all members of fsa_digipeater
> >field of fsa struct, also the struct has padding bytes between
> >sax25_call and sax25_ndigis fields.  This structure is then copied to
> >userland.  It leads to leaking of contents of kernel stack memory.
> >
> >Signed-off-by: Vasiliy Kulikov<segooon@gmail.com>
> >Signed-off-by: David S. Miller<davem@davemloft.net>
> >
> >CVE-2010-3875
> >BugLink: http://bugs.launchpad.net/bugs/710714
> >(cherry picked from commit fe10ae53384e48c51996941b7720ee16995cbcb7 upstream)
> >Signed-off-by: Andy Whitcroft<apw@canonical.com>
> >---
> >  net/ax25/af_ax25.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> >diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> >index cfdfd7e..6e2371a 100644
> >--- a/net/ax25/af_ax25.c
> >+++ b/net/ax25/af_ax25.c
> >@@ -1392,6 +1392,7 @@ static int ax25_getname(struct socket *sock, struct sockaddr *uaddr,
> >  	ax25_cb *ax25;
> >  	int err = 0;
> >
> >+	memset(fsa, 0, sizeof(fsa));
> >  	lock_sock(sk);
> >  	ax25 = ax25_sk(sk);
> >
> >@@ -1403,7 +1404,6 @@ static int ax25_getname(struct socket *sock, struct sockaddr *uaddr,
> >
> >  		fsa->fsa_ax25.sax25_family = AF_AX25;
> >  		fsa->fsa_ax25.sax25_call   = ax25->dest_addr;
> >-		fsa->fsa_ax25.sax25_ndigis = 0;
> >
> >  		if (ax25->digipeat != NULL) {
> >  			ndigi = ax25->digipeat->ndigi;
> 
> shouldn't that be 'memset(fsa, 0, sizeof(*fsa));' ?

Yes indeed, good spot.  There even is an upstream fix for the fix.  I'll
respin these.

-apw

Patch

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index cfdfd7e..6e2371a 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -1392,6 +1392,7 @@  static int ax25_getname(struct socket *sock, struct sockaddr *uaddr,
 	ax25_cb *ax25;
 	int err = 0;
 
+	memset(fsa, 0, sizeof(fsa));
 	lock_sock(sk);
 	ax25 = ax25_sk(sk);
 
@@ -1403,7 +1404,6 @@  static int ax25_getname(struct socket *sock, struct sockaddr *uaddr,
 
 		fsa->fsa_ax25.sax25_family = AF_AX25;
 		fsa->fsa_ax25.sax25_call   = ax25->dest_addr;
-		fsa->fsa_ax25.sax25_ndigis = 0;
 
 		if (ax25->digipeat != NULL) {
 			ndigi = ax25->digipeat->ndigi;