[PATCH 2/10] Fix leaking of kernel heap addresses in net/

[PATCH 2/10] Fix leaking of kernel heap addresses in net/

[Dan Rosenberg]

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 08ffe9e..5960ad7 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -165,9 +165,16 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 	struct bcm_sock *bo = bcm_sk(sk);
 	struct bcm_op *op;
 
-	seq_printf(m, ">>> socket %p", sk->sk_socket);
-	seq_printf(m, " / sk %p", sk);
-	seq_printf(m, " / bo %p", bo);
+	/* Only expose kernel addresses to privileged readers */
+	if (capable(CAP_NET_ADMIN))
+		seq_printf(m, ">>> socket %p", sk->sk_socket);
+		seq_printf(m, " / sk %p", sk);
+		seq_printf(m, " / bo %p", bo);
+	else
+		seq_printf(m, ">>> socket %lu", sock_i_ino(sk));
+		seq_printf(m, " / sk %d", 0);
+		seq_printf(m, " / bo %d", 0);
+
 	seq_printf(m, " / dropped %lu", bo->dropped_usr_msgs);
 	seq_printf(m, " / bound %s", bcm_proc_getifname(ifname, bo->ifindex));
 	seq_printf(m, " <<<\n");
@@ -1520,8 +1527,8 @@ static int bcm_connect(struct socket *sock, struct sockaddr *uaddr, int len,
 	bo->bound = 1;
 
 	if (proc_dir) {
-		/* unique socket address as filename */
-		sprintf(bo->procname, "%p", sock);
+		/* unique socket inode as filename */
+		sprintf(bo->procname, "%lx", sock_i_ino(sk));
 		bo->bcm_proc_read = proc_create_data(bo->procname, 0644,
 						     proc_dir,
 						     &bcm_proc_fops, sk);

Re: [PATCH 2/10] Fix leaking of kernel heap addresses in net/

[Stephen Hemminger]

On Thu, 11 Nov 2010 20:06:59 -0500
Dan Rosenberg <drosenberg@vsecurity.com> wrote:

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 08ffe9e..5960ad7 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -165,9 +165,16 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>  	struct bcm_sock *bo = bcm_sk(sk);
>  	struct bcm_op *op;
>  
> -	seq_printf(m, ">>> socket %p", sk->sk_socket);
> -	seq_printf(m, " / sk %p", sk);
> -	seq_printf(m, " / bo %p", bo);
> +	/* Only expose kernel addresses to privileged readers */
> +	if (capable(CAP_NET_ADMIN))
> +		seq_printf(m, ">>> socket %p", sk->sk_socket);
> +		seq_printf(m, " / sk %p", sk);
> +		seq_printf(m, " / bo %p", bo);
> +	else
> +		seq_printf(m, ">>> socket %lu", sock_i_ino(sk));
> +		seq_printf(m, " / sk %d", 0);
> +		seq_printf(m, " / bo %d", 0);
> +

Printing different data based on security state seems like an ABI
nightmare.


-- 

Re: [PATCH 2/10] Fix leaking of kernel heap addresses in net/

[Dan Rosenberg]

> 
> Printing different data based on security state seems like an ABI
> nightmare.
> 

I can't remove the data entirely, because that would seriously break the
ABI.  I deliberately kept the same format so as not to break any
userspace programs relying on consistent output - are there really
programs that would break when they read a 0 instead of an address?

-Dan

Re: [PATCH 2/10] Fix leaking of kernel heap addresses in net/

[Ben Hutchings]

On Thu, 2010-11-11 at 20:06 -0500, Dan Rosenberg wrote:
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 08ffe9e..5960ad7 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[...]
> +		seq_printf(m, ">>> socket %lu", sock_i_ino(sk));

Why decimal here...

[...]
> +		/* unique socket inode as filename */
> +		sprintf(bo->procname, "%lx", sock_i_ino(sk));

...and hexadecimal here?

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 2/10] Fix leaking of kernel heap addresses in net/

[Dan Rosenberg]

> On Thu, 2010-11-11 at 20:06 -0500, Dan Rosenberg wrote:
> > diff --git a/net/can/bcm.c b/net/can/bcm.c
> > index 08ffe9e..5960ad7 100644
> > --- a/net/can/bcm.c
> > +++ b/net/can/bcm.c
> [...]
> > +		seq_printf(m, ">>> socket %lu", sock_i_ino(sk));
> 
> Why decimal here...
> 
> [...]
> > +		/* unique socket inode as filename */
> > +		sprintf(bo->procname, "%lx", sock_i_ino(sk));
> 
> ...and hexadecimal here?
> 
> Ben.
> 

You're right, it should be consistent.  I avoided decimal in the /proc
filename because it may be too long - the next version will do the same
for the seq_print output.

-Dan