generic object ids, v2

The routine XORs the given pointer with a random value producing an ID (32 or 64 bit, depending on the arch).

Since it's a valuable information -- only CAP_SYS_ADMIN is allowed to obtain it.

- Tejun worried about the single poison value was a weak side - leaking one makes all the IDs vulnerable. To address this several poison values - one per object type - are introduced. They are stored in a plain array.
- Pekka proposed to initialized poison values in the late_initcall callback
- ... and move the code to mm/util.c

Quoted 6 lines · On Fri, 23 Dec 2011 16:47:42 +0400 Cyrill Gorcunov <[email protected]> wrote:

The routine XORs the given pointer with a random value producing an ID (32 or 64 bit, depending on the arch).

Since it's a valuable information -- only CAP_SYS_ADMIN is allowed to obtain it.

Quoted 6 lines

- Tejun worried about the single poison value was a weak side - leaking one makes all the IDs vulnerable. To address this several poison values - one per object type - are introduced. They are stored in a plain array.
- Pekka proposed to initialized poison values in the late_initcall callback
- ... and move the code to mm/util.c

I'm trying to remember what this is all about, and I don't want to have to remember all the discussion from last time this came up!

Please, do cover all this in the changelogs: tell us what the code is all for and try to capture the design decisions thus far. It's a useful reminder for current reviewers and is very valuable for new reviewers.

The root-only restriction sounds like a pretty bad one. I suspect it really isn't that bad but again, the changelog should discuss the pros and cons here.

A thought: if all we're trying to do here is to check for the sameness of objects, can we push the comparison into the kernel so we don't have this exporting-sensitive-info problem at all? Just return a boolean to userspace?

Something like

int sys_pid_fields_equal(pid_t pid1, pid_t pid2, enum pid_field field_id);

?

