Patchwork Fix CAN info leak/minor heap overflow

login
register
mail settings
Submitter Urs Thuermann
Date Nov. 5, 2010, 6:33 p.m.
Message ID <ygfiq0bsjry.fsf@janus.isnogud.escape.de>
Download mbox | patch
Permalink /patch/70282/
State Superseded
Delegated to: David Miller
Headers show

Comments

Urs Thuermann - Nov. 5, 2010, 6:33 p.m.
This patch removes the leakage of kernel space addresses to userspace.
Instead, socket inode numbers are used to create unique proc file
names for CAN_BCM sockets and for referring to sockets in filter
lists.  In addition, this makes debugging easier, since inode numbers
are also shown in ls -l /proc/<pid>/fd/<fd> and lsof(8) output.

BTW, if kernel space addresses are considered security critical
information one should also take a look and possibly change

    /proc/net/{tcp,tcp6,udp,udp6,raw,raw6,unix}

and maybe some others.

The change of the procfs content leads to a new version string
20101105.

Signed-off-by: Urs Thuermann <urs@isnogud.escape.de>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
CC: Dan Rosenberg <drosenberg@vsecurity.com>
CC: Linus Torvalds <torvalds@linux-foundation.org>

---

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Hartkopp - Nov. 9, 2010, 7:52 a.m.
On 05.11.2010 19:33, Urs Thuermann wrote:
> This patch removes the leakage of kernel space addresses to userspace.
> Instead, socket inode numbers are used to create unique proc file
> names for CAN_BCM sockets and for referring to sockets in filter
> lists.  In addition, this makes debugging easier, since inode numbers
> are also shown in ls -l /proc/<pid>/fd/<fd> and lsof(8) output.
> 
> BTW, if kernel space addresses are considered security critical
> information one should also take a look and possibly change
> 
>     /proc/net/{tcp,tcp6,udp,udp6,raw,raw6,unix}
> 
> and maybe some others.
> 
> The change of the procfs content leads to a new version string
> 20101105.
> 
> Signed-off-by: Urs Thuermann <urs@isnogud.escape.de>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

Besides the ongoing(?) discussion about the exposed kernel addresses in procfs
- what are your plans about this patch that already moves the kernel addresses
to inode numbers?

Is it something for net-2.6 / net-next-2.6 / stable ?

Especially in this case we do not see any problems with userspace tools that
could break as it would be for some other /proc/net entries.

Once this patch is applied (and the procfs layout is changed anyway), i'd also
like to send a patch from my backlog that would extend the procfs output for
can-bcm with an additional drop counter.

Best regards,
Oliver


