[patch 1/1] net: convert %p usage to %pK

[patch 1/1] net: convert %p usage to %pK

[akpm]

From: Dan Rosenberg <drosenberg@vsecurity.com>

The %pK format specifier is designed to hide exposed kernel pointers,
specifically via /proc interfaces.  Exposing these pointers provides an
easy target for kernel write vulnerabilities, since they reveal the
locations of writable structures containing easily triggerable function
pointers.  The behavior of %pK depends on the kptr_restrict sysctl.

If kptr_restrict is set to 0, no deviation from the standard %p behavior
occurs.  If kptr_restrict is set to 1, the default, if the current user
(intended to be a reader via seq_printf(), etc.) does not have CAP_SYSLOG
(currently in the LSM tree), kernel pointers using %pK are printed as 0's.
 If kptr_restrict is set to 2, kernel pointers using %pK are printed as
0's regardless of privileges.  Replacing with 0's was chosen over the
default "(null)", which cannot be parsed by userland %p, which expects
"(nil)".

The supporting code for kptr_restrict and %pK are currently in the -mm
tree.  This patch converts users of %p in net/ to %pK.  Cases of printing
pointers to the syslog are not covered, since this would eliminate useful
information for postmortem debugging and the reading of the syslog is
already optionally protected by the dmesg_restrict sysctl.

Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
Cc: James Morris <jmorris@namei.org>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Thomas Graf <tgraf@infradead.org>
Cc: Eugene Teo <eugeneteo@kernel.org>
Cc: Kees Cook <kees.cook@canonical.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: David S. Miller <davem@davemloft.net>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Eric Paris <eparis@parisplace.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 net/atm/proc.c           |    4 ++--
 net/can/bcm.c            |    6 +++---
 net/ipv4/raw.c           |    2 +-
 net/ipv4/tcp_ipv4.c      |    6 +++---
 net/ipv4/udp.c           |    2 +-
 net/ipv6/raw.c           |    2 +-
 net/ipv6/tcp_ipv6.c      |    6 +++---
 net/ipv6/udp.c           |    2 +-
 net/key/af_key.c         |    2 +-
 net/netlink/af_netlink.c |    2 +-
 net/packet/af_packet.c   |    2 +-
 net/phonet/socket.c      |    2 +-
 net/sctp/proc.c          |    4 ++--
 net/unix/af_unix.c       |    2 +-
 14 files changed, 22 insertions(+), 22 deletions(-)

