On Wed, May 25, 2011 at 09:50:40PM -0400, David Miller wrote:

From: Kees Cook <redacted>
Date: Wed, 25 May 2011 16:29:21 -0700

quoted

Hi David,

On Tue, May 24, 2011 at 03:58:01AM -0400, David Miller wrote:

quoted

From: Eric Dumazet <redacted>
Date: Tue, 24 May 2011 09:45:01 +0200

quoted

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

quoted

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