A pile in c/r sake

kernel/kcmp.c new file +20 -0

+
+/*
+ * We don't expose real in-memory order of objects for security
+ * reasons, still the comparision results should be suitable for
+ * sorting. Thus, we obfuscate kernel pointers values (using random
+ * cookies obtaned at early boot stage) and compare the production
+ * instead.
+ */
+static unsigned long cookies[KCMP_TYPES][2] __read_mostly;
+
+static long kptr_obfuscate(long v, int type)
+{
+	return (v ^ cookies[type][0]) * cookies[type][1];
+}
+
+/*
+ * 0 - equal
+ * 1 - less than
+ * 2 - greater than
+ * 3 - not equal but ordering unavailable (reserved for future)

+ */

kernel/kcmp.c line 43

Quoted 9 lines

kernel/kcmp.c new file +9 -0

+static int kcmp_ptr(long v1, long v2, enum kcmp_type type)
+{
+	long ret;
+
+	ret = kptr_obfuscate(v1, type) - kptr_obfuscate(v2, type);
+
+	return (ret < 0) | ((ret > 0) << 1);
+}
+

I just want to point out that we could do hard cryptography, too -- using DES or AES and compare the result since symmetric cryptography is an isomorphism. One would have to compare the whole result, obviously, not a truncated one, so using memcmp() or the similar.

I'll leave it up to the security guys to worry about if that is needed, but since it is something that can be slotted in without changing the API it seems reasonably safe.

It takes up to 5 agruments - the pids of the two tasks (which characteristics should be compared), the comparision type and (in case of comparision of files) two file descriptors.

Quoted 3 lines

It takes up to 5 agruments - the pids of the two tasks (which characteristics should be compared), the comparision type and (in case of comparision of files) two file descriptors.

Can you please write a manpage for it? That's really required to evaluate the interface properly.

As I understand it every time the kernel adds some new kind of state this would need to be extended too? This would seem like a lot of work, especially since you always need to synchronize kernel/user space. How would the user space break if it doesn't know about some newly added state?

Maybe it would be better to put more of the relevant code into the kernel to encapsulate this better.

kernel/kcmp.c line 149

Quoted 5 lines

kernel/kcmp.c new file +5 -0

+	case KCMP_SYSVSEM:
+#ifdef CONFIG_SYSVIPC
+		ret = kcmp_ptr((long)task1->sysvsem.undo_list,
+			       (long)task2->sysvsem.undo_list,
+			       KCMP_SYSVSEM);

I assume that's normally NULL.

tools/testing/selftests/kcmp/kcmp_test.c line 25

Quoted 4 lines

tools/testing/selftests/kcmp/kcmp_test.c new file +4 -0

+#ifdef CONFIG_X86_64
+static long syscall5(int nr, unsigned long arg0, unsigned long arg1,
+		unsigned long arg2, unsigned long arg3,
+		unsigned long arg4)

Why not just use syscall() in glibc?

Sure, I'll try to (btw, where I should send it to? And in which format novadays mans are written? In plain old troff or some human readable asciidocs?) And... should I post man page on LKML as well?

Quoted 6 lines · Cyrill Gorcunov

Sure, I'll try to (btw, where I should send it to? And in which format novadays mans are written? In plain old troff or some human readable asciidocs?) And... should I post man page on LKML as well?

Normally they're written using the mandoc macro set in *roff.  Michael Kerrisk (mtk) is the authority on how to do things.

Quoted 8 lines · H. Peter Anvin

Quoted 6 lines · Cyrill Gorcunov

Sure, I'll try to (btw, where I should send it to? And in which format novadays mans are written? In plain old troff or some human readable asciidocs?) And... should I post man page on LKML as well?

Quoted 2 lines

Normally they're written using the mandoc macro set in *roff.  Michael Kerrisk (mtk) is the authority on how to do things.

Cyrill, see http://www.kernel.org/doc/man-pages/patches.html and the following man pages
man-pages(7)
man(7)
groff_man(7)

Hi Michael, thanks! I've found this link yesterday night. Then I tried to clone man-pages but for some reason it always fail

 | [cyrill@moon projects]$ git clone git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
 | Cloning into 'man-pages'...
 | fatal: The remote end hung up unexpectedly
 | [cyrill@moon projects]$

maybe there another mirror I could fetch this repo?

Sure, I'll try to (btw, where I should send it to? And in which format novadays mans are written? In plain old troff or some human readable asciidocs?) And... should I post man page on LKML as well?

Quoted 7 lines