For /proc/pid/fdinfo/* userspace can open /proc/pid1/fdinfo/0 and /proc/pid2/fdinfo/0 and call sys_are_these_files_the_same(fd1, fd2, ...).

Perhaps sys_pid_fields_equal() can use sys_are_these_files_the_same() as well, if we can think up a way of passing it two fds to represent the two pids.

Have a think about it ;)

Maybe we could simply extend prctl then?

Quoted 1 line · On Wed, 28 Dec 2011 11:42:14 +0400 Cyrill Gorcunov <[email protected]> wrote:

Maybe we could simply extend prctl then?

I'd suggest a syscall. prctl is a big crap bucket and this thing has simple, well defined semantics. syscalls are cheap.

A thought: if all we're trying to do here is to check for the sameness of objects, can we push the comparison into the kernel so we don't have this exporting-sensitive-info problem at all? Just return a boolean to userspace?

Something like

int sys_pid_fields_equal(pid_t pid1, pid_t pid2, enum pid_field field_id);

?

For /proc/pid/fdinfo/* userspace can open /proc/pid1/fdinfo/0 and /proc/pid2/fdinfo/0 and call sys_are_these_files_the_same(fd1, fd2, ...).

Perhaps sys_pid_fields_equal() can use sys_are_these_files_the_same() as well, if we can think up a way of passing it two fds to represent the two pids.

Have a think about it ;)

Quoted 14 lines

A thought: if all we're trying to do here is to check for the sameness of objects, can we push the comparison into the kernel so we don't have this exporting-sensitive-info problem at all? Just return a boolean to userspace?

Something like

int sys_pid_fields_equal(pid_t pid1, pid_t pid2, enum pid_field field_id);

?

For /proc/pid/fdinfo/* userspace can open /proc/pid1/fdinfo/0 and /proc/pid2/fdinfo/0 and call sys_are_these_files_the_same(fd1, fd2, ...).

Quoted 5 lines

Perhaps sys_pid_fields_equal() can use sys_are_these_files_the_same() as well, if we can think up a way of passing it two fds to represent the two pids.

Have a think about it ;)

With this the complexity of determining sharing for N files scattered across several tasks would be N^2, since we'll have to compare each file to each file.

On the other hand having just N IDs at hands would allow us to use more efficient algorithms resulting in e.g. N*log(N) complexity.

That said I'd really appreciate if we work out a solution with IDs.

The routine XORs the given pointer with a random value producing an ID (32 or 64 bit, depending on the arch).

Since it's a valuable information -- only CAP_SYS_ADMIN is allowed to obtain it.

- Tejun worried about the single poison value was a weak side - leaking one makes all the IDs vulnerable. To address this several poison values - one per object type - are introduced. They are stored in a plain array.
- Pekka proposed to initialized poison values in the late_initcall callback
- ... and move the code to mm/util.c

...

mm/Kconfig +12 -0

+	default n
+	help
+	  Turn on the functionality that can generate IDs for kernel
+	  objects, which are exported to userspace via /proc filesystem.
+
+	  It is useful if you need to examinate kernel objects and test
+	  if they are shared between several tasks. These IDs should never
+	  be used for anything but the "sameness" test. Besides, the IDs are
+	  dynamic and valid only while object is alive, once it get freed or
+	  kernel is rebooted -- the IDs will be changed.
+
+	  If unsure, say N here.

Index: linux-2.6.git/mm/Makefile
===================================================================

mm/Makefile +1 -0

@@ -51,3 +51,4 @@ obj-$(CONFIG_HWPOISON_INJECT) += hwpoiso
 obj-$(CONFIG_DEBUG_KMEMLEAK) += kmemleak.o
 obj-$(CONFIG_DEBUG_KMEMLEAK_TEST) += kmemleak-test.o
 obj-$(CONFIG_CLEANCACHE) += cleancache.o
+obj-$(CONFIG_GENERIC_OBJECT_ID) += gen_obj_id.o

Index: linux-2.6.git/mm/gen_obj_id.c
===================================================================

Quoted 6 lines · On Fri, 23 Dec 2011 16:47:42 +0400 Cyrill Gorcunov <[email protected]> wrote:

The routine XORs the given pointer with a random value producing an ID (32 or 64 bit, depending on the arch).

Since it's a valuable information -- only CAP_SYS_ADMIN is allowed to obtain it.

Quoted 9 lines

- Tejun worried about the single poison value was a weak side - leaking one makes all the IDs vulnerable. To address this several poison values - one per object type - are introduced. They are stored in a plain array.
- Pekka proposed to initialized poison values in the late_initcall callback
- ... and move the code to mm/util.c

...

The code in general looks simple and reasonable to me. I'm too much of a security weenie to pass judgement on the security aspects.

Quoted 8 lines

...

mm/Kconfig +0 -0

@@ -373,3 +373,19 @@ config CLEANCACHE
 	  in a negligible performance hit.
 

mm/Kconfig config CLEANCACHE · line 379

Quoted 5 lines

mm/Kconfig +4 -0

 	  If unsure, say Y to enable cleancache
+
+config GENERIC_OBJECT_ID
+	bool "Enable generic object ID infrastructure"
+	depends on CHECKPOINT_RESTORE

Is c/r useless without GENERIC_OBJECT_ID? If so, perhaps a `select' would be good here.

Quoted 12 lines

mm/Kconfig +12 -0

+	default n
+	help
+	  Turn on the functionality that can generate IDs for kernel
+	  objects, which are exported to userspace via /proc filesystem.
+
+	  It is useful if you need to examinate kernel objects and test
+	  if they are shared between several tasks. These IDs should never
+	  be used for anything but the "sameness" test. Besides, the IDs are
+	  dynamic and valid only while object is alive, once it get freed or
+	  kernel is rebooted -- the IDs will be changed.
+
+	  If unsure, say N here.

Quoted 3 lines

Is c/r useless without GENERIC_OBJECT_ID? If so, perhaps a `select' would be good here.

Is GENERIC_OBJECT_ID useful elsewhere? If so, then it shouldn't depend on CHECKPOINT_RESTORE -- it should just live in lib/ instead of in mm/ and CHECKPOINT_RESTORE should select it.

mm/Kconfig config CLEANCACHE · line 382

Quoted 6 lines

mm/Kconfig +6 -0

+config GENERIC_OBJECT_ID
+	bool "Enable generic object ID infrastructure"
+	depends on CHECKPOINT_RESTORE
+	default n
+	help
+	  Turn on the functionality that can generate IDs for kernel

"Turn on functionality"

mm/Kconfig config CLEANCACHE · line 383

Quoted 1 line

mm/Kconfig +1 -0

+	  objects, which are exported to userspace via /proc filesystem.

Lose the ,

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	  It is useful if you need to examinate kernel objects and test

examine

mm/Kconfig config CLEANCACHE · line 387

Quoted 2 lines

mm/Kconfig +2 -0

+	  if they are shared between several tasks. These IDs should never
+	  be used for anything but the "sameness" test. Besides, the IDs are

s/Besides, the/The/

mm/Kconfig config CLEANCACHE · line 388

Quoted 1 line

mm/Kconfig +1 -0

+	  dynamic and valid only while object is alive, once it get freed or

s/alive, once/alive. Once/

mm/Kconfig config CLEANCACHE · line 389

Quoted 1 line

mm/Kconfig +1 -0

+	  kernel is rebooted -- the IDs will be changed.

s/ --/,/

Quoted 4 lines

mm/Kconfig config CLEANCACHE · line 389

Quoted 2 lines

mm/Kconfig +1 -0

+	  kernel is rebooted -- the IDs will be changed.

s/ --/,/

heh, I avoided mentioning these things because I usually rewrite the user-facing text when merging patches from non-native English speakers.

mm/gen_obj_id.c new file +6 -0

+unsigned long gen_obj_id(void *ptr, int type)
+{
+	if (!capable(CAP_SYS_ADMIN) || !ptr)
+		return 0;
+
+	BUG_ON(type >= GEN_OBJ_ID_TYPES);

Hello, Cyrill.

Just my 2 cents.

Quoted 6 lines · Cyrill Gorcunov

mm/gen_obj_id.c new file +6 -0

+unsigned long gen_obj_id(void *ptr, int type)
+{
+	if (!capable(CAP_SYS_ADMIN) || !ptr)
+		return 0;
+
+	BUG_ON(type >= GEN_OBJ_ID_TYPES);

mm/gen_obj_id.c line 25

Quoted 10 lines

mm/gen_obj_id.c new file +10 -0

+
+	/*
+	 * Note the simple XOR is used here not in a sake
+	 * of security by any means, but rather to break
+	 * an "impression" that such IDs means something
+	 * other than a number which can be used for comparison
+	 * with another number generated by this helper only.
+	 */
+	return ((unsigned long)ptr) ^ gen_obj_cookie[type];
+}