> CC: Dan Rosenberg <drosenberg@vsecurity.com>
> CC: Linus Torvalds <torvalds@linux-foundation.org>
> 
> ---
> 
> diff --git a/include/linux/can/core.h b/include/linux/can/core.h
> index 6c507be..e20a841 100644
> --- a/include/linux/can/core.h
> +++ b/include/linux/can/core.h
> @@ -19,7 +19,7 @@
>  #include <linux/skbuff.h>
>  #include <linux/netdevice.h>
>  
> -#define CAN_VERSION "20090105"
> +#define CAN_VERSION "20101105"
>  
>  /* increment this number each time you change some user-space interface */
>  #define CAN_ABI_VERSION "8"
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 08ffe9e..0e81e04 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -86,6 +86,12 @@ MODULE_LICENSE("Dual BSD/GPL");
>  MODULE_AUTHOR("Oliver Hartkopp <oliver.hartkopp@volkswagen.de>");
>  MODULE_ALIAS("can-proto-2");
>  
> +/*
> + * Point to the sockets inode number inside the bcm ident string.
> + * We skip the string length of "bcm " (== 4) created in bcm_init().
> + */
> +#define INODENUM(bo) (bo->ident + 4)
> +
>  /* easy access to can_frame payload */
>  static inline u64 GET_U64(const struct can_frame *cp)
>  {
> @@ -125,7 +131,7 @@ struct bcm_sock {
>  	struct list_head tx_ops;
>  	unsigned long dropped_usr_msgs;
>  	struct proc_dir_entry *bcm_proc_read;
> -	char procname [9]; /* pointer printed in ASCII with \0 */
> +	char ident[32];
>  };
>  
>  static inline struct bcm_sock *bcm_sk(const struct sock *sk)
> @@ -165,9 +171,7 @@ 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);
> +	seq_printf(m, ">>> socket inode %s", INODENUM(bo));
>  	seq_printf(m, " / dropped %lu", bo->dropped_usr_msgs);
>  	seq_printf(m, " / bound %s", bcm_proc_getifname(ifname, bo->ifindex));
>  	seq_printf(m, " <<<\n");
> @@ -1168,7 +1172,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  				err = can_rx_register(dev, op->can_id,
>  						      REGMASK(op->can_id),
>  						      bcm_rx_handler, op,
> -						      "bcm");
> +						      bo->ident);
>  
>  				op->rx_reg_dev = dev;
>  				dev_put(dev);
> @@ -1177,7 +1181,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		} else
>  			err = can_rx_register(NULL, op->can_id,
>  					      REGMASK(op->can_id),
> -					      bcm_rx_handler, op, "bcm");
> +					      bcm_rx_handler, op, bo->ident);
>  		if (err) {
>  			/* this bcm rx op is broken -> remove it */
>  			list_del(&op->list);
> @@ -1402,6 +1406,8 @@ static int bcm_init(struct sock *sk)
>  {
>  	struct bcm_sock *bo = bcm_sk(sk);
>  
> +	snprintf(bo->ident, sizeof(bo->ident), "bcm %lu", sock_i_ino(sk));
> +
>  	bo->bound            = 0;
>  	bo->ifindex          = 0;
>  	bo->dropped_usr_msgs = 0;
> @@ -1466,7 +1472,7 @@ static int bcm_release(struct socket *sock)
>  
>  	/* remove procfs entry */
>  	if (proc_dir && bo->bcm_proc_read)
> -		remove_proc_entry(bo->procname, proc_dir);
> +		remove_proc_entry(INODENUM(bo), proc_dir);
>  
>  	/* remove device reference */
>  	if (bo->bound) {
> @@ -1519,13 +1525,11 @@ 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);
> -		bo->bcm_proc_read = proc_create_data(bo->procname, 0644,
> +	/* use unique socket inode number as filename */
> +	if (proc_dir)
> +		bo->bcm_proc_read = proc_create_data(INODENUM(bo), 0644,
>  						     proc_dir,
>  						     &bcm_proc_fops, sk);
> -	}
>  
>  	return 0;
>  }
> diff --git a/net/can/proc.c b/net/can/proc.c
> index f4265cc..15bed1c 100644
> --- a/net/can/proc.c
> +++ b/net/can/proc.c
> @@ -204,23 +204,17 @@ static void can_print_rcvlist(struct seq_file *m, struct hlist_head *rx_list,
>  
>  	hlist_for_each_entry_rcu(r, n, rx_list, list) {
>  		char *fmt = (r->can_id & CAN_EFF_FLAG)?
> -			"   %-5s  %08X  %08x  %08x  %08x  %8ld  %s\n" :
> -			"   %-5s     %03X    %08x  %08lx  %08lx  %8ld  %s\n";
> +			"   %-5s  %08X  %08x  %8ld   %s\n" :
> +			"   %-5s     %03X    %08x  %8ld   %s\n";
>  
>  		seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
> -				(unsigned long)r->func, (unsigned long)r->data,
>  				r->matches, r->ident);
>  	}
>  }
>  
>  static void can_print_recv_banner(struct seq_file *m)
>  {
> -	/*
> -	 *                  can1.  00000000  00000000  00000000
> -	 *                 .......          0  tp20
> -	 */
> -	seq_puts(m, "  device   can_id   can_mask  function"
> -			"  userdata   matches  ident\n");
> +	seq_puts(m, "  device   can_id   can_mask   matches   ident\n");
>  }
>  
>  static int can_stats_proc_show(struct seq_file *m, void *v)
> diff --git a/net/can/raw.c b/net/can/raw.c
> index e88f610..e057f0d 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -88,6 +88,7 @@ struct raw_sock {
>  	struct can_filter dfilter; /* default/single filter */
>  	struct can_filter *filter; /* pointer to filter(s) */
>  	can_err_mask_t err_mask;
> +	char ident[32];
>  };
>  
>  /*
> @@ -154,13 +155,14 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
>  static int raw_enable_filters(struct net_device *dev, struct sock *sk,
>  			      struct can_filter *filter, int count)
>  {
> +	struct raw_sock *ro = raw_sk(sk);
>  	int err = 0;
>  	int i;
>  
>  	for (i = 0; i < count; i++) {
>  		err = can_rx_register(dev, filter[i].can_id,
>  				      filter[i].can_mask,
> -				      raw_rcv, sk, "raw");
> +				      raw_rcv, sk, ro->ident);
>  		if (err) {
>  			/* clean up successfully registered filters */
>  			while (--i >= 0)
> @@ -177,11 +179,12 @@ static int raw_enable_filters(struct net_device *dev, struct sock *sk,
>  static int raw_enable_errfilter(struct net_device *dev, struct sock *sk,
>  				can_err_mask_t err_mask)
>  {
> +	struct raw_sock *ro = raw_sk(sk);
>  	int err = 0;
>  
>  	if (err_mask)
>  		err = can_rx_register(dev, 0, err_mask | CAN_ERR_FLAG,
> -				      raw_rcv, sk, "raw");
> +				      raw_rcv, sk, ro->ident);
>  
>  	return err;
>  }
> @@ -281,6 +284,8 @@ static int raw_init(struct sock *sk)
>  {
>  	struct raw_sock *ro = raw_sk(sk);
>  
> +	snprintf(ro->ident, sizeof(ro->ident), "raw %lu", sock_i_ino(sk));
> +
>  	ro->bound            = 0;
>  	ro->ifindex          = 0;
>  

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 9, 2010, 5:05 p.m.
From: Oliver Hartkopp <socketcan@hartkopp.net>
Date: Tue, 09 Nov 2010 08:52:21 +0100

> Once this patch is applied (and the procfs layout is changed anyway), i'd also
> like to send a patch from my backlog that would extend the procfs output for
> can-bcm with an additional drop counter.

I find this kind of discussion extremely disappointing.

All of this stuff you CAN guys do with procfs files and version
strings is completely wrong and bogus.

Once you create a procfs file layout, you're basically stuck and you
can at best only reasonably add new fields at the end, you can't
really change existing fields.

And sysfs would have been a lot more appropriate, you could use
attributes for each value you want to export and then just add new
sysfs attributes when you want to export new values which has very
clear semantics and backwards compatability implications.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Hartkopp - Nov. 10, 2010, 6:52 a.m.
On 09.11.2010 18:05, David Miller wrote:
> From: Oliver Hartkopp <socketcan@hartkopp.net>
> Date: Tue, 09 Nov 2010 08:52:21 +0100
> 
>> Once this patch is applied (and the procfs layout is changed anyway), i'd also
>> like to send a patch from my backlog that would extend the procfs output for
>> can-bcm with an additional drop counter.
> 
> I find this kind of discussion extremely disappointing.
> 
> All of this stuff you CAN guys do with procfs files and version
> strings is completely wrong and bogus.
> 
> Once you create a procfs file layout, you're basically stuck and you
> can at best only reasonably add new fields at the end, you can't
> really change existing fields.
> 
> And sysfs would have been a lot more appropriate, you could use
> attributes for each value you want to export and then just add new
> sysfs attributes when you want to export new values which has very
> clear semantics and backwards compatability implications.

I admit that from my todays knowledge i would have done things differently.
But the network layer information bits have been always exposed in /proc/net
as it was in 2003 when we started the implementation on a 2.4.x kernel.
There are netdriver infos in sysfs but no netlayer entries.

From my point of view the only thing could be to improve the current
situation, which the posted patch does:

- remove kernel addresses that were only relevant at implementation time
- allow AF_CAN protocols to provide their own information due to their needs
- provide inode numbers that can be found in procfs at several places
  => improvements for developers in userspace & kernelspace

The patch has been discussed on SocketCAN ML and the filter entries have not
been identified as a problem for userspace tools. E.g. /proc/net/can/stats is
one of the entries that's used by userspace tools.

IMHO the patch improves the historic situation and fixes the useless leakage
of kernel addresses. Please consider to apply that procfs changes.

Best regards,
Oliver
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 10, 2010, 5:51 p.m.
From: Oliver Hartkopp <socketcan@hartkopp.net>
Date: Wed, 10 Nov 2010 07:52:27 +0100

> IMHO the patch improves the historic situation and fixes the useless leakage
> of kernel addresses. Please consider to apply that procfs changes.

I'm only fine with fixing the kernel pointer fields in some way.

But moving forward any other change to the procfs file is simply
a waste of time.

You should create sysfs files and add logic to your tools to look
for them and use them if they exist.

Your forward path _SHOULD NOT_ be continuing this procfs versioning
madness.  Use something sane and do the work to make userland start
to be ready for this transition.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Hartkopp - Nov. 10, 2010, 10:10 p.m.
On 10.11.2010 18:51, David Miller wrote:
> From: Oliver Hartkopp <socketcan@hartkopp.net>
> Date: Wed, 10 Nov 2010 07:52:27 +0100
> 
>> IMHO the patch improves the historic situation and fixes the useless leakage
>> of kernel addresses. Please consider to apply that procfs changes.
> 
> I'm only fine with fixing the kernel pointer fields in some way.
> 
> But moving forward any other change to the procfs file is simply
> a waste of time.
> 
> You should create sysfs files and add logic to your tools to look
> for them and use them if they exist.
> 
> Your forward path _SHOULD NOT_ be continuing this procfs versioning
> madness.  Use something sane and do the work to make userland start
> to be ready for this transition.

Hm, summarizing the given restrictions and taking into account that just
setting the pointer fields to '0' is said to be annoying, the only thing that
can be fixed is the minor heap overflow caused by the char array. I'll send a
patch for that.

As you don't want to change the layout even if there's no tool relying on the
entries i wanted to modify, i'll just stop my attempts to improve it.

Regards,
Oliver


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/can/core.h b/include/linux/can/core.h
index 6c507be..e20a841 100644
--- a/include/linux/can/core.h
+++ b/include/linux/can/core.h
@@ -19,7 +19,7 @@ 
 #include <linux/skbuff.h>
 #include <linux/netdevice.h>
 
-#define CAN_VERSION "20090105"
+#define CAN_VERSION "20101105"
 
 /* increment this number each time you change some user-space interface */
 #define CAN_ABI_VERSION "8"
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 08ffe9e..0e81e04 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -86,6 +86,12 @@  MODULE_LICENSE("Dual BSD/GPL");
 MODULE_AUTHOR("Oliver Hartkopp <oliver.hartkopp@volkswagen.de>");
 MODULE_ALIAS("can-proto-2");
 
+/*
+ * Point to the sockets inode number inside the bcm ident string.
+ * We skip the string length of "bcm " (== 4) created in bcm_init().
+ */
+#define INODENUM(bo) (bo->ident + 4)
+
 /* easy access to can_frame payload */
 static inline u64 GET_U64(const struct can_frame *cp)
 {
@@ -125,7 +131,7 @@  struct bcm_sock {
 	struct list_head tx_ops;
 	unsigned long dropped_usr_msgs;
 	struct proc_dir_entry *bcm_proc_read;
-	char procname [9]; /* pointer printed in ASCII with \0 */
+	char ident[32];
 };
 
 static inline struct bcm_sock *bcm_sk(const struct sock *sk)
@@ -165,9 +171,7 @@  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);
+	seq_printf(m, ">>> socket inode %s", INODENUM(bo));
 	seq_printf(m, " / dropped %lu", bo->dropped_usr_msgs);
 	seq_printf(m, " / bound %s", bcm_proc_getifname(ifname, bo->ifindex));
 	seq_printf(m, " <<<\n");
@@ -1168,7 +1172,7 @@  static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 				err = can_rx_register(dev, op->can_id,
 						      REGMASK(op->can_id),
 						      bcm_rx_handler, op,
-						      "bcm");
+						      bo->ident);
 
 				op->rx_reg_dev = dev;
 				dev_put(dev);
