[SECURITY] CAN info leak/minor heap overflow

[SECURITY] CAN info leak/minor heap overflow

[Dan Rosenberg]

In bcm_connect() (in net/can/bcm.c), I noticed the following code:

	sprintf(bo->procname, "%p", sock);

"procname" is a 9-byte char array.  This code is wrong on two levels.
First, leaking a kernel address via a /proc filename is bad.  Secondly,
on 64-bit platforms, up to 17 bytes may be copied into the buffer.
Fortunately, structure padding will most likely prevent this from being
a problem, except for the trailing NULL byte, which may overwrite the
first byte of the next heap object.  Please name your procfile in a way
that doesn't leak information and fits into the desired name buffer.

-Dan

Re: [SECURITY] CAN info leak/minor heap overflow

[Oliver Hartkopp]

Hello Dan,

On 02.11.2010 19:28, Dan Rosenberg wrote:
> In bcm_connect() (in net/can/bcm.c), I noticed the following code:
> 
> 	sprintf(bo->procname, "%p", sock);
> 
> "procname" is a 9-byte char array.  This code is wrong on two levels.
> First, leaking a kernel address via a /proc filename is bad.

Why is this bad? Can the addresses of CAN-BCM sock structs be used for
anything from userspace?

For me they are just intented to be unique numbers ...

> Secondly,
> on 64-bit platforms, up to 17 bytes may be copied into the buffer.

Hm - that's indeed not wanted. Will send a patch at least for this issue.

> Fortunately, structure padding will most likely prevent this from being
> a problem, except for the trailing NULL byte, which may overwrite the
> first byte of the next heap object.  Please name your procfile in a way
> that doesn't leak information and fits into the desired name buffer.
> 
> -Dan
> 

Regards,
Oliver

Re: [SECURITY] CAN info leak/minor heap overflow

[Dan Rosenberg]

> Why is this bad? Can the addresses of CAN-BCM sock structs be used for
> anything from userspace?
> 
> For me they are just intented to be unique numbers ...
> 

This is a bad idea because it makes exploiting other kernel
vulnerabilities easier.  Exposing the address of an object in a slab
cache, especially an object that unprivileged users have some level of
control of, is just an invitation to use that structure when writing
exploits, for heap overflows or otherwise.

-Dan

> > Secondly,
> > on 64-bit platforms, up to 17 bytes may be copied into the buffer.
> 
> Hm - that's indeed not wanted. Will send a patch at least for this issue.
> 
> > Fortunately, structure padding will most likely prevent this from being
> > a problem, except for the trailing NULL byte, which may overwrite the
> > first byte of the next heap object.  Please name your procfile in a way
> > that doesn't leak information and fits into the desired name buffer.
> > 
> > -Dan
> > 
> 
> Regards,
> Oliver

Re: [Security] [SECURITY] CAN info leak/minor heap overflow

[Linus Torvalds]

On Tue, Nov 2, 2010 at 3:53 PM, Dan Rosenberg <drosenberg@vsecurity.com> wrote:
>
>> Why is this bad? Can the addresses of CAN-BCM sock structs be used for
>> anything from userspace?
>>
>> For me they are just intented to be unique numbers ...
>>
>
> This is a bad idea because it makes exploiting other kernel
> vulnerabilities easier.  Exposing the address of an object in a slab
> cache, especially an object that unprivileged users have some level of
> control of, is just an invitation to use that structure when writing
> exploits, for heap overflows or otherwise.

Indeed. At the very least, hash them and truncate them with some
secret per-boot value or something. Even better, use something like a
socket number so that maybe they can be associated with
/proc/<xyz>/fd/<x> or other system info if somebody were to care.

             Linus

Re: [Security] [SECURITY] CAN info leak/minor heap overflow

[Oliver Hartkopp]

On 02.11.2010 20:57, Linus Torvalds wrote:
> On Tue, Nov 2, 2010 at 3:53 PM, Dan Rosenberg <drosenberg@vsecurity.com> wrote:
>>
>>> Why is this bad? Can the addresses of CAN-BCM sock structs be used for
>>> anything from userspace?
>>>
>>> For me they are just intented to be unique numbers ...
>>>
>>
>> This is a bad idea because it makes exploiting other kernel
>> vulnerabilities easier.  Exposing the address of an object in a slab
>> cache, especially an object that unprivileged users have some level of
>> control of, is just an invitation to use that structure when writing
>> exploits, for heap overflows or otherwise.
> 
> Indeed. At the very least, hash them and truncate them with some
> secret per-boot value or something. Even better, use something like a
> socket number so that maybe they can be associated with
> /proc/<xyz>/fd/<x> or other system info if somebody were to care.

Good hint!

Will pick this idea.

Thanks,
Oliver

Re: [SECURITY] CAN info leak/minor heap overflow

[Oliver Hartkopp]

On 02.11.2010 20:53, Dan Rosenberg wrote:
> 
>> Why is this bad? Can the addresses of CAN-BCM sock structs be used for
>> anything from userspace?
>>
>> For me they are just intended to be unique numbers ...
>>
> 
> This is a bad idea because it makes exploiting other kernel
> vulnerabilities easier.  Exposing the address of an object in a slab
> cache, especially an object that unprivileged users have some level of
> control of, is just an invitation to use that structure when writing
> exploits, for heap overflows or otherwise.

The "level of control of" is just creating a socket or not. None of the data
in the created struct can be influenced by an unprivileged user.
Btw. i can generally follow your concerns after this explanation.

I'm going to check the kernel src for other approaches to display unique
numbers in procfs and will send a patch that takes care.

Thanks,
Oliver

[PATCH] Fix CAN info leak/minor heap overflow

[Urs Thuermann]

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>

---

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;
 

Re: [PATCH] Fix CAN info leak/minor heap overflow

[Oliver Hartkopp]

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;
>  

Re: [PATCH] Fix CAN info leak/minor heap overflow

[David Miller]

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.

Re: [PATCH] Fix CAN info leak/minor heap overflow

[Oliver Hartkopp]

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

Re: [PATCH] Fix CAN info leak/minor heap overflow

[David Miller]

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.

Re: [PATCH] Fix CAN info leak/minor heap overflow

[Oliver Hartkopp]

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