To me, XOR & CAP_SYS_ADMIN combination doesn't make much sense. With CAP_SYS_ADMIN, there's no reason for XOR - we can just export pointers. If we plan on removing CAP_SYS_ADMIN restriction down the road, XOR doesn't help much. It's too weak. The XOR is unnecessary with CAP_SYS_ADMIN and useless without it. It seems pointless to me. If we're going down this route, I think doing cryptographically safe hash would be much better.

Hi Tejun, thanks for comment! Yes, XOR is useless here in security meaning, but it simply breaks impression that these generating numbers "mean" somthing (I remained them as Vasily asked).

I personally fine to simply leave plain pointers here and root-only access since that is enough for us (and our tool will require root privileges anyway :)

OTOH, we could add some sha2 here with pointer+cookie as an initial value but I fear this will bring more code comlexity and computing sha2 hash is not that fast operation, which should be taken into account (note on x86-32 since pointers are 32bit values one could compute prehash for all space covered and if an attacker will know somehow cookie value the hash will be easily broken, not sure if it's really usefull for someone, since if you have root access to the machine such IDs will be the last thing attacker should be interested in :)

And it seems noone except us need this interface yet, so maybe sticking with "pointer exported under root-only" would be enough?

Hello,

Quoted 3 lines · Cyrill Gorcunov

Hi Tejun, thanks for comment! Yes, XOR is useless here in security meaning, but it simply breaks impression that these generating numbers "mean" somthing (I remained them as Vasily asked).

But that comes at the cost of creating the impression that the XOR does something, which doesn't seem like a good situation. e.g. Why do we need per-domain XOR random keys for then? That code now doesn't mean anything.

Quoted 4 lines

I personally fine to simply leave plain pointers here and root-only access since that is enough for us (and our tool will require root privileges anyway :)

Quoted 8 lines

OTOH, we could add some sha2 here with pointer+cookie as an initial value but I fear this will bring more code comlexity and computing sha2 hash is not that fast operation, which should be taken into account (note on x86-32 since pointers are 32bit values one could compute prehash for all space covered and if an attacker will know somehow cookie value the hash will be easily broken, not sure if it's really usefull for someone, since if you have root access to the machine such IDs will be the last thing attacker should be interested in :)

We have the whole crypto subsystem dealing with this. It sure would be more complex than ^ operator but it's not like you have to open code the whole thing. Is it really that complex to use?

Quoted 3 lines

And it seems noone except us need this interface yet, so maybe sticking with "pointer exported under root-only" would be enough?

Maybe, dunno. But even if it's gonna be raw pointer or XOR'd value for now, I would suggest exporting it in the form which can be replaced by proper hash in the future. ie. Don't let userland assume it's 32bit or 64bit value.

Quoted 3 lines

We have the whole crypto subsystem dealing with this. It sure would be more complex than ^ operator but it's not like you have to open code the whole thing. Is it really that complex to use?

No, Tejun, the use of crypto engine is not hard but it means more memory consumption (one need to carry resulting hashes and print them out into /proc) and more cpu consuption while we really need some fast and cheap solution. Unlike other usage of crypto engine (such as encoding for net layer, iirc ipsec uses it) I'm not really convinced we should use that heavy artillery here ;)

I see, I could use some other form of output, it's not a problem. The main problem which interface community prefer, should I really switch to crypto usage or we can leave with root-only+plain-pointer approach?

Hello,

Quoted 10 lines · Cyrill Gorcunov

Quoted 3 lines

We have the whole crypto subsystem dealing with this. It sure would be more complex than ^ operator but it's not like you have to open code the whole thing. Is it really that complex to use?

No, Tejun, the use of crypto engine is not hard but it means more memory consumption (one need to carry resulting hashes and print them out into /proc) and more cpu consuption while we really need some fast and cheap solution. Unlike other usage of crypto engine (such as encoding for net layer, iirc ipsec uses it) I'm not really convinced we should use that heavy artillery here ;)

But the cost would be attributed to the user requesting that specific data and given the amount of data to be hashed, I don't think the computational or memory overhead should be the deciding design factor here. There are far more grave issues here. Userland visible API and security.

Quoted 3 lines

I see, I could use some other form of output, it's not a problem. The main problem which interface community prefer, should I really switch to crypto usage or we can leave with root-only+plain-pointer approach?

I don't know either but if proper hashing (crypto or not) is simple enough, this really isn't a tradeoff we need to make, no?

Well, it is not deciding but it should be taken into account. One could be reading this IDs again and again and again affecting performance of the whole system,

which means I really would prefer to limit access to such
features (ie root-only). If (as I said) for other cases there is simply no way to _not_ use crypto, our case might be the one where using crypto is redundant.

Hello,

Quoted 3 lines · Cyrill Gorcunov

Well, it is not deciding but it should be taken into account. One could be reading this IDs again and again and again affecting performance of the whole system,

I can't see how it would affect performance of the whole system. Calculating hash doesn't involve any further locking or use of scarce global resource. It would add small amount memory and processing overhead for the task reading the hash. It sure is something to be considered but I really don't think this should be a major factor here.

Quoted 3 lines

which means I really would prefer to limit access to such
features (ie root-only). If (as I said) for other cases there is simply no way to _not_ use crypto, our case might be the one where using crypto is redundant.

Limiting it to root and just exporting printer (or maybe XOR with a randomish value) may be good enough. I don't know. However, we no longer consider exporting pointers to unpriviliedged userland safe and this can be useful in many circumstances, so if it's not too difficult, I think trying to use proper hash would be nide.

Yeah, indeed.

Quoted 4 lines

Quoted 3 lines

which means I really would prefer to limit access to such
features (ie root-only). If (as I said) for other cases there is simply no way to _not_ use crypto, our case might be the one where using crypto is redundant.

Quoted 5 lines

Limiting it to root and just exporting printer (or maybe XOR with a randomish value) may be good enough. I don't know. However, we no longer consider exporting pointers to unpriviliedged userland safe and this can be useful in many circumstances, so if it's not too difficult, I think trying to use proper hash would be nide.

OK, Tejun, I'll try, but no promises :) Thanks!

Tejun, I've tried to use crypto engine here but it produced a warning about being used in non-sleepable context (which takes place when we read /proc/<pid>/fdinfo/* files). So I used lib/sha1.c instead. The final result is below, please review.

The output as expected is sha1 hash but note I still placed root-only access here since I prefer security guys to confirm if such production is indeed safe to be exported for regular users.

| [root@localhost ~]# cat /proc/2/ns/net
| id: ebfc829fc16a466c9c02b031ceab65c277040c02

Cyrill
---
From: Cyrill Gorcunov <[email protected]>
Subject: Add routine for generating an ID for kernel pointer v2

The routine XORs the given pointer with a random value, expands the production to sha1 message block and compute the sha1 hash producing an ID.

Util confirmed that the production is a safe one to be exported for the unprivileged users CAP_SYS_ADMIN guird is used.

Because the routine might be used in non-sleepable context built-in realization of sha1 is chosen instead of crypto manager and its helpers.

v1:
- Tejun worried about the single poison value was a weak side - leaking one makes all the IDs vulnerable. To address this several poison values - one per object type - are introduced. They are stored in a plain array.
- Pekka proposed to initialized poison values in the late_initcall callback
- ... and move the code to mm/util.c, but eventually it's moved to an own file.

v2:
- Tejun suggested to use crypto engine instead of plain XOR and provide hashes to unprivilege users as well.
- A number of fixes for Kconfig prompt from Valdis
- Andrew pointed more #incclude's are needed

Based-on-patch-from: Pavel Emelyanov <[email protected]>

Signed-off-by Cyrill Gorcunov <[email protected]>

CC Glauber Costa <[email protected]>

CC Andi Kleen <[email protected]>

CC Tejun Heo <[email protected]>

CC Matt Helsley <[email protected]>

CC Pekka Enberg <[email protected]>

CC Eric Dumazet <[email protected]>

CC Vasiliy Kulikov <[email protected]>

CC Andrew Morton <[email protected]>

CC Alexey Dobriyan <[email protected]>

CC [email protected]

---
 include/linux/gen_obj_id.h |   26 ++++++++++++++
 mm/Kconfig                 |   18 ++++++++++
 mm/Makefile                |    1 
 mm/gen_obj_id.c            |   81 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 126 insertions(+)

Index: linux-2.6.git/include/linux/gen_obj_id.h
===================================================================

include/linux/gen_obj_id.h new file +25 -0

@@ -0,0 +1,26 @@
+#ifndef _LINUX_GEN_OBJ_ID_H
+#define _LINUX_GEN_OBJ_ID_H
+
+#include <linux/cryptohash.h>
+#include <linux/err.h>
+
+#ifdef __KERNEL__
+
+enum {
+	GEN_OBJ_ID_TYPES,
+};
+#define GEN_OBJ_ID_DIGEST_SIZE	(SHA_DIGEST_WORDS * sizeof(__u32))
+#define GEN_OBJ_ID_BUF_SIZE	(GEN_OBJ_ID_DIGEST_SIZE * 2)
+
+#ifdef CONFIG_GENERIC_OBJECT_ID
+extern int gen_obj_id(void *ptr, int type, char *dst, unsigned long size);
+#else
+static inline int gen_obj_id(void *ptr, int type, char *dst, unsigned long size)
+{
+	return -EINVAL;
+}
+#endif
+
+#endif /* __KERNEL__ */
+#endif /* _LINUX_GEN_OBJ_ID_H */

Index: linux-2.6.git/mm/Kconfig
===================================================================

mm/Kconfig +18 -0

@@ -373,3 +373,21 @@ config CLEANCACHE
 	  in a negligible performance hit.
 
 	  If unsure, say Y to enable cleancache
+
+config GENERIC_OBJECT_ID
+	bool "Enable generic object ID infrastructure"
+	depends on CHECKPOINT_RESTORE
+	depends on CRYPTO_SHA1
+	default n
+	help
+	  Turn on functionality that can generate IDs for kernel
+	  objects, which are exported to the userspace via /proc
+	  filesystem.
+
+	  It is useful if you need to examine kernel objects and test
+	  if they are shared between several tasks. These IDs should never
+	  be used for anything but the "sameness" test. The IDs are dynamic
+	  and valid only while object is alive. Once it get freed or kernel
+	  is rebooted, the IDs will be changed.
+
+	  If unsure, say N here.

Index: linux-2.6.git/mm/Makefile
===================================================================

mm/Makefile +1 -0

@@ -51,3 +51,4 @@ obj-$(CONFIG_HWPOISON_INJECT) += hwpoiso
 obj-$(CONFIG_DEBUG_KMEMLEAK) += kmemleak.o
 obj-$(CONFIG_DEBUG_KMEMLEAK_TEST) += kmemleak-test.o
 obj-$(CONFIG_CLEANCACHE) += cleancache.o
+obj-$(CONFIG_GENERIC_OBJECT_ID) += gen_obj_id.o

Index: linux-2.6.git/mm/gen_obj_id.c
===================================================================

mm/gen_obj_id.c new file +81 -0

@@ -0,0 +1,81 @@
+#include <linux/kernel.h>
+#include <linux/capability.h>
+#include <linux/cryptohash.h>
+#include <linux/string.h>
+#include <linux/random.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/cache.h>
+#include <linux/bug.h>
+#include <linux/err.h>
+
+#include <linux/gen_obj_id.h>
+
+static unsigned long gen_obj_cookie[GEN_OBJ_ID_TYPES] __read_mostly;
+
+int gen_obj_id(void *ptr, int type, char *dst, unsigned long size)
+{
+	__u32 hash[SHA_DIGEST_WORDS];
+	__u32 workspace[SHA_WORKSPACE_WORDS];
+	__u8 extract[SHA_MESSAGE_BYTES];
+	__u8 *t = (__u8 *)hash;
+	unsigned long id;
+	int i;
+
+	BUG_ON(type >= GEN_OBJ_ID_TYPES);
+
+	if (!capable(CAP_SYS_ADMIN) ||
+	    size < GEN_OBJ_ID_BUF_SIZE) {
+		memset(dst, '0', size);
+		return -EINVAL;
+	}
+
+	id = ((unsigned long)ptr) ^ gen_obj_cookie[type];
+
+	BUILD_BUG_ON(SHA_MESSAGE_BYTES % sizeof(long));
+
+	/*
+	 * Expand the ID to the whole message block.
+	 */
+	for (i = 0; i < (SHA_MESSAGE_BYTES / sizeof(long)); i++)
+		((long *)extract)[i] = id;
+
+	sha_init(hash);
+	sha_transform(hash, extract, workspace);
+
+	snprintf(dst, size,
+		 "%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x"
+		 "%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x",
+		 t[ 0], t[ 1], t[ 2], t[ 3], t[ 4],
+		 t[ 5], t[ 6], t[ 7], t[ 8], t[ 9],
+		 t[10], t[11], t[12], t[13], t[14],
+		 t[15], t[16], t[17], t[18], t[19]);
+
+	return 0;
+}
+
+static __init int gen_obj_cookie_init(void)
+{
+#if BITS_PER_LONG == 64
+	const unsigned long emergency_cookie = 0xefcdab8967452301;
+#else
+	const unsigned long emergency_cookie = 0x98badcf9;
+#endif
+	int i;
+
+	for (i = 0; i < GEN_OBJ_ID_TYPES; i++) {
+		get_random_bytes(&gen_obj_cookie[i],
+				 sizeof(unsigned long));
+		/*
+		 * In 'impossible' case of random-bytes = 0
+		 * we still would have non-zero value.
+		 */
+		gen_obj_cookie[i] =
+			(gen_obj_cookie[i] & __PAGE_OFFSET) +
+			(emergency_cookie & ~__PAGE_OFFSET);
+	}
+
+	return 0;
+}
+
+late_initcall(gen_obj_cookie_init);

Hello,

Quoted 4 lines · Cyrill Gorcunov

Tejun, I've tried to use crypto engine here but it produced a warning about being used in non-sleepable context (which takes place when we read /proc/<pid>/fdinfo/* files). So I used lib/sha1.c instead. The final result is below, please review.

I don't know anything about cryptography and have no idea whether sha1 is good enough, so I can't really say much. :)

Which part triggered the context warning? IIRC, crypto context preparation and actual calculation can be done in separate steps. Can't the calculation part be done from non-sleepable context?

cc'ing Herbert & David and quoting the whole message.

Probably I've had to crypto_alloc_hash earlier and simply keep a reference to algo but since I'm not sure if looking for modules in late-init-call is good idea.

Quoted 4 lines · Cyrill Gorcunov

Probably I've had to crypto_alloc_hash earlier and simply keep a reference to algo but since I'm not sure if looking for modules in late-init-call is good idea.

Right, the allocation needs to occur in a sleepable context.

If you're just hashing something small and have no need for hardware acceleration then lib/sha1.c is fine.

Quoted 11 lines · Herbert Xu

Quoted 4 lines · Cyrill Gorcunov

Probably I've had to crypto_alloc_hash earlier and simply keep a reference to algo but since I'm not sure if looking for modules in late-init-call is good idea.

Right, the allocation needs to occur in a sleepable context.

If you're just hashing something small and have no need for hardware acceleration then lib/sha1.c is fine.

Hi, yeah, it's just one message block hashing so I've switched to lib/sha1.c. Herbert, I'm more interested in security analysis -- would the sha1(msg), where the 'msg' is the kernel pointer XOR'ed with random value and expanded to the 512 bits would be safe enough for export to unprivilege users?

Quoted 13 lines · Cyrill Gorcunov

Quoted 11 lines · Herbert Xu

Quoted 4 lines · Cyrill Gorcunov

Probably I've had to crypto_alloc_hash earlier and simply keep a reference to algo but since I'm not sure if looking for modules in late-init-call is good idea.

Right, the allocation needs to occur in a sleepable context.

If you're just hashing something small and have no need for hardware acceleration then lib/sha1.c is fine.

Quoted 5 lines

Hi, yeah, it's just one message block hashing so I've switched to lib/sha1.c. Herbert, I'm more interested in security analysis -- would the sha1(msg), where the 'msg' is the kernel pointer XOR'ed with random value and expanded to the 512 bits would be safe enough for export to unprivilege users?

Even if now we don't know an attacking way of sha1 reverse hashing, we may discover within 10 years. Many secure messages lost from hardware speedup and new algorithm attack. so, nobody can say it's abi safe.

And, if you don't use perfect hash, you may have a hash collision risk. What's happen if different pointer makes same ID?

Quoted 20 lines · KOSAKI Motohiro

Quoted 13 lines · Cyrill Gorcunov

Quoted 11 lines · Herbert Xu

Quoted 4 lines · Cyrill Gorcunov

Probably I've had to crypto_alloc_hash earlier and simply keep a reference to algo but since I'm not sure if looking for modules in late-init-call is good idea.

Right, the allocation needs to occur in a sleepable context.

If you're just hashing something small and have no need for hardware acceleration then lib/sha1.c is fine.

Quoted 5 lines

Hi, yeah, it's just one message block hashing so I've switched to lib/sha1.c. Herbert, I'm more interested in security analysis -- would the sha1(msg), where the 'msg' is the kernel pointer XOR'ed with random value and expanded to the 512 bits would be safe enough for export to unprivilege users?

Quoted 5 lines

Even if now we don't know an attacking way of sha1 reverse hashing, we may discover within 10 years. Many secure messages lost from hardware speedup and new algorithm attack. so, nobody can say it's abi safe.

Yes, I know. But there is a big difference between direct hash crack and indirect crack caused by limited space of data used for such hash. That's the reason why random cookie was used and xor production was expanded to the whole message block.

Quoted 2 lines

And, if you don't use perfect hash, you may have a hash collision risk. What's happen if different pointer makes same ID?

Well, strictly speaking this is pretty bad case for me. Of course this wont lead to catastrophic results for user-space application I think but definitely I would prefer to not have collisions here.

Guys, this become more and more complex, finally I fear someone propose to do ideal hashing run-time ;) Maybe we can step back and live with root-only and plain pointers here? I'm not sure who else might need such facility except us, and if once there will be a candidate -- we could take a look on hashing again and provide safe hashes there. No?