@@ -1177,7 +1181,7 @@  static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 		} else
 			err = can_rx_register(NULL, op->can_id,
 					      REGMASK(op->can_id),
-					      bcm_rx_handler, op, "bcm");
+					      bcm_rx_handler, op, bo->ident);
 		if (err) {
 			/* this bcm rx op is broken -> remove it */
 			list_del(&op->list);
@@ -1402,6 +1406,8 @@  static int bcm_init(struct sock *sk)
 {
 	struct bcm_sock *bo = bcm_sk(sk);
 
+	snprintf(bo->ident, sizeof(bo->ident), "bcm %lu", sock_i_ino(sk));
+
 	bo->bound            = 0;
 	bo->ifindex          = 0;
 	bo->dropped_usr_msgs = 0;
@@ -1466,7 +1472,7 @@  static int bcm_release(struct socket *sock)
 
 	/* remove procfs entry */
 	if (proc_dir && bo->bcm_proc_read)
-		remove_proc_entry(bo->procname, proc_dir);
+		remove_proc_entry(INODENUM(bo), proc_dir);
 
 	/* remove device reference */
 	if (bo->bound) {
@@ -1519,13 +1525,11 @@  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);
-		bo->bcm_proc_read = proc_create_data(bo->procname, 0644,
+	/* use unique socket inode number as filename */
+	if (proc_dir)
+		bo->bcm_proc_read = proc_create_data(INODENUM(bo), 0644,
 						     proc_dir,
 						     &bcm_proc_fops, sk);
-	}
 
 	return 0;
 }
diff --git a/net/can/proc.c b/net/can/proc.c
index f4265cc..15bed1c 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -204,23 +204,17 @@  static void can_print_rcvlist(struct seq_file *m, struct hlist_head *rx_list,
 
 	hlist_for_each_entry_rcu(r, n, rx_list, list) {
 		char *fmt = (r->can_id & CAN_EFF_FLAG)?
-			"   %-5s  %08X  %08x  %08x  %08x  %8ld  %s\n" :
-			"   %-5s     %03X    %08x  %08lx  %08lx  %8ld  %s\n";
+			"   %-5s  %08X  %08x  %8ld   %s\n" :
+			"   %-5s     %03X    %08x  %8ld   %s\n";
 
 		seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
-				(unsigned long)r->func, (unsigned long)r->data,
 				r->matches, r->ident);
 	}
 }
 
 static void can_print_recv_banner(struct seq_file *m)
 {
-	/*
-	 *                  can1.  00000000  00000000  00000000
-	 *                 .......          0  tp20
-	 */
-	seq_puts(m, "  device   can_id   can_mask  function"
-			"  userdata   matches  ident\n");
+	seq_puts(m, "  device   can_id   can_mask   matches   ident\n");
 }
 
 static int can_stats_proc_show(struct seq_file *m, void *v)