As I understand it every time the kernel adds some new kind of state this would need to be extended too? This would seem like a lot of work, especially since you always need to synchronize kernel/user space. How would the user space break if it doesn't know about some newly added state?

Wait, maybe I should use kernel-doc here and put comments with example right on top of SYSCALL definition?

Quoted 7 lines

kernel/kcmp.c line 149

Quoted 4 lines

kernel/kcmp.c new file +4 -0

+#ifdef CONFIG_SYSVIPC
+		ret = kcmp_ptr((long)task1->sysvsem.undo_list,
+			       (long)task2->sysvsem.undo_list,
+			       KCMP_SYSVSEM);

I assume that's normally NULL.

Hmm.. Andi, I seem not following. And?

Quoted 4 lines · Cyrill Gorcunov

Sure, I'll try to (btw, where I should send it to? And in which format novadays mans are written? In plain old troff or some human readable asciidocs?) And... should I post man page on LKML as well?

Manpages are written in troff with -man and you would add the ASCII output of the manpage to the commit log and send the troff source at some point to linux-man.

The basic problem is if this interface is at the right level of abstraction. I have some doubts on that. It seems like a long term maintenance nightmare to me. It may be better to put the loop that would call this into the kernel.

Quoted 2 lines

Hmm.. Andi, I seem not following. And?

It just doesn't seem like a very useful thing to compare.

Quoted 4 lines

The basic problem is if this interface is at the right level of abstraction. I have some doubts on that. It seems like a long term maintenance nightmare to me. It may be better to put the loop that would call this into the kernel.

Hmm, ie selftest right in kenel?

Quoted 5 lines

Quoted 4 lines

The basic problem is if this interface is at the right level of abstraction. I have some doubts on that. It seems like a long term maintenance nightmare to me. It may be better to put the loop that would call this into the kernel.

Quoted 1 line

Hmm, ie selftest right in kenel?

Not testing, but more the general stability of the interface. IMHO it exposes too many kernel internals. I know they are already exposed by clone/unshare, but in those nothing breaks if the user program doesn't know about some new flags. But this looks like the user always has to be updated for every change. I think I would prefer if more of the user was in kernel to not expose that much.

kernel/kcmp.c new file +12 -0