diff -puN net/atm/proc.c~net-convert-%p-usage-to-%pk net/atm/proc.c
--- a/net/atm/proc.c~net-convert-%p-usage-to-%pk
+++ a/net/atm/proc.c
@@ -191,7 +191,7 @@ static void vcc_info(struct seq_file *se
 {
 	struct sock *sk = sk_atm(vcc);
 
-	seq_printf(seq, "%p ", vcc);
+	seq_printf(seq, "%pK ", vcc);
 	if (!vcc->dev)
 		seq_printf(seq, "Unassigned    ");
 	else
@@ -218,7 +218,7 @@ static void svc_info(struct seq_file *se
 {
 	if (!vcc->dev)
 		seq_printf(seq, sizeof(void *) == 4 ?
-			   "N/A@%p%10s" : "N/A@%p%2s", vcc, "");
+			   "N/A@%pK%10s" : "N/A@%pK%2s", vcc, "");
 	else
 		seq_printf(seq, "%3d %3d %5d         ",
 			   vcc->dev->number, vcc->vpi, vcc->vci);
diff -puN net/can/bcm.c~net-convert-%p-usage-to-%pk net/can/bcm.c
--- a/net/can/bcm.c~net-convert-%p-usage-to-%pk
+++ a/net/can/bcm.c
@@ -165,9 +165,9 @@ static int bcm_proc_show(struct seq_file
 	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 %pK", sk->sk_socket);
+	seq_printf(m, " / sk %pK", sk);
+	seq_printf(m, " / bo %pK", 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");
diff -puN net/ipv4/raw.c~net-convert-%p-usage-to-%pk net/ipv4/raw.c
--- a/net/ipv4/raw.c~net-convert-%p-usage-to-%pk
+++ a/net/ipv4/raw.c
@@ -979,7 +979,7 @@ static void raw_sock_seq_show(struct seq
 	      srcp  = inet->inet_num;
 
 	seq_printf(seq, "%4d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5d %8d %lu %d %p %d\n",
+		" %02X %08X:%08X %02X:%08lX %08X %5d %8d %lu %d %pK %d\n",
 		i, src, srcp, dest, destp, sp->sk_state,
 		sk_wmem_alloc_get(sp),
 		sk_rmem_alloc_get(sp),
diff -puN net/ipv4/tcp_ipv4.c~net-convert-%p-usage-to-%pk net/ipv4/tcp_ipv4.c
--- a/net/ipv4/tcp_ipv4.c~net-convert-%p-usage-to-%pk
+++ a/net/ipv4/tcp_ipv4.c
@@ -2371,7 +2371,7 @@ static void get_openreq4(struct sock *sk
 	int ttd = req->expires - jiffies;
 
 	seq_printf(f, "%4d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5d %8d %u %d %p%n",
+		" %02X %08X:%08X %02X:%08lX %08X %5d %8d %u %d %pK%n",
 		i,
 		ireq->loc_addr,
 		ntohs(inet_sk(sk)->inet_sport),
@@ -2426,7 +2426,7 @@ static void get_tcp4_sock(struct sock *s
 		rx_queue = max_t(int, tp->rcv_nxt - tp->copied_seq, 0);
 
 	seq_printf(f, "%4d: %08X:%04X %08X:%04X %02X %08X:%08X %02X:%08lX "
-			"%08X %5d %8d %lu %d %p %lu %lu %u %u %d%n",
+			"%08X %5d %8d %lu %d %pK %lu %lu %u %u %d%n",
 		i, src, srcp, dest, destp, sk->sk_state,
 		tp->write_seq - tp->snd_una,
 		rx_queue,
@@ -2461,7 +2461,7 @@ static void get_timewait4_sock(struct in
 	srcp  = ntohs(tw->tw_sport);
 
 	seq_printf(f, "%4d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %p%n",
+		" %02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %pK%n",
 		i, src, srcp, dest, destp, tw->tw_substate, 0, 0,
 		3, jiffies_to_clock_t(ttd), 0, 0, 0, 0,
 		atomic_read(&tw->tw_refcnt), tw, len);
diff -puN net/ipv4/udp.c~net-convert-%p-usage-to-%pk net/ipv4/udp.c
--- a/net/ipv4/udp.c~net-convert-%p-usage-to-%pk
+++ a/net/ipv4/udp.c
@@ -2090,7 +2090,7 @@ static void udp4_format_sock(struct sock
 	__u16 srcp	  = ntohs(inet->inet_sport);
 
 	seq_printf(f, "%5d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5d %8d %lu %d %p %d%n",
+		" %02X %08X:%08X %02X:%08lX %08X %5d %8d %lu %d %pK %d%n",
 		bucket, src, srcp, dest, destp, sp->sk_state,
 		sk_wmem_alloc_get(sp),
 		sk_rmem_alloc_get(sp),
diff -puN net/ipv6/raw.c~net-convert-%p-usage-to-%pk net/ipv6/raw.c
--- a/net/ipv6/raw.c~net-convert-%p-usage-to-%pk
+++ a/net/ipv6/raw.c
@@ -1240,7 +1240,7 @@ static void raw6_sock_seq_show(struct se
 	srcp  = inet_sk(sp)->inet_num;
 	seq_printf(seq,
 		   "%4d: %08X%08X%08X%08X:%04X %08X%08X%08X%08X:%04X "
-		   "%02X %08X:%08X %02X:%08lX %08X %5d %8d %lu %d %p %d\n",
+		   "%02X %08X:%08X %02X:%08lX %08X %5d %8d %lu %d %pK %d\n",
 		   i,
 		   src->s6_addr32[0], src->s6_addr32[1],
 		   src->s6_addr32[2], src->s6_addr32[3], srcp,
diff -puN net/ipv6/tcp_ipv6.c~net-convert-%p-usage-to-%pk net/ipv6/tcp_ipv6.c
--- a/net/ipv6/tcp_ipv6.c~net-convert-%p-usage-to-%pk
+++ a/net/ipv6/tcp_ipv6.c
@@ -2036,7 +2036,7 @@ static void get_openreq6(struct seq_file
 
 	seq_printf(seq,
 		   "%4d: %08X%08X%08X%08X:%04X %08X%08X%08X%08X:%04X "
-		   "%02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %p\n",
+		   "%02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %pK\n",
 		   i,
 		   src->s6_addr32[0], src->s6_addr32[1],
 		   src->s6_addr32[2], src->s6_addr32[3],
@@ -2087,7 +2087,7 @@ static void get_tcp6_sock(struct seq_fil
 
 	seq_printf(seq,
 		   "%4d: %08X%08X%08X%08X:%04X %08X%08X%08X%08X:%04X "
-		   "%02X %08X:%08X %02X:%08lX %08X %5d %8d %lu %d %p %lu %lu %u %u %d\n",
+		   "%02X %08X:%08X %02X:%08lX %08X %5d %8d %lu %d %pK %lu %lu %u %u %d\n",
 		   i,
 		   src->s6_addr32[0], src->s6_addr32[1],
 		   src->s6_addr32[2], src->s6_addr32[3], srcp,
@@ -2129,7 +2129,7 @@ static void get_timewait6_sock(struct se
 
 	seq_printf(seq,
 		   "%4d: %08X%08X%08X%08X:%04X %08X%08X%08X%08X:%04X "
-		   "%02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %p\n",
+		   "%02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %pK\n",
 		   i,
 		   src->s6_addr32[0], src->s6_addr32[1],
 		   src->s6_addr32[2], src->s6_addr32[3], srcp,
diff -puN net/ipv6/udp.c~net-convert-%p-usage-to-%pk net/ipv6/udp.c
--- a/net/ipv6/udp.c~net-convert-%p-usage-to-%pk
+++ a/net/ipv6/udp.c
@@ -1391,7 +1391,7 @@ static void udp6_sock_seq_show(struct se
 	srcp  = ntohs(inet->inet_sport);
 	seq_printf(seq,
 		   "%5d: %08X%08X%08X%08X:%04X %08X%08X%08X%08X:%04X "
-		   "%02X %08X:%08X %02X:%08lX %08X %5d %8d %lu %d %p %d\n",
+		   "%02X %08X:%08X %02X:%08lX %08X %5d %8d %lu %d %pK %d\n",
 		   bucket,
 		   src->s6_addr32[0], src->s6_addr32[1],
 		   src->s6_addr32[2], src->s6_addr32[3], srcp,
diff -puN net/key/af_key.c~net-convert-%p-usage-to-%pk net/key/af_key.c
--- a/net/key/af_key.c~net-convert-%p-usage-to-%pk
+++ a/net/key/af_key.c
@@ -3656,7 +3656,7 @@ static int pfkey_seq_show(struct seq_fil
 	if (v == SEQ_START_TOKEN)
 		seq_printf(f ,"sk       RefCnt Rmem   Wmem   User   Inode\n");
 	else
-		seq_printf(f ,"%p %-6d %-6u %-6u %-6u %-6lu\n",
+		seq_printf(f, "%pK %-6d %-6u %-6u %-6u %-6lu\n",
 			       s,
 			       atomic_read(&s->sk_refcnt),
 			       sk_rmem_alloc_get(s),
diff -puN net/netlink/af_netlink.c~net-convert-%p-usage-to-%pk net/netlink/af_netlink.c
--- a/net/netlink/af_netlink.c~net-convert-%p-usage-to-%pk
+++ a/net/netlink/af_netlink.c
@@ -1985,7 +1985,7 @@ static int netlink_seq_show(struct seq_f
 		struct sock *s = v;
 		struct netlink_sock *nlk = nlk_sk(s);
 
-		seq_printf(seq, "%p %-3d %-6d %08x %-8d %-8d %p %-8d %-8d %-8lu\n",
+		seq_printf(seq, "%pK %-3d %-6d %08x %-8d %-8d %pK %-8d %-8d %-8lu\n",
 			   s,
 			   s->sk_protocol,
 			   nlk->pid,
diff -puN net/packet/af_packet.c~net-convert-%p-usage-to-%pk net/packet/af_packet.c
--- a/net/packet/af_packet.c~net-convert-%p-usage-to-%pk
+++ a/net/packet/af_packet.c
@@ -2706,7 +2706,7 @@ static int packet_seq_show(struct seq_fi
 		const struct packet_sock *po = pkt_sk(s);
 
 		seq_printf(seq,
-			   "%p %-6d %-4d %04x   %-5d %1d %-6u %-6u %-6lu\n",
+			   "%pK %-6d %-4d %04x   %-5d %1d %-6u %-6u %-6lu\n",
 			   s,
 			   atomic_read(&s->sk_refcnt),
 			   s->sk_type,
diff -puN net/phonet/socket.c~net-convert-%p-usage-to-%pk net/phonet/socket.c
--- a/net/phonet/socket.c~net-convert-%p-usage-to-%pk
+++ a/net/phonet/socket.c
@@ -607,7 +607,7 @@ static int pn_sock_seq_show(struct seq_f
 		struct pn_sock *pn = pn_sk(sk);
 
 		seq_printf(seq, "%2d %04X:%04X:%02X %02X %08X:%08X %5d %lu "
-			"%d %p %d%n",
+			"%d %pK %d%n",
 			sk->sk_protocol, pn->sobject, pn->dobject,
 			pn->resource, sk->sk_state,
 			sk_wmem_alloc_get(sk), sk_rmem_alloc_get(sk),
diff -puN net/sctp/proc.c~net-convert-%p-usage-to-%pk net/sctp/proc.c
--- a/net/sctp/proc.c~net-convert-%p-usage-to-%pk
+++ a/net/sctp/proc.c
@@ -212,7 +212,7 @@ static int sctp_eps_seq_show(struct seq_
 	sctp_for_each_hentry(epb, node, &head->chain) {
 		ep = sctp_ep(epb);
 		sk = epb->sk;
-		seq_printf(seq, "%8p %8p %-3d %-3d %-4d %-5d %5d %5lu ", ep, sk,
+		seq_printf(seq, "%8pK %8pK %-3d %-3d %-4d %-5d %5d %5lu ", ep, sk,
 			   sctp_sk(sk)->type, sk->sk_state, hash,
 			   epb->bind_addr.port,
 			   sock_i_uid(sk), sock_i_ino(sk));
@@ -316,7 +316,7 @@ static int sctp_assocs_seq_show(struct s
 		assoc = sctp_assoc(epb);
 		sk = epb->sk;
 		seq_printf(seq,
-			   "%8p %8p %-3d %-3d %-2d %-4d "
+			   "%8pK %8pK %-3d %-3d %-2d %-4d "
 			   "%4d %8d %8d %7d %5lu %-5d %5d ",
 			   assoc, sk, sctp_sk(sk)->type, sk->sk_state,
 			   assoc->state, hash,
diff -puN net/unix/af_unix.c~net-convert-%p-usage-to-%pk net/unix/af_unix.c
--- a/net/unix/af_unix.c~net-convert-%p-usage-to-%pk
+++ a/net/unix/af_unix.c
@@ -2254,7 +2254,7 @@ static int unix_seq_show(struct seq_file
 		struct unix_sock *u = unix_sk(s);
 		unix_state_lock(s);
 
-		seq_printf(seq, "%p: %08X %08X %08X %04X %02X %5lu",
+		seq_printf(seq, "%pK: %08X %08X %08X %04X %02X %5lu",
 			s,
 			atomic_read(&s->sk_refcnt),
 			0,
_

Re: [patch 1/1] net: convert %p usage to %pK

[David Miller]

From: akpm@linux-foundation.org
Date: Mon, 23 May 2011 15:17:35 -0700

> From: Dan Rosenberg <drosenberg@vsecurity.com>
> 
> The %pK format specifier is designed to hide exposed kernel pointers,
> specifically via /proc interfaces.  Exposing these pointers provides an
> easy target for kernel write vulnerabilities, since they reveal the
> locations of writable structures containing easily triggerable function
> pointers.  The behavior of %pK depends on the kptr_restrict sysctl.
> 
> If kptr_restrict is set to 0, no deviation from the standard %p behavior
> occurs.  If kptr_restrict is set to 1, the default, if the current user
> (intended to be a reader via seq_printf(), etc.) does not have CAP_SYSLOG
> (currently in the LSM tree), kernel pointers using %pK are printed as 0's.
>  If kptr_restrict is set to 2, kernel pointers using %pK are printed as
> 0's regardless of privileges.  Replacing with 0's was chosen over the
> default "(null)", which cannot be parsed by userland %p, which expects
> "(nil)".
> 
> The supporting code for kptr_restrict and %pK are currently in the -mm
> tree.  This patch converts users of %p in net/ to %pK.  Cases of printing
> pointers to the syslog are not covered, since this would eliminate useful
> information for postmortem debugging and the reading of the syslog is
> already optionally protected by the dmesg_restrict sysctl.
> 
> Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Applied.

Re: [patch 1/1] net: convert %p usage to %pK

[Eric Dumazet]

Le mardi 24 mai 2011 à 01:13 -0400, David Miller a écrit :
> From: akpm@linux-foundation.org
> Date: Mon, 23 May 2011 15:17:35 -0700
> 
> > From: Dan Rosenberg <drosenberg@vsecurity.com>
> > 
> > The %pK format specifier is designed to hide exposed kernel pointers,
> > specifically via /proc interfaces.  Exposing these pointers provides an
> > easy target for kernel write vulnerabilities, since they reveal the
> > locations of writable structures containing easily triggerable function
> > pointers.  The behavior of %pK depends on the kptr_restrict sysctl.
> > 
> > If kptr_restrict is set to 0, no deviation from the standard %p behavior
> > occurs.  If kptr_restrict is set to 1, the default, if the current user
> > (intended to be a reader via seq_printf(), etc.) does not have CAP_SYSLOG
> > (currently in the LSM tree), kernel pointers using %pK are printed as 0's.
> >  If kptr_restrict is set to 2, kernel pointers using %pK are printed as
> > 0's regardless of privileges.  Replacing with 0's was chosen over the
> > default "(null)", which cannot be parsed by userland %p, which expects
> > "(nil)".
> > 
> > The supporting code for kptr_restrict and %pK are currently in the -mm
> > tree.  This patch converts users of %p in net/ to %pK.  Cases of printing
> > pointers to the syslog are not covered, since this would eliminate useful
> > information for postmortem debugging and the reading of the syslog is
> > already optionally protected by the dmesg_restrict sysctl.
> > 
> > Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> 
> Applied.

We probably need to extend this to inet_diag as well.

"ss -e" currently expose kernel pointers, like /proc files used to do
before this patch.

Thanks

[PATCH] inet_diag: hide socket pointers

Provide a mayber_hide_ptr() helper and use it in inet_diag to not
disclose kernel pointers to user, with kptr_restrict logic :

kptr_restrict = 0 : kernel pointers are not mangled
kptr_restrict = 1 : if the current user does not have CAP_SYSLOG,
kernel pointers are replaced by 0
kptr_restrict = 2 : kernel pointers are replaced by 0

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 include/linux/printk.h |    1 +
 lib/vsprintf.c         |   15 +++++++++++----
 net/ipv4/inet_diag.c   |    6 ++++--
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index ee048e7..47c0cef 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -111,6 +111,7 @@ extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 extern int printk_delay_msec;
 extern int dmesg_restrict;
 extern int kptr_restrict;
+void *maybe_hide_ptr(void *ptr);
 
 void log_buf_kexec_setup(void);
 #else
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 1d659d7..20d3576 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -799,6 +799,16 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 
 int kptr_restrict __read_mostly;
 
+void *maybe_hide_ptr(void *ptr)
+{
+	if (!((kptr_restrict == 0) ||
+	      (kptr_restrict == 1 &&
+	       has_capability_noaudit(current, CAP_SYSLOG))))
+		ptr = NULL;
+	return ptr;
+}
+EXPORT_SYMBOL(maybe_hide_ptr);
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -911,10 +921,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 				spec.field_width = 2 * sizeof(void *);
 			return string(buf, end, "pK-error", spec);
 		}
-		if (!((kptr_restrict == 0) ||
-		      (kptr_restrict == 1 &&
-		       has_capability_noaudit(current, CAP_SYSLOG))))
-			ptr = NULL;
+		ptr = maybe_hide_ptr(ptr);
 		break;
 	}
 	spec.flags |= SMALL;
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 6ffe94c..b5646a3 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -84,6 +84,7 @@ static int inet_csk_diag_fill(struct sock *sk,
 	struct inet_diag_meminfo  *minfo = NULL;
 	unsigned char	 *b = skb_tail_pointer(skb);
 	const struct inet_diag_handler *handler;
+	u64 ptr;
 
 	handler = inet_diag_table[unlh->nlmsg_type];
 	BUG_ON(handler == NULL);
@@ -114,8 +115,9 @@ static int inet_csk_diag_fill(struct sock *sk,
 	r->idiag_retrans = 0;
 
 	r->id.idiag_if = sk->sk_bound_dev_if;
-	r->id.idiag_cookie[0] = (u32)(unsigned long)sk;
-	r->id.idiag_cookie[1] = (u32)(((unsigned long)sk >> 31) >> 1);
+	ptr = (u64)maybe_hide_ptr(sk);
+	r->id.idiag_cookie[0] = (u32)ptr;
+	r->id.idiag_cookie[1] = (u32)(ptr >> 32);
 
 	r->id.idiag_sport = inet->inet_sport;
 	r->id.idiag_dport = inet->inet_dport;

Re: [patch 1/1] net: convert %p usage to %pK

[Joe Perches]

On Tue, 2011-05-24 at 08:17 +0200, Eric Dumazet wrote:
> We probably need to extend this to inet_diag as well.
> Provide a mayber_hide_ptr() helper and use it in inet_diag to not
> disclose kernel pointers to user, with kptr_restrict logic :
> kptr_restrict = 0 : kernel pointers are not mangled
> kptr_restrict = 1 : if the current user does not have CAP_SYSLOG,
> kernel pointers are replaced by 0
> kptr_restrict = 2 : kernel pointers are replaced by 0
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> +void *maybe_hide_ptr(void *ptr)
> +{
> +	if (!((kptr_restrict == 0) ||
> +	      (kptr_restrict == 1 &&
> +	       has_capability_noaudit(current, CAP_SYSLOG))))
> +		ptr = NULL;
> +	return ptr;
> +}
> +EXPORT_SYMBOL(maybe_hide_ptr);

Makes sense to me.

Maybe for clarity it'd be better to use a switch/case
or something like:

	if (kptr_restrict == 0)
		return ptr;
	if (ptr_restrict == 1 &&
	    has_capability_noaudit(current, CAP_SYSLOG))
		return ptr;
	return NULL;

Re: [patch 1/1] net: convert %p usage to %pK

[Ingo Molnar]

* Joe Perches <joe@perches.com> wrote:

> Maybe for clarity it'd be better to use a switch/case
> or something like:
> 
> 	if (kptr_restrict == 0)
> 		return ptr;
> 	if (ptr_restrict == 1 &&
> 	    has_capability_noaudit(current, CAP_SYSLOG))
> 		return ptr;
> 	return NULL;

While not breaking that line really - so something like:

	if (kptr_restrict == 0)
		return ptr;

	if (kptr_restrict == 1 && has_capability_noaudit(current, CAP_SYSLOG))
		return ptr;

	return NULL;

Thanks,

	Ingo

Re: [patch 1/1] net: convert %p usage to %pK

[Eric Dumazet]

Le mardi 24 mai 2011 à 08:57 +0200, Ingo Molnar a écrit :
> * Joe Perches <joe@perches.com> wrote:
> 
> > Maybe for clarity it'd be better to use a switch/case
> > or something like:
> > 
> > 	if (kptr_restrict == 0)
> > 		return ptr;
> > 	if (ptr_restrict == 1 &&
> > 	    has_capability_noaudit(current, CAP_SYSLOG))
> > 		return ptr;
> > 	return NULL;
> 
> While not breaking that line really - so something like:
> 
> 	if (kptr_restrict == 0)
> 		return ptr;
> 
> 	if (kptr_restrict == 1 && has_capability_noaudit(current, CAP_SYSLOG))
> 		return ptr;
> 
> 	return NULL;
> 
> Thanks,
> 
> 	Ingo

Here we are, thanks.

[PATCH v2] inet_diag: hide socket pointers

Provide a mayber_hide_ptr() helper and use it in inet_diag to not
disclose kernel pointers to user, with following kptr_restrict logic :

kptr_restrict = 0 : kernel pointers are not mangled
kptr_restrict = 1 : if the current user does not have CAP_SYSLOG,
		    kernel pointers are replaced by 0
kptr_restrict = 2 : kernel pointers are replaced by 0

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
v2: kerneldoc and cleanup (Joe & Ingo feedback)

 include/linux/printk.h |    1 +
 lib/vsprintf.c         |   25 +++++++++++++++++++++----
 net/ipv4/inet_diag.c   |    6 ++++--
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index ee048e7..47c0cef 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -111,6 +111,7 @@ extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 extern int printk_delay_msec;
 extern int dmesg_restrict;
 extern int kptr_restrict;
+void *maybe_hide_ptr(void *ptr);
 
 void log_buf_kexec_setup(void);
 #else
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 1d659d7..db1a193 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -798,6 +798,26 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 }
 
 int kptr_restrict __read_mostly;
+/**
+ * maybe_hide_ptr - Eventually nullify a kernel pointer given to user
+ * @ptr: kernel pointer
+ *
+ * kptr_restrict = 0 : kernel pointer not changed
+ * kptr_restrict = 1 : if the current user does not have CAP_SYSLOG,
+ *		       kernel pointer is replaced by 0
+ * kptr_restrict = 2 : kernel pointer is replaced by 0 for all users.
+ */
+void *maybe_hide_ptr(void *ptr)
+{
+	if (!kptr_restrict)
+		return ptr;
+
+	if (kptr_restrict == 1 && has_capability_noaudit(current, CAP_SYSLOG))
+		return ptr;
+
+	return NULL;
+}
+EXPORT_SYMBOL(maybe_hide_ptr);
 
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
@@ -911,10 +931,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 				spec.field_width = 2 * sizeof(void *);
 			return string(buf, end, "pK-error", spec);
 		}
-		if (!((kptr_restrict == 0) ||
-		      (kptr_restrict == 1 &&
-		       has_capability_noaudit(current, CAP_SYSLOG))))
-			ptr = NULL;
+		ptr = maybe_hide_ptr(ptr);
 		break;
 	}
 	spec.flags |= SMALL;
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 6ffe94c..b5646a3 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -84,6 +84,7 @@ static int inet_csk_diag_fill(struct sock *sk,
 	struct inet_diag_meminfo  *minfo = NULL;
 	unsigned char	 *b = skb_tail_pointer(skb);
 	const struct inet_diag_handler *handler;
+	u64 ptr;
 
 	handler = inet_diag_table[unlh->nlmsg_type];
 	BUG_ON(handler == NULL);
@@ -114,8 +115,9 @@ static int inet_csk_diag_fill(struct sock *sk,
 	r->idiag_retrans = 0;
 
 	r->id.idiag_if = sk->sk_bound_dev_if;
-	r->id.idiag_cookie[0] = (u32)(unsigned long)sk;
-	r->id.idiag_cookie[1] = (u32)(((unsigned long)sk >> 31) >> 1);
+	ptr = (u64)maybe_hide_ptr(sk);
+	r->id.idiag_cookie[0] = (u32)ptr;
+	r->id.idiag_cookie[1] = (u32)(ptr >> 32);
 
 	r->id.idiag_sport = inet->inet_sport;
 	r->id.idiag_dport = inet->inet_dport;

Re: [patch 1/1] net: convert %p usage to %pK

[Joe Perches]

On Tue, 2011-05-24 at 09:04 +0200, Eric Dumazet wrote:
> Le mardi 24 mai 2011 à 08:57 +0200, Ingo Molnar a écrit :
> > * Joe Perches <joe@perches.com> wrote:
> > > Maybe for clarity it'd be better to use a switch/case
> > > or something like:
> Here we are, thanks.

Hey Eric.  Just more trivia:

> [PATCH v2] inet_diag: hide socket pointers
> Provide a mayber_hide_ptr() helper and use it in inet_diag to not

typo maybe_hide_ptr

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> @@ -798,6 +798,26 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
>  }
>  
>  int kptr_restrict __read_mostly;
> +/**
> + * maybe_hide_ptr - Eventually nullify a kernel pointer given to user

Not a great description.
Maybe something like:
 * maybe_hide_ptr - Set user values of kernel pointers to null
 *                  when appropriate
[]
> diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
> index 6ffe94c..b5646a3 100644
> --- a/net/ipv4/inet_diag.c
> +++ b/net/ipv4/inet_diag.c
> @@ -84,6 +84,7 @@ static int inet_csk_diag_fill(struct sock *sk,
>  	struct inet_diag_meminfo  *minfo = NULL;
>  	unsigned char	 *b = skb_tail_pointer(skb);
>  	const struct inet_diag_handler *handler;
> +	u64 ptr;
>  
>  	handler = inet_diag_table[unlh->nlmsg_type];
>  	BUG_ON(handler == NULL);
> @@ -114,8 +115,9 @@ static int inet_csk_diag_fill(struct sock *sk,
>  	r->idiag_retrans = 0;
>  
>  	r->id.idiag_if = sk->sk_bound_dev_if;
> -	r->id.idiag_cookie[0] = (u32)(unsigned long)sk;
> -	r->id.idiag_cookie[1] = (u32)(((unsigned long)sk >> 31) >> 1);
> +	ptr = (u64)maybe_hide_ptr(sk);
> +	r->id.idiag_cookie[0] = (u32)ptr;
> +	r->id.idiag_cookie[1] = (u32)(ptr >> 32);

I think it's be better without the casts
using the standard kernel.h macros.

	void *ptr;

	ptr = maybe_hide_ptr(sk);
	r->id.idiag_cookie[0] = lower_32_bits(ptr);
	r->id.idiag_cookie[1] = upper_32_bits(ptr);

Re: [patch 1/1] net: convert %p usage to %pK

[Eric Dumazet]

Le mardi 24 mai 2011 à 00:35 -0700, Joe Perches a écrit :

> I think it's be better without the casts
> using the standard kernel.h macros.
> 
> 	void *ptr;
> 
> 	ptr = maybe_hide_ptr(sk);
> 	r->id.idiag_cookie[0] = lower_32_bits(ptr);
> 	r->id.idiag_cookie[1] = upper_32_bits(ptr);
> 

I am not sure I want to patch lower_32_bits() and upper_32_bits() for
this.

They dont work on pointers, but on "numbers", according to kerneldoc
Andrew wrote years ago. gcc agrees :

net/ipv4/inet_diag.c: In function ‘inet_csk_diag_fill’:
net/ipv4/inet_diag.c:119: warning: cast from pointer to integer of different size
net/ipv4/inet_diag.c:120: error: invalid operands to binary >>
make[1]: *** [net/ipv4/inet_diag.o] Error 1

Re: [patch 1/1] net: convert %p usage to %pK

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 24 May 2011 09:45:01 +0200

> Le mardi 24 mai 2011 à 00:35 -0700, Joe Perches a écrit :
> 
>> I think it's be better without the casts
>> using the standard kernel.h macros.
>> 
>> 	void *ptr;
>> 
>> 	ptr = maybe_hide_ptr(sk);
>> 	r->id.idiag_cookie[0] = lower_32_bits(ptr);
>> 	r->id.idiag_cookie[1] = upper_32_bits(ptr);
>> 
> 
> I am not sure I want to patch lower_32_bits() and upper_32_bits() for
> this.
> 
> They dont work on pointers, but on "numbers", according to kerneldoc
> Andrew wrote years ago. gcc agrees :
> 
> net/ipv4/inet_diag.c: In function ‘inet_csk_diag_fill’:
> net/ipv4/inet_diag.c:119: warning: cast from pointer to integer of different size
> net/ipv4/inet_diag.c:120: error: invalid operands to binary >>
> make[1]: *** [net/ipv4/inet_diag.o] Error 1

Also you can't do this, the "cookie" is used by the kernel future
lookups to find sockets.

The kernel pointer is part of the API, so sorry you can't "hide"
kernel pointers in this case without really breaking user visible
things.

Re: [patch 1/1] net: convert %p usage to %pK

[Stephen Hemminger]

On Tue, 24 May 2011 03:58:01 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 24 May 2011 09:45:01 +0200
> 
> > Le mardi 24 mai 2011 à 00:35 -0700, Joe Perches a écrit :
> > 
> >> I think it's be better without the casts
> >> using the standard kernel.h macros.
> >> 
> >> 	void *ptr;
> >> 
> >> 	ptr = maybe_hide_ptr(sk);
> >> 	r->id.idiag_cookie[0] = lower_32_bits(ptr);
> >> 	r->id.idiag_cookie[1] = upper_32_bits(ptr);
> >> 
> > 
> > I am not sure I want to patch lower_32_bits() and upper_32_bits() for
> > this.
> > 
> > They dont work on pointers, but on "numbers", according to kerneldoc
> > Andrew wrote years ago. gcc agrees :
> > 
> > net/ipv4/inet_diag.c: In function ‘inet_csk_diag_fill’:
> > net/ipv4/inet_diag.c:119: warning: cast from pointer to integer of different size
> > net/ipv4/inet_diag.c:120: error: invalid operands to binary >>
> > make[1]: *** [net/ipv4/inet_diag.o] Error 1
> 
> Also you can't do this, the "cookie" is used by the kernel future
> lookups to find sockets.
> 
> The kernel pointer is part of the API, so sorry you can't "hide"
> kernel pointers in this case without really breaking user visible
> things.
> [Error decoding BASE64]

Couldn't the pointer be replaced by a socket index?

-- 

Re: [patch 1/1] net: convert %p usage to %pK

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Tue, 24 May 2011 07:43:55 -0700

> Couldn't the pointer be replaced by a socket index?

Well, it's a pointer exactly so we don't have to index into a table.

Re: [patch 1/1] net: convert %p usage to %pK

[Kees Cook]

Hi David,

On Tue, May 24, 2011 at 03:58:01AM -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 24 May 2011 09:45:01 +0200
> 
> > Le mardi 24 mai 2011 à 00:35 -0700, Joe Perches a écrit :
> > 
> >> I think it's be better without the casts
> >> using the standard kernel.h macros.
> >> 
> >> 	void *ptr;
> >> 
> >> 	ptr = maybe_hide_ptr(sk);
> >> 	r->id.idiag_cookie[0] = lower_32_bits(ptr);
> >> 	r->id.idiag_cookie[1] = upper_32_bits(ptr);
> >> 
> > 
> > I am not sure I want to patch lower_32_bits() and upper_32_bits() for
> > this.
> > 
> > They dont work on pointers, but on "numbers", according to kerneldoc
> > Andrew wrote years ago. gcc agrees :
> > 
> > net/ipv4/inet_diag.c: In function ‘inet_csk_diag_fill’:
> > net/ipv4/inet_diag.c:119: warning: cast from pointer to integer of different size
> > net/ipv4/inet_diag.c:120: error: invalid operands to binary >>
> > make[1]: *** [net/ipv4/inet_diag.o] Error 1
> 
> Also you can't do this, the "cookie" is used by the kernel future
> lookups to find sockets.
> 
> The kernel pointer is part of the API, so sorry you can't "hide"
> kernel pointers in this case without really breaking user visible
> things.

But this is precisely what we're trying to control with kptr_restrict.
Setting kptr_restrict will make inet_diag (and some details of similar
things in /proc) meaningless. Based on the name, "diag" isn't going to be
used in normal operation, and kptr_restrict is 0 by default, so only system
owners interested in this will enable it and effectively disable inet_diag.

It seems like everything that fills idiag_cookie needs to be adjusted, not
just the one instance, too:

$ fgrep 'idiag_cookie[0] = ' net/ipv4/inet_diag.c
	r->id.idiag_cookie[0] = (u32)(unsigned long)sk;
	r->id.idiag_cookie[0] = (u32)(unsigned long)tw;
	r->id.idiag_cookie[0] = (u32)(unsigned long)req;

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [patch 1/1] net: convert %p usage to %pK

[David Miller]

From: Kees Cook <kees.cook@canonical.com>
Date: Wed, 25 May 2011 16:29:21 -0700

> Hi David,
> 
> On Tue, May 24, 2011 at 03:58:01AM -0400, David Miller wrote:
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Tue, 24 May 2011 09:45:01 +0200
>> 
>> > Le mardi 24 mai 2011 à 00:35 -0700, Joe Perches a écrit :
>> > 
>> >> I think it's be better without the casts
>> >> using the standard kernel.h macros.
>> >> 
>> >> 	void *ptr;
>> >> 
>> >> 	ptr = maybe_hide_ptr(sk);
>> >> 	r->id.idiag_cookie[0] = lower_32_bits(ptr);
>> >> 	r->id.idiag_cookie[1] = upper_32_bits(ptr);
>> >> 
>> > 
>> > I am not sure I want to patch lower_32_bits() and upper_32_bits() for
>> > this.
>> > 
>> > They dont work on pointers, but on "numbers", according to kerneldoc
>> > Andrew wrote years ago. gcc agrees :
>> > 
>> > net/ipv4/inet_diag.c: In function ‘inet_csk_diag_fill’:
>> > net/ipv4/inet_diag.c:119: warning: cast from pointer to integer of different size
>> > net/ipv4/inet_diag.c:120: error: invalid operands to binary >>
>> > make[1]: *** [net/ipv4/inet_diag.o] Error 1
>> 
>> Also you can't do this, the "cookie" is used by the kernel future
>> lookups to find sockets.
>> 
>> The kernel pointer is part of the API, so sorry you can't "hide"
>> kernel pointers in this case without really breaking user visible
>> things.
> 
> But this is precisely what we're trying to control with kptr_restrict.
> Setting kptr_restrict will make inet_diag (and some details of similar
> things in /proc) meaningless. Based on the name, "diag" isn't going to be
> used in normal operation, and kptr_restrict is 0 by default, so only system
> owners interested in this will enable it and effectively disable inet_diag.

Are you kidding me?

inet_diag is the standard way to dump sockets using netlink.
It's not a special obscure debugging facility, it's for real
users.

And the encoded kernel pointer here is used as a shortcut to looking
up precise sockets.

Re: [patch 1/1] net: convert %p usage to %pK

[Kees Cook]

On Wed, May 25, 2011 at 09:50:40PM -0400, David Miller wrote:
> From: Kees Cook <kees.cook@canonical.com>
> Date: Wed, 25 May 2011 16:29:21 -0700
> 
> > Hi David,
> > 
> > On Tue, May 24, 2011 at 03:58:01AM -0400, David Miller wrote:
> >> From: Eric Dumazet <eric.dumazet@gmail.com>
> >> Date: Tue, 24 May 2011 09:45:01 +0200
> >> 
> >> > Le mardi 24 mai 2011 à 00:35 -0700, Joe Perches a écrit :
> >> > 
> >> >> I think it's be better without the casts
> >> >> using the standard kernel.h macros.
> >> >> 
> >> >> 	void *ptr;
> >> >> 
> >> >> 	ptr = maybe_hide_ptr(sk);
> >> >> 	r->id.idiag_cookie[0] = lower_32_bits(ptr);
> >> >> 	r->id.idiag_cookie[1] = upper_32_bits(ptr);
> >> >> 
> >> > 
> >> > I am not sure I want to patch lower_32_bits() and upper_32_bits() for
> >> > this.
> >> > 
> >> > They dont work on pointers, but on "numbers", according to kerneldoc
> >> > Andrew wrote years ago. gcc agrees :
> >> > 
> >> > net/ipv4/inet_diag.c: In function ‘inet_csk_diag_fill’:
> >> > net/ipv4/inet_diag.c:119: warning: cast from pointer to integer of different size
> >> > net/ipv4/inet_diag.c:120: error: invalid operands to binary >>
> >> > make[1]: *** [net/ipv4/inet_diag.o] Error 1
> >> 
> >> Also you can't do this, the "cookie" is used by the kernel future
> >> lookups to find sockets.
> >> 
> >> The kernel pointer is part of the API, so sorry you can't "hide"
> >> kernel pointers in this case without really breaking user visible
> >> things.
> > 
> > But this is precisely what we're trying to control with kptr_restrict.
> > Setting kptr_restrict will make inet_diag (and some details of similar
> > things in /proc) meaningless. Based on the name, "diag" isn't going to be
> > used in normal operation, and kptr_restrict is 0 by default, so only system
> > owners interested in this will enable it and effectively disable inet_diag.
> 
> Are you kidding me?
> 
> inet_diag is the standard way to dump sockets using netlink.
> It's not a special obscure debugging facility, it's for real
> users.
> 
> And the encoded kernel pointer here is used as a shortcut to looking
> up precise sockets.

We got this dropped from the /proc view; why can't we do the same for
this netlink interface?

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [patch 1/1] net: convert %p usage to %pK

[David Miller]

From: Kees Cook <kees.cook@canonical.com>
Date: Thu, 26 May 2011 17:14:49 -0700

> On Wed, May 25, 2011 at 09:50:40PM -0400, David Miller wrote:
>> From: Kees Cook <kees.cook@canonical.com>
>> Date: Wed, 25 May 2011 16:29:21 -0700
>> 
>> > Hi David,
>> > 
>> > On Tue, May 24, 2011 at 03:58:01AM -0400, David Miller wrote:
>> >> From: Eric Dumazet <eric.dumazet@gmail.com>
>> >> Date: Tue, 24 May 2011 09:45:01 +0200
>> >> 
>> >> > Le mardi 24 mai 2011 à 00:35 -0700, Joe Perches a écrit :
>> >> > 
>> >> >> I think it's be better without the casts
>> >> >> using the standard kernel.h macros.
>> >> >> 
>> >> >> 	void *ptr;
>> >> >> 
>> >> >> 	ptr = maybe_hide_ptr(sk);
>> >> >> 	r->id.idiag_cookie[0] = lower_32_bits(ptr);
>> >> >> 	r->id.idiag_cookie[1] = upper_32_bits(ptr);
>> >> >> 
>> >> > 
>> >> > I am not sure I want to patch lower_32_bits() and upper_32_bits() for
>> >> > this.
>> >> > 
>> >> > They dont work on pointers, but on "numbers", according to kerneldoc
>> >> > Andrew wrote years ago. gcc agrees :
>> >> > 
>> >> > net/ipv4/inet_diag.c: In function ‘inet_csk_diag_fill’:
>> >> > net/ipv4/inet_diag.c:119: warning: cast from pointer to integer of different size
>> >> > net/ipv4/inet_diag.c:120: error: invalid operands to binary >>
>> >> > make[1]: *** [net/ipv4/inet_diag.o] Error 1
>> >> 
>> >> Also you can't do this, the "cookie" is used by the kernel future
>> >> lookups to find sockets.
>> >> 
>> >> The kernel pointer is part of the API, so sorry you can't "hide"
>> >> kernel pointers in this case without really breaking user visible
>> >> things.
>> > 
>> > But this is precisely what we're trying to control with kptr_restrict.
>> > Setting kptr_restrict will make inet_diag (and some details of similar
>> > things in /proc) meaningless. Based on the name, "diag" isn't going to be
>> > used in normal operation, and kptr_restrict is 0 by default, so only system
>> > owners interested in this will enable it and effectively disable inet_diag.
>> 
>> Are you kidding me?
>> 
>> inet_diag is the standard way to dump sockets using netlink.
>> It's not a special obscure debugging facility, it's for real
>> users.
>> 
>> And the encoded kernel pointer here is used as a shortcut to looking
>> up precise sockets.
> 
> We got this dropped from the /proc view; why can't we do the same for
> this netlink interface?

Because it's not only an opaque "output" blob, it's also an input key
for lookups which the user can trigger.

Re: [patch 1/1] net: convert %p usage to %pK

[Eric Dumazet]

Le jeudi 26 mai 2011 à 22:44 -0400, David Miller a écrit :
> From: Kees Cook <kees.cook@canonical.com>
> Date: Thu, 26 May 2011 17:14:49 -0700
> > 
> > We got this dropped from the /proc view; why can't we do the same for
> > this netlink interface?
> 
> Because it's not only an opaque "output" blob, it's also an input key
> for lookups which the user can trigger.

Yes, we wan add a layer to obfuscate the real pointers. We dont trust
values given by user, only match them.

Either we use a XOR with a boot time random value (but let the NULL
cookie being the NULL one), or we generate an unique 64bit socket id for
the cookie (and keep a 64bit cookie in all sockets, increasing ram
usage)

Re: [patch 1/1] net: convert %p usage to %pK

[Ingo Molnar]

* Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Le jeudi 26 mai 2011 à 22:44 -0400, David Miller a écrit :
> > From: Kees Cook <kees.cook@canonical.com>
> > Date: Thu, 26 May 2011 17:14:49 -0700
> > > 
> > > We got this dropped from the /proc view; why can't we do the same for
> > > this netlink interface?
> > 
> > Because it's not only an opaque "output" blob, it's also an input key
> > for lookups which the user can trigger.
> 
> Yes, we wan add a layer to obfuscate the real pointers. We dont 
> trust values given by user, only match them.
> 
> Either we use a XOR with a boot time random value (but let the NULL 
> cookie being the NULL one), or we generate an unique 64bit socket 
> id for the cookie (and keep a 64bit cookie in all sockets, 
> increasing ram usage)

FYI, Dan Rosenberg is currently working on a kernel image 
randomization feature, see this lkml thread:

   [RFC][PATCH] Randomize kernel base address on boot

That will come with an easy vsprintf method to 'unrandomize' IPs. 

( this will be used to display a real-looking /proc/kallsyms and all 
  IPs that the kernel passes to user-space (via perf, etc.) will be 
  unrandomized as well, protecting the randomization seed. )

Once that code goes upstream the networking code could rather simply 
use it to 'randomize' these real data pointers as well. (Assuming you 
never ever pass in zero, that would expose the secret seed.)

The only worry would be statistical analysis performed by local 
attackers: by creating and closing enough sockets on a busy system 
you can over time cover almost all ranges of RAM, so if you can 
observe a pattern of 'over the address space limit' addresses at the 
top or the bottom of the address space you can estimate the random 
seed to within 4-5 bits realistically, maybe even more.

The upper and lower limit of the address space (big holes are useful 
too) can be calculated rather precisely based on RAM size, the 
identity of the system and the kernel version.

So maybe networking real-pointer randomization should be separate 
from kernel IP randomization after all.

Thanks,

	Ingo