diff --git a/net/can/raw.c b/net/can/raw.c
index e88f610..e057f0d 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -88,6 +88,7 @@  struct raw_sock {
 	struct can_filter dfilter; /* default/single filter */
 	struct can_filter *filter; /* pointer to filter(s) */
 	can_err_mask_t err_mask;
+	char ident[32];
 };
 
 /*
@@ -154,13 +155,14 @@  static void raw_rcv(struct sk_buff *oskb, void *data)
 static int raw_enable_filters(struct net_device *dev, struct sock *sk,
 			      struct can_filter *filter, int count)
 {
+	struct raw_sock *ro = raw_sk(sk);
 	int err = 0;
 	int i;
 
 	for (i = 0; i < count; i++) {
 		err = can_rx_register(dev, filter[i].can_id,
 				      filter[i].can_mask,
-				      raw_rcv, sk, "raw");
+				      raw_rcv, sk, ro->ident);
 		if (err) {
 			/* clean up successfully registered filters */
 			while (--i >= 0)
@@ -177,11 +179,12 @@  static int raw_enable_filters(struct net_device *dev, struct sock *sk,
 static int raw_enable_errfilter(struct net_device *dev, struct sock *sk,
 				can_err_mask_t err_mask)
 {
+	struct raw_sock *ro = raw_sk(sk);
 	int err = 0;
 
 	if (err_mask)
 		err = can_rx_register(dev, 0, err_mask | CAN_ERR_FLAG,
-				      raw_rcv, sk, "raw");
+				      raw_rcv, sk, ro->ident);
 
 	return err;
 }
@@ -281,6 +284,8 @@  static int raw_init(struct sock *sk)
 {
 	struct raw_sock *ro = raw_sk(sk);
 
+	snprintf(ro->ident, sizeof(ro->ident), "raw %lu", sock_i_ino(sk));
+
 	ro->bound            = 0;
 	ro->ifindex          = 0;