+/*
+ * We don't expose real in-memory order of objects for security
+ * reasons, still the comparision results should be suitable for
+ * sorting. Thus, we obfuscate kernel pointers values (using random
+ * cookies obtaned at early boot stage) and compare the production
+ * instead.
+ */
+static unsigned long cookies[KCMP_TYPES][2] __read_mostly;
+
+static long kptr_obfuscate(long v, int type)
+{
+	return (v ^ cookies[type][0]) * cookies[type][1];

kernel/kcmp.c new file +11 -0

+	long ret;
+
+	ret = kptr_obfuscate(v1, type) - kptr_obfuscate(v2, type);
+
+	return (ret < 0) | ((ret > 0) << 1);
+}
+
+/* The caller must have pinned the task */
+static struct file *
+get_file_raw_ptr(struct task_struct *task, unsigned int idx)
+{

Can you make the prototype:
static int kcmp_ptr(void *v1, void *v2, enum kcmp_type type)

So the caller does not need to have casts?

I believe I commented on this earlier but it looks like it got lost in the noise.

file == NULL != file == NULL.

When you can't lookup the file kcmp should return -EBADF.

I believe I commented on this earlier but it looks like it got lost in the noise.

file == NULL != file == NULL.

When you can't lookup the file kcmp should return -EBADF.

Hi Cyrill,

kernel/kcmp.c line 26

Quoted 12 lines · Cyrill Gorcunov

kernel/kcmp.c new file +12 -0

+/*
+ * We don't expose real in-memory order of objects for security
+ * reasons, still the comparision results should be suitable for
+ * sorting. Thus, we obfuscate kernel pointers values (using random
+ * cookies obtaned at early boot stage) and compare the production
+ * instead.
+ */
+static unsigned long cookies[KCMP_TYPES][2] __read_mostly;
+
+static long kptr_obfuscate(long v, int type)
+{
+	return (v ^ cookies[type][0]) * cookies[type][1];

AFACS, cookies is fully random value, is it possible that

((v1 ^ cookies[type][0]) * cookies[type][1] == (v2 ^ cookies[type][0])
* cookies[type][1]) &&
(v1 != v2)

for too round cookies[type][1]?

Thanks,

Signature · 3 lines

Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing
environments

No, cookies[1] is always odd.

While doing the checkpoint-restore in the userspace one need to determine whether various kernel objects (like mm_struct-s of file_struct-s) are shared between tasks and restore this state.

The 2nd step can be solved by using appropriate CLONE_ flags and the unshare syscall, while there's currently no ways for solving the 1st one.

One of the ways for checking whether two tasks share e.g. mm_struct is to provide some mm_struct ID of a task to its proc file, but showing such info considered to be not that good for security reasons.

Thus after some debates we end up in conclusion that using that named 'comparision' syscall might be the best candidate. So here is it -- __NR_kcmp.

It takes up to 5 agruments - the pids of the two tasks (which characteristics should be compared), the comparision type and (in case of comparision of files) two file descriptors.

Lookups for pids are done in the caller's PID namespace only.

At moment only x86 is supported and tested.

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

CC "Eric W. Biederman" <[email protected]>

CC Pavel Emelyanov <[email protected]>

CC Andrey Vagin <[email protected]>

CC KOSAKI Motohiro <[email protected]>

CC Ingo Molnar <[email protected]>

CC H. Peter Anvin <[email protected]>

CC Thomas Gleixner <[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]

---
 arch/x86/syscalls/syscall_32.tbl         |    1 
 arch/x86/syscalls/syscall_64.tbl         |    1 
 include/linux/kcmp.h                     |   17 ++
 include/linux/syscalls.h                 |    2 
 kernel/Makefile                          |    1 
 kernel/kcmp.c                            |  181 +++++++++++++++++++++++++++++++
 tools/testing/selftests/kcmp/Makefile    |   35 +++++
 tools/testing/selftests/kcmp/kcmp_test.c |  126 +++++++++++++++++++++
 tools/testing/selftests/run_tests        |    2 
 9 files changed, 365 insertions(+), 1 deletion(-)

Index: linux-2.6.git/arch/x86/syscalls/syscall_32.tbl
===================================================================

arch/x86/syscalls/syscall_32.tbl +1 -0

@@ -355,3 +355,4 @@
 346	i386	setns			sys_setns
 347	i386	process_vm_readv	sys_process_vm_readv		compat_sys_process_vm_readv
 348	i386	process_vm_writev	sys_process_vm_writev		compat_sys_process_vm_writev
+349	i386	kcmp			sys_kcmp

Index: linux-2.6.git/arch/x86/syscalls/syscall_64.tbl
===================================================================

arch/x86/syscalls/syscall_64.tbl +1 -0

@@ -318,3 +318,4 @@
 309	64	getcpu			sys_getcpu
 310	64	process_vm_readv	sys_process_vm_readv
 311	64	process_vm_writev	sys_process_vm_writev
+312	64	kcmp			sys_kcmp

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

include/linux/kcmp.h new file +17 -0

@@ -0,0 +1,17 @@
+#ifndef _LINUX_KCMP_H
+#define _LINUX_KCMP_H
+
+/* Comparision type */
+enum kcmp_type {
+	KCMP_FILE,
+	KCMP_VM,
+	KCMP_FILES,
+	KCMP_FS,
+	KCMP_SIGHAND,
+	KCMP_IO,
+	KCMP_SYSVSEM,
+
+	KCMP_TYPES,
+};
+
+#endif /* _LINUX_KCMP_H */

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

include/linux/syscalls.h +2 -0

@@ -857,4 +857,6 @@ asmlinkage long sys_process_vm_writev(pi
 				      unsigned long riovcnt,
 				      unsigned long flags);
 
+asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
+			 unsigned long idx1, unsigned long idx2);
 #endif

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

kernel/Makefile +1 -0

@@ -25,6 +25,7 @@ endif
 obj-y += sched/
 obj-y += power/
 
+obj-$(CONFIG_X86) += kcmp.o
 obj-$(CONFIG_FREEZER) += freezer.o
 obj-$(CONFIG_PROFILING) += profile.o
 obj-$(CONFIG_SYSCTL_SYSCALL_CHECK) += sysctl_check.o

Index: linux-2.6.git/kernel/kcmp.c
===================================================================

kernel/kcmp.c new file +107 -0

@@ -0,0 +1,181 @@
+#include <linux/kernel.h>
+#include <linux/syscalls.h>
+#include <linux/fdtable.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/kcmp.h>
+
+#include <asm/unistd.h>
+
+/*
+ * We don't expose real in-memory order of objects for security
+ * reasons, still the comparision results should be suitable for
+ * sorting. Thus, we obfuscate kernel pointers values (using random
+ * cookies obtaned at early boot stage) and compare the production
+ * instead.
+ */
+static unsigned long cookies[KCMP_TYPES][2] __read_mostly;
+
+static long kptr_obfuscate(long v, int type)
+{
+	return (v ^ cookies[type][0]) * cookies[type][1];
+}
+
+/*
+ * 0 - equal
+ * 1 - less than
+ * 2 - greater than
+ * 3 - not equal but ordering unavailable (reserved for future)
+ */
+static int kcmp_ptr(long v1, long v2, enum kcmp_type type)
+{
+	long ret;
+
+	ret = kptr_obfuscate(v1, type) - kptr_obfuscate(v2, type);
+
+	return (ret < 0) | ((ret > 0) << 1);
+}
+
+/* The caller must have pinned the task */
+static struct file *
+get_file_raw_ptr(struct task_struct *task, unsigned int idx)
+{
+	struct fdtable *fdt;
+	struct file *file;
+
+	spin_lock(&task->files->file_lock);
+	fdt = files_fdtable(task->files);
+	if (idx < fdt->max_fds)
+		file = fdt->fd[idx];
+	else
+		file = NULL;
+	spin_unlock(&task->files->file_lock);
+
+	return file;
+}
+
+SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
+		unsigned long, idx1, unsigned long, idx2)
+{
+	struct task_struct *task1, *task2;
+	int ret;
+
+	rcu_read_lock();
+
+	/*
+	 * Tasks are looked up in caller's
+	 * PID namespace only.
+	 */
+
+	task1 = find_task_by_vpid(pid1);
+	if (!task1) {
+		rcu_read_unlock();
+		return -ESRCH;
+	}
+
+	task2 = find_task_by_vpid(pid2);
+	if (!task2) {
+		put_task_struct(task1);
+		rcu_read_unlock();
+		return -ESRCH;
+	}
+
+	get_task_struct(task1);
+	get_task_struct(task2);
+
+	rcu_read_unlock();
+
+	/*
+	 * One should have enough rights to inspect tasks details.
+	 */
+	if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
+	    !ptrace_may_access(task2, PTRACE_MODE_READ)) {
+		ret = -EACCES;
+		goto err;
+	}
+
+	/*
+	 * Note for all cases but the KCMP_FILE we
+	 * don't take any locks in a sake of speed.
+	 */
+
+	switch (type) {

kernel/kcmp.c new file +24 -0

+		break;
+	}
+	case KCMP_VM:
+		ret = kcmp_ptr((long)task1->mm,
+			       (long)task2->mm,
+			       KCMP_VM);
+		break;
+	case KCMP_FILES:
+		ret = kcmp_ptr((long)task1->files,
+			       (long)task2->files,
+			       KCMP_FILES);
+		break;
+	case KCMP_FS:
+		ret = kcmp_ptr((long)task1->fs,
+			       (long)task2->fs,
+			       KCMP_FS);
+		break;
+	case KCMP_SIGHAND:
+		ret = kcmp_ptr((long)task1->sighand,
+			       (long)task2->sighand,
+			       KCMP_SIGHAND);
+		break;
+	case KCMP_IO:
+		ret = kcmp_ptr((long)task1->io_context,

kernel/kcmp.c new file +29 -0

+#endif
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
+	}
+
+err:
+	put_task_struct(task1);
+	put_task_struct(task2);
+
+	return ret;
+}
+
+static __init int kcmp_cookie_init(void)
+{
+	int i, j;
+
+	for (i = 0; i < KCMP_TYPES; i++) {
+		for (j = 0; j < 2; j++) {
+			get_random_bytes(&cookies[i][j],
+					 sizeof(cookies[i][j]));
+		}
+		cookies[i][1] |= (~(~0UL >>  1) | 1);
+	}
+
+	return 0;
+}
+late_initcall(kcmp_cookie_init);