Quoted 6 lines

Guys, this become more and more complex, finally I fear someone propose to do ideal hashing run-time ;) Maybe we can step back and live with root-only and plain pointers here? I'm not sure who else might need such facility except us, and if once there will be a candidate -- we could take a look on hashing again and provide safe hashes there. No?

But recently kernel security fashion are, we don't expose a kernel pointer at all even though the file is root only. I'm not sure how much effective such fashion. but you seems run opposite way.

I doubt user land can implement good comparison way. Why you gave up Andrew's sys_are_these_files_the_same() idea?

On Fri, Dec 30, 2011 at 06:51:37PM -0500, KOSAKI Motohiro wrote:
...

Quoted 13 lines

Quoted 6 lines

Guys, this become more and more complex, finally I fear someone propose to do ideal hashing run-time ;) Maybe we can step back and live with root-only and plain pointers here? I'm not sure who else might need such facility except us, and if once there will be a candidate -- we could take a look on hashing again and provide safe hashes there. No?

But recently kernel security fashion are, we don't expose a kernel pointer at all even though the file is root only. I'm not sure how much effective such fashion. but you seems run opposite way.

I doubt user land can implement good comparison way. Why you gave up Andrew's sys_are_these_files_the_same() idea?

Because of speed, as Pavel mentioned

 | With this the complexity of determining sharing for N files scattered
 | across several tasks would be N^2, since we'll have to compare each
 | file to each file.
 | 
 | On the other hand having just N IDs at hands would allow us to use more
 | efficient algorithms resulting in e.g. N*log(N) complexity.