Index: linux-2.6.git/tools/testing/selftests/kcmp/Makefile
===================================================================

tools/testing/selftests/kcmp/Makefile new file +35 -0

@@ -0,0 +1,35 @@
+ifeq ($(strip $(V)),)
+	E = @echo
+	Q = @
+else
+	E = @\#
+	Q =
+endif
+export E Q
+
+uname_M := $(shell uname -m 2>/dev/null || echo not)
+ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/i386/)
+ifeq ($(ARCH),i386)
+        ARCH := X86
+	CFLAGS := -DCONFIG_X86_32
+endif
+ifeq ($(ARCH),x86_64)
+	ARCH := X86
+	CFLAGS := -DCONFIG_X86_64
+endif
+
+CFLAGS += -I../../../../arch/x86/include/generated/
+CFLAGS += -I../../../../include/
+
+all:
+ifeq ($(ARCH),X86)
+	$(E) "  CC run_test"
+	$(Q) gcc $(CFLAGS) kcmp_test.c -o run_test
+else
+	$(E) "Not an x86 target, can't build breakpoints selftests"
+endif
+
+clean:
+	$(E) "  CLEAN"
+	$(Q) rm -fr ./run_test
+	$(Q) rm -fr ./test-file

Index: linux-2.6.git/tools/testing/selftests/kcmp/kcmp_test.c
===================================================================

tools/testing/selftests/kcmp/kcmp_test.c new file +100 -0

@@ -0,0 +1,126 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <signal.h>
+#include <limits.h>
+#include <unistd.h>
+#include <errno.h>
+#include <string.h>
+#include <fcntl.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+
+#ifdef CONFIG_X86_64
+#include "asm/unistd_64.h"
+#else
+#include "asm/unistd_32.h"
+#endif
+
+#include "linux/kcmp.h"
+
+#ifdef CONFIG_X86_64
+static long syscall5(int nr, unsigned long arg0, unsigned long arg1,
+		unsigned long arg2, unsigned long arg3,
+		unsigned long arg4)
+{
+	long ret;
+	asm volatile(
+		"movl %1, %%eax		\n"
+		"movq %2, %%rdi		\n"
+		"movq %3, %%rsi		\n"
+		"movq %4, %%rdx		\n"
+		"movq %5, %%r10		\n"
+		"movq %6, %%r8		\n"
+		"syscall		\n"
+		"movq %%rax, %0		\n"
+		: "=r"(ret)
+		: "g" ((int)nr), "g" (arg0), "g" (arg1), "g" (arg2),
+			"g" (arg3), "g" (arg4)
+		: "rax", "rdi", "rsi", "rdx", "r10", "r8", "memory");
+	return ret;
+}
+#else
+static long syscall5(int nr, unsigned long arg0, unsigned long arg1,
+		unsigned long arg2, unsigned long arg3,
+		unsigned long arg4)
+{
+	long ret;
+	asm volatile(
+		"movl %1, %%eax		\n"
+		"movl %2, %%ebx		\n"
+		"movl %3, %%ecx		\n"
+		"movl %4, %%edx		\n"
+		"movl %5, %%esi		\n"
+		"movl %6, %%edi		\n"
+		"intl $0x80		\n"
+		"movq %%eax, %0		\n"
+		: "=r"(ret)
+		: "g" ((int)nr), "g" (arg0), "g" (arg1), "g" (arg2),
+			"g" (arg3), "g" (arg4)
+		: "eax", "ebx", "ecx", "edx", "esi", "edi", "memory");
+	return ret;
+}
+#endif
+
+static long sys_kcmp(int pid1, int pid2, int type, int fd1, int fd2)
+{
+	return syscall5(__NR_kcmp, (long)pid1, (long)pid2,
+			(long)type, (long)fd1, (long)fd2);
+}
+
+int main(int argc, char **argv)
+{
+	int pid1, pid2;
+	int fd1, fd2;
+	int status;
+
+	fd1 = open("test-file", O_RDWR | O_CREAT | O_TRUNC, 0644);
+	pid1 = getpid();
+
+	if (fd1 < 0) {
+		perror("Can't create file");
+		exit(1);
+	}
+
+	pid2 = fork();
+	if (pid2 < 0) {
+		perror("fork failed");
+		exit(1);
+	}
+
+	if (!pid2) {
+		int pid2 = getpid();
+
+		fd2 = open("test-file", O_RDWR, 0644);
+		if (fd2 < 0) {
+			perror("Can't open file");
+			exit(1);
+		}
+

tools/testing/selftests/kcmp/kcmp_test.c new file +16 -0

+
+			/* This one should fail */
+		       sys_kcmp(pid1, pid2, KCMP_TYPES + 1,	0, 0));
+
+		/* This one should return same fd */
+		printf("pid1: %6d pid2: %6d SAME-FD: %2d\n",
+		       pid1, pid2,
+		       sys_kcmp(pid1, pid2, KCMP_FILE,		fd1, fd1));
+
+		exit(0);
+	}
+
+	waitpid(pid2, &status, P_ALL);
+
+	return 0;
+}

Index: linux-2.6.git/tools/testing/selftests/run_tests
===================================================================

tools/testing/selftests/run_tests +1 -1

@@ -1,6 +1,6 @@
 #!/bin/bash
 
-TARGETS=breakpoints
+TARGETS="breakpoints kcmp"
 
 for TARGET in $TARGETS
 do