Cyrill
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 14 lines · Cyrill Gorcunov

On Fri, Dec 30, 2011 at 06:51:37PM -0500, KOSAKI Motohiro wrote:
...

Quoted 11 lines

Quoted 6 lines

Guys, this become more and more complex, finally I fear someone propose to do ideal hashing run-time ;) Maybe we can step back and live with root-only and plain pointers here? I'm not sure who else might need such facility except us, and if once there will be a candidate -- we could take a look on hashing again and provide safe hashes there. No?

But recently kernel security fashion are, we don't expose a kernel pointer at all even though the file is root only. I'm not sure how much effective such fashion. but you seems run opposite way.

Quoted 2 lines

Quoted 2 lines

I doubt user land can implement good comparison way. Why you gave up Andrew's sys_are_these_files_the_same() idea?

By memory, it seems that fuse expose kernel pointer encrypting it with tea. Tea is simple and quick you should get a glimpse at it.

mm/gen_obj_id.c new file +3 -0

+int gen_obj_id(void *ptr, int type, char *dst, unsigned long size)
+{
[...]
+       unsigned long key;
[...]

[...]
+       key = ((unsigned long)ptr) ^ gen_obj_cookie[type];
+       sg_init_one(sg, &key, sizeof(key));

+               snprintf(dst, size,
+                        "%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x"
+                        "%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x",
+                        tmp[ 0], tmp[ 1], tmp[ 2], tmp[ 3], tmp[ 4],
+                        tmp[ 5], tmp[ 6], tmp[ 7], tmp[ 8], tmp[ 9],
+                        tmp[10], tmp[11], tmp[12], tmp[13], tmp[14],
+                        tmp[15], tmp[16], tmp[17], tmp[18], tmp[19]);

Don't hexdump the key, just return pure binary and allow the user to encode it if they care (EG: files in "proc" would hexdump, but a syscall would not). Also, I somehow suspect there is a preexisting function to do this for a byte array.

This patch adds proc_ns_read method which provides IDs for /proc/pid/ns/* files.

Based-on-patch-from: Pavel Emelyanov <[email protected]>

Signed-off-by Cyrill Gorcunov <[email protected]>

CC Glauber Costa <[email protected]>

CC Andi Kleen <[email protected]>

CC Tejun Heo <[email protected]>

CC Matt Helsley <[email protected]>

CC Pekka Enberg <[email protected]>

CC Eric Dumazet <[email protected]>

CC Vasiliy Kulikov <[email protected]>

CC Andrew Morton <[email protected]>

CC Alexey Dobriyan <[email protected]>

---
 Documentation/filesystems/proc.txt |   24 ++++++++++++++++++++++++
 fs/proc/namespaces.c               |   22 ++++++++++++++++++++++
 include/linux/gen_obj_id.h         |    1 +
 3 files changed, 47 insertions(+)

Index: linux-2.6.git/Documentation/filesystems/proc.txt
===================================================================

Documentation/filesystems/proc.txt +24 -0

@@ -40,6 +40,7 @@ Table of Contents
   3.4	/proc/<pid>/coredump_filter - Core dump filtering settings
   3.5	/proc/<pid>/mountinfo - Information about mounts
   3.6	/proc/<pid>/comm  & /proc/<pid>/task/<tid>/comm
+  3.7	/proc/<pid>/ns - Information about namespaces
 
 
 ------------------------------------------------------------------------------

@@ -1545,3 +1546,26 @@ a task to set its own or one of its thre
 is limited in size compared to the cmdline value, so writing anything longer
 then the kernel's TASK_COMM_LEN (currently 16 chars) will result in a truncated
 comm value.
+
+3.7	/proc/<pid>/ns - Information about namespaces
+-----------------------------------------------------
+
+This directory consists of the following files "net", "uts", "ipc",
+and depend if appropriate CONFIG_ entry is set, i.e. it's possible
+to have only one, two or all three files here.
+
+Currently file contents provides that named "object id" number, which
+is a number useful for the one purpose only -- to test if two differen
+<pid> share the namespace.
+
+A typical format is
+
+id:	445332486300860161
+
+i.e. "id" followed by a number. One should never assume the number
+means something, it is only useful for "sameness" test with another number
+obtained from another <pid>.
+
+Moreover, a safe approach is to remember it as a string, since format may
+change in future and id would be not a long integer value, but something
+else, say SHA1/2 or even uuid encoded stream.

Index: linux-2.6.git/fs/proc/namespaces.c
===================================================================

fs/proc/namespaces.c +22 -0

@@ -12,6 +12,7 @@
 #include <linux/mnt_namespace.h>
 #include <linux/ipc_namespace.h>
 #include <linux/pid_namespace.h>
+#include <linux/gen_obj_id.h>
 #include "internal.h"
 
 

@@ -27,10 +28,31 @@ static const struct proc_ns_operations *
 #endif
 };
 
+#ifdef CONFIG_GENERIC_OBJECT_ID
+static ssize_t proc_ns_read(struct file *file, char __user *buf,
+				      size_t len, loff_t *ppos)
+{
+	struct proc_inode *ei = PROC_I(file->f_dentry->d_inode);
+	char tmp[32];
+
+	snprintf(tmp, sizeof(tmp), "id:\t%lu\n",
+		gen_obj_id(ei->ns, GEN_OBJ_ID_NS));
+	return simple_read_from_buffer(buf, len, ppos, tmp, strlen(tmp));
+}
+
+static const struct file_operations ns_file_operations = {
+	.llseek		= no_llseek,
+	.read		= proc_ns_read,
+};
+
+#else
+
 static const struct file_operations ns_file_operations = {
 	.llseek		= no_llseek,
 };
 
+#endif /* CONFIG_GENERIC_OBJECT_ID */
+
 static struct dentry *proc_ns_instantiate(struct inode *dir,
 	struct dentry *dentry, struct task_struct *task, const void *ptr)
 {

Index: linux-2.6.git/include/linux/gen_obj_id.h
===================================================================

include/linux/gen_obj_id.h +1 -1

@@ -4,6 +4,7 @@
 #ifdef __KERNEL__
 
 enum {
+	GEN_OBJ_ID_NS,
 	GEN_OBJ_ID_TYPES,
 };
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 2 lines · Cyrill Gorcunov

This patch adds proc_ns_read method which provides IDs for /proc/pid/ns/* files.

Nacked-by "Eric W. Biederman" <[email protected]>

This is a poorly thought out user interface. If we are going to return this kind of information and I believe we should we should, we should return the id in the inode field with stat.

Comparing device+inode for equality is the traditional way to see if two objects are the same in unix and there is no reason to make up a new interface to get this functionality.

Furthermore we should always return the information, as it is valuable even outside of the checkpoint/restart context.

I am also concerned that you appear to be building an interface for use by checkpoint/restart that makes it impossible checkpoint/restart the programs using that interface. The reason is that you appear to be putting this nebulous id into a global namespace and as such even if we wanted to I don't see how we could build a version where we could restore the id during a restart. And the thing is if you start building interfaces with identifiers you can not possibly restore I expect you will find you have painted yourself into a corner.

Using inode from stat avoids painting yourself into a corner because you have the possibility of different mounts with different device numbers having different inode numbers.

For the short term I don't see value in being able to restore the object identifiers, but I do see a lot of value in allowing for a future where nested checkpoint/restart is an option.

Eric

Quoted 32 lines · Eric W. Biederman

Quoted 2 lines · Cyrill Gorcunov

This patch adds proc_ns_read method which provides IDs for /proc/pid/ns/* files.

Nacked-by "Eric W. Biederman" <[email protected]>

This is a poorly thought out user interface. If we are going to return this kind of information and I believe we should we should, we should return the id in the inode field with stat.

Comparing device+inode for equality is the traditional way to see if two objects are the same in unix and there is no reason to make up a new interface to get this functionality.

Furthermore we should always return the information, as it is valuable even outside of the checkpoint/restart context.

I am also concerned that you appear to be building an interface for use by checkpoint/restart that makes it impossible checkpoint/restart the programs using that interface. The reason is that you appear to be putting this nebulous id into a global namespace and as such even if we wanted to I don't see how we could build a version where we could restore the id during a restart. And the thing is if you start building interfaces with identifiers you can not possibly restore I expect you will find you have painted yourself into a corner.

Using inode from stat avoids painting yourself into a corner because you have the possibility of different mounts with different device numbers having different inode numbers.

Quoted 6 lines

For the short term I don't see value in being able to restore the object identifiers, but I do see a lot of value in allowing for a future where nested checkpoint/restart is an option.

Eric

Hi Eric, thanks a lot for comments! I must admit I never though about nested checkpoint/restore simply because even plain and direct CR still has a number of problems which are not yet addressed.

As to return such ID in ino field (if I understand you right -- you propose to return such ID as inode of kstat structure) -- I don't think it would be right either. Instead of one iteface applied to all objects we export there will be a few different approaches instead -- for net-ns it would be dev+ino, for tasks and other members of task-structure it'll be IDs from /proc (as implemented in another patches). I like more Kyle's idea about object_id() call which would simply return the entrypted ID to user-space and it'll be up to user-space to do anything it wants with such pieces of information.

Yes, there will be no way to restore such IDs later but the interface is not supposed to work this way.

All this mess only because of lack
of way to figure out which task resources are shared and which are not. Maybe if we can carry CLONE_ flags from copy_process()/unshare()/setns() (and which else modify task resources?) inside task_struct and provide these flags back to user-space we might not need the IDs helpers at all. But I think such approach might end up in a pretty big patch bloating the kernel. In turn I wanted to bring as minimum new functionality as possible *with* a way to completely turn it off if user don't need it.

Quoted 5 lines

Hi Eric, thanks a lot for comments! I must admit I never though about nested checkpoint/restore simply because even plain and direct CR still has a number of problems which are not yet addressed.

Quoted 9 lines

As to return such ID in ino field (if I understand you right -- you propose to return such ID as inode of kstat structure) -- I don't think it would be right either. Instead of one iteface applied to all objects we export there will be a few different approaches instead -- for net-ns it would be dev+ino, for tasks and other members of task-structure it'll be IDs from /proc (as implemented in another patches). I like more Kyle's idea about object_id() call which would simply return the entrypted ID to user-space and it'll be up to user-space to do anything it wants with such pieces of information.

Right now everything thing that is exported is dev+ino. My objection is that you are adding yet another interface to get that information.

I already have patches that already implement dev+ino for the namespaces so I fully expect that to happen independently of your patches. My priority is to get the rest of the namespaces exported which requires a bit more review.

Quoted 2 lines

Yes, there will be no way to restore such IDs later but the interface is not supposed to work this way.

It sounds like it won't be possible to retrofit the ability to restore the IDs later. If the path to what will be needed to support nested checkpoint/restore is not clear the user space interface is broken by design. And since it is broken by design I say the design needs to bake more before we think of baking it.

Quoted 8 lines

All this mess only because of lack
of way to figure out which task resources are shared and which are not. Maybe if we can carry CLONE_ flags from copy_process()/unshare()/setns() (and which else modify task resources?) inside task_struct and provide these flags back to user-space we might not need the IDs helpers at all. But I think such approach might end up in a pretty big patch bloating the kernel. In turn I wanted to bring as minimum new functionality as possible *with* a way to completely turn it off if user don't need it.

The tricky case is file descriptors and file descriptors can be passed over unix domain sockets in arbitrary ways.

If you can find a way to do this without id helpers that sounds like a good design.

I have a nasty feeling that by trying to do this piecemeal instead of in one big system call you are slowly painting yourself into a corner from which you can not get out.