kernel/kcmp.c line 117

Quoted 11 lines · Cyrill Gorcunov

kernel/kcmp.c new file +11 -0

+	switch (type) {
+	case KCMP_FILE: {
+		struct file *filp1, *filp2;
+
+		filp1 = get_file_raw_ptr(task1, idx1);
+		filp2 = get_file_raw_ptr(task2, idx2);
+
+		if (filp1 && filp2)
+			ret = kcmp_ptr((long)filp1, (long)filp2, KCMP_FILE);
+		else
+			ret = -ENOENT;

If my remember is correct, Andrew pointed out EINVAL is better than ENOENT.

Quoted 4 lines

kernel/kcmp.c new file +4 -0

+		break;
+	case KCMP_SYSVSEM:
+#ifdef CONFIG_SYSVIPC
+		ret = kcmp_ptr((long)task1->sysvsem.undo_list,

kernel/kcmp.c line 151

Quoted 4 lines

kernel/kcmp.c new file +4 -0

+			       (long)task2->sysvsem.undo_list,
+			       KCMP_SYSVSEM);
+#else
+		ret = -EINVAL;

ENOTSUP is better, I think. because of, EINVAL implicitly mean _caller_ is wrong. but in this case, it is not bad. only the kernel doesn't have enough feature.

Review

Quoted 2 lines

(quoted diff) +1 -0

+		goto err;

you don't need err label at all.

kernel/kcmp.c line 165

Quoted 13 lines

kernel/kcmp.c new file +13 -0

+#endif
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
+	}
+
+err:
+	put_task_struct(task1);
+	put_task_struct(task2);
+
+	return ret;
+}

Quoted 5 lines · KOSAKI Motohiro

kernel/kcmp.c line 117

Quoted 3 lines

kernel/kcmp.c new file +3 -0

+			ret = kcmp_ptr((long)filp1, (long)filp2, KCMP_FILE);
+		else
+			ret = -ENOENT;

If my remember is correct, Andrew pointed out EINVAL is better than ENOENT.

Ah yes. And really what it should be is

		if (!filp1 || !filp2)

return -EBADF;

At least EBADF is what you return if it is your process that doesn't have the filedescriptor.

Quoted 7 lines

kernel/kcmp.c line 151

Quoted 3 lines

kernel/kcmp.c new file +3 -0

+			       KCMP_SYSVSEM);
+#else
+		ret = -EINVAL;

ENOTSUP is better, I think. because of, EINVAL implicitly mean _caller_ is wrong. but in this case, it is not bad. only the kernel doesn't have enough feature.

Careful a type compiled out should in principle match a type whose support has not been implemented. That is the default case should match what happens when you don't compile in sysvipc support.

Quoted 7 lines · Eric W. Biederman

Quoted 5 lines · KOSAKI Motohiro

kernel/kcmp.c line 117

Quoted 3 lines

kernel/kcmp.c new file +3 -0

+			ret = kcmp_ptr((long)filp1, (long)filp2, KCMP_FILE);
+		else
+			ret = -ENOENT;

If my remember is correct, Andrew pointed out EINVAL is better than ENOENT.

Quoted 7 lines

Ah yes. And really what it should be is

		if (!filp1 || !filp2)

return -EBADF;

At least EBADF is what you return if it is your process that doesn't have the filedescriptor.

Eric, I've sent out version with

		if (filp1 && filp2)
			...
		else
			ret = -EBADF;

maybe you're lookin into previous version?

Quoted 8 lines

Quoted 7 lines

kernel/kcmp.c line 151

Quoted 3 lines

kernel/kcmp.c new file +3 -0

+			       KCMP_SYSVSEM);
+#else
+		ret = -EINVAL;

ENOTSUP is better, I think. because of, EINVAL implicitly mean _caller_ is wrong. but in this case, it is not bad. only the kernel doesn't have enough feature.

Quoted 3 lines

Careful a type compiled out should in principle match a type whose support has not been implemented. That is the default case should match what happens when you don't compile in sysvipc support.

I don't get it :) Will -EINVAL be enough or not?

kernel/kcmp.c line 151

Quoted 7 lines

kernel/kcmp.c new file +7 -0

+	case KCMP_SYSVSEM:
+#ifdef CONFIG_SYSVIPC
+		ret = kcmp_ptr((long)task1->sysvsem.undo_list,
+			       (long)task2->sysvsem.undo_list,
+			       KCMP_SYSVSEM);
+#else
+		ret = -EINVAL;

ENOTSUP is better, I think. because of, EINVAL implicitly mean _caller_ is wrong. but in this case, it is not bad. only the kernel doesn't have enough feature.

Quoted 11 lines

kernel/kcmp.c line 151

Quoted 7 lines

kernel/kcmp.c new file +7 -0

+	case KCMP_SYSVSEM:
+#ifdef CONFIG_SYSVIPC
+		ret = kcmp_ptr((long)task1->sysvsem.undo_list,
+			       (long)task2->sysvsem.undo_list,
+			       KCMP_SYSVSEM);
+#else
+		ret = -EINVAL;

ENOTSUP is better, I think. because of, EINVAL implicitly mean _caller_ is wrong. but in this case, it is not bad. only the kernel doesn't have enough feature.

Isn't it usually the case for ENOSYS, then ?

kernel/kcmp.c line 117

Quoted 5 lines

kernel/kcmp.c new file +5 -0

+
+		if (filp1 && filp2)
+			ret = kcmp_ptr((long)filp1, (long)filp2, KCMP_FILE);
+		else
+			ret = -ENOENT;

If my remember is correct, Andrew pointed out EINVAL is better than ENOENT.

kernel/kcmp.c line 151

Quoted 6 lines

kernel/kcmp.c new file +6 -0

+#ifdef CONFIG_SYSVIPC
+		ret = kcmp_ptr((long)task1->sysvsem.undo_list,
+			       (long)task2->sysvsem.undo_list,
+			       KCMP_SYSVSEM);
+#else
+		ret = -EINVAL;

ENOTSUP is better, I think. because of, EINVAL implicitly mean _caller_ is wrong. but in this case, it is not bad. only the kernel doesn't have enough feature.

Review

Quoted 2 lines

(quoted diff) +1 -0

+		goto err;

you don't need err label at all.

tools/testing/selftests/kcmp/kcmp_test.c

Quoted 11 lines

tools/testing/selftests/kcmp/kcmp_test.c new file +11 -0

+
+                printf("pid1: %6d pid2: %6d FD: %2d FILES: %2d VM: %2d FS: %2d "
+		       "SIGHAND: %2d IO: %2d SYSVSEM: %2d INV: %2d\n",
+		       pid1, pid2,
+		       sys_kcmp(pid1, pid2, KCMP_FILE,		fd1, fd2),
+		       sys_kcmp(pid1, pid2, KCMP_FILES,		0, 0),
+		       sys_kcmp(pid1, pid2, KCMP_VM,		0, 0),
+		       sys_kcmp(pid1, pid2, KCMP_FS,		0, 0),
+		       sys_kcmp(pid1, pid2, KCMP_SIGHAND,	0, 0),
+		       sys_kcmp(pid1, pid2, KCMP_IO,		0, 0),
+		       sys_kcmp(pid1, pid2, KCMP_SYSVSEM,	0, 0),.

The best practice of auto test is

AssertFooBar(expected_value, actual_value);

and, just only print "correct or not". Only you know the correct value.

We would like to have an ability to restore command line arguments and envirion pointers so the task being restored would print appropriate values in /proc/pid/cmdline and /proc/pid/envirion. The exit_code is needed to restore zombie tasks.

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

+               if (mm && permitted) {
+                       seq_printf(m, "%lu %lu %lu %lu %lu %lu %lu ",

We would like to have an ability to restore command line arguments and envirion pointers so the task being restored would print appropriate values in /proc/pid/cmdline and /proc/pid/envirion. The exit_code is needed to restore zombie tasks.

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

Cc Pavel Emelyanov <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc Kees Cook <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Alexey Dobriyan <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Andrew Vagin <[email protected]>

Cc Vasiliy Kulikov <[email protected]>

Cc Alexey Dobriyan <[email protected]>

Cc "Eric W. Biederman" <[email protected]>

---
 Documentation/filesystems/proc.txt |    5 +++++
 fs/proc/array.c                    |   23 ++++++++++++++++++-----
 2 files changed, 23 insertions(+), 5 deletions(-)

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

Documentation/filesystems/proc.txt +5 -0

@@ -311,6 +311,11 @@ Table 1-4: Contents of the stat files (a
   start_data    address above which program data+bss is placed
   end_data      address below which program data+bss is placed
   start_brk     address above which program heap can be expanded with brk()
+  arg_start     address above which program command line is placed
+  arg_end       address below which program command line is placed
+  env_start     address above which program environment is placed
+  env_end       address below which program environment is placed
+  exit_code     the thread's exit_code in the form reported by the waitpid system call
..............................................................................
 
 The /proc/PID/maps file containing the currently mapped memory regions and

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

fs/proc/array.c +5 -5

@@ -464,7 +464,7 @@ static int do_task_stat(struct seq_file
 
 	seq_printf(m, "%d (%s) %c %d %d %d %d %d %u %lu \
 %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \
-%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld %lu %lu %lu\n",
+%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld ",
 		pid_nr_ns(pid, ns),
 		tcomm,
 		state,

@@ -511,10 +511,23 @@ static int do_task_stat(struct seq_file
 		task->policy,
 		(unsigned long long)delayacct_blkio_ticks(task),
 		cputime_to_clock_t(gtime),
-		cputime_to_clock_t(cgtime),
-		(mm && permitted) ? mm->start_data : 0,
-		(mm && permitted) ? mm->end_data : 0,
-		(mm && permitted) ? mm->start_brk : 0);
+		cputime_to_clock_t(cgtime));
+
+		if (mm && permitted) {
+			seq_printf(m, "%lu %lu %lu %lu %lu %lu %lu ",

fs/proc/array.c static int do_task_stat(struct seq_file · line 530

Quoted 4 lines

fs/proc/array.c +3 -0

+
+		seq_printf(m, "%d\n", task->exit_code);
+

Bad this part seems to make new side channel. exit_code is one of inter process communication messages. IPC messages should NOT be observed from completely unrelated proesses.

+               if (opt == PR_SET_MM_START_STACK)
+                       mm->start_stack = addr;
+               else if (opt == PR_SET_MM_ARG_START)
+                       mm->arg_start = addr;
+               else if (opt == PR_SET_MM_ARG_END)
+                       mm->arg_end = addr;
+               else if (opt == PR_SET_MM_ENV_START)
+                       mm->env_start = addr;
+               else if (opt == PR_SET_MM_ENV_END)
+                       mm->env_end = addr;
+               break;

Quoted 11 lines · Cyrill Gorcunov

+               if (opt == PR_SET_MM_START_STACK)
+                       mm->start_stack = addr;
+               else if (opt == PR_SET_MM_ARG_START)
+                       mm->arg_start = addr;
+               else if (opt == PR_SET_MM_ARG_END)
+                       mm->arg_end = addr;
+               else if (opt == PR_SET_MM_ENV_START)
+                       mm->env_start = addr;
+               else if (opt == PR_SET_MM_ENV_END)
+                       mm->env_end = addr;
+               break;

Why not a switch statement here? Not that it really matters. :)

Quoted 6 lines

kernel/sys.c +6 -0

+
+       case PR_SET_MM_AUXV:
+               if (arg4 > sizeof(mm->saved_auxv))
+                       goto out;
+               up_read(&mm->mmap_sem);
+

Quoted 5 lines

+               error = -EFAULT;
+               if (!copy_from_user(mm->saved_auxv, (const void __user *)addr, arg4))
+                       error = 0;
+
+               return error;

Is the mmap_sem released here because of the copy_from_user()? Is it still safe to write to saved_auxv?

Quoted 6 lines

Quoted 6 lines

kernel/sys.c +6 -0

+
+       case PR_SET_MM_AUXV:
+               if (arg4 > sizeof(mm->saved_auxv))
+                       goto out;
+               up_read(&mm->mmap_sem);
+

Quoted 9 lines

kernel/sys.c static int prctl_set_mm(int opt, unsigne · line 1814

Quoted 5 lines

kernel/sys.c +5 -0

+               error = -EFAULT;
+               if (!copy_from_user(mm->saved_auxv, (const void __user *)addr, arg4))
+                       error = 0;
+
+               return error;

Is the mmap_sem released here because of the copy_from_user()? Is it still safe to write to saved_auxv?

At moment I believe yes (if only I'm not missing something), we poke this vector at elf loading procedure, so it's up to user to sync access to "own" saved_auxv and write sane values inside.

Quoted 6 lines

kernel/sys.c +6 -0

+
+       case PR_SET_MM_AUXV:
+               if (arg4 > sizeof(mm->saved_auxv))
+                       goto out;
+               up_read(&mm->mmap_sem);
+

kernel/sys.c static int prctl_set_mm(int opt, unsigne · line 1814

Quoted 5 lines

kernel/sys.c +5 -0

+               error = -EFAULT;
+               if (!copy_from_user(mm->saved_auxv, (const void __user *)addr, arg4))
+                       error = 0;
+
+               return error;

Is the mmap_sem released here because of the copy_from_user()? Is it still safe to write to saved_auxv?