Re: [RFC v1 05/17] security/seccomp: Add LSM and create arrays of syscall metadata

On 24/03/2016 17:01, Casey Schaufler wrote:

On 3/23/2016 6:46 PM, Mickaël Salaün wrote:

quoted

diff --git a/security/seccomp/lsm.c b/security/seccomp/lsm.c
new file mode 100644
index 000000000000..93c881724341
--- /dev/null
+++ b/security/seccomp/lsm.c

@@ -0,0 +1,87 @@
+/*
+ * Seccomp Linux Security Module
+ *
+ * Copyright (C) 2016  Mickaël Salaün <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2, as
+ * published by the Free Software Foundation.
+ */
+
+#include <asm/syscall.h>	/* sys_call_table */
+#include <linux/compat.h>
+#include <linux/slab.h>	/* kcalloc() */
+#include <linux/syscalls.h>	/* syscall_argdesc */
+
+#include "lsm.h"
+
+/* TODO: Remove the need for CONFIG_SYSFS dependency */
+
+struct syscall_argdesc (*seccomp_syscalls_argdesc)[] = NULL;
+#ifdef CONFIG_COMPAT
+struct syscall_argdesc (*compat_seccomp_syscalls_argdesc)[] = NULL;
+#endif	/* CONFIG_COMPAT */
+
+static const struct syscall_argdesc *__init
+find_syscall_argdesc(const struct syscall_argdesc *start,
+		const struct syscall_argdesc *stop, const void *addr)
+{
+	if (unlikely(!addr || !start || !stop)) {
+		WARN_ON(1);
+		return NULL;
+	}
+
+	for (; start < stop; start++) {
+		if (start->addr == addr)
+			return start;
+	}
+	return NULL;
+}
+
+static inline void __init init_argdesc(void)
+{
+	const struct syscall_argdesc *argdesc;
+	const void *addr;
+	int i;
+
+	seccomp_syscalls_argdesc = kcalloc(NR_syscalls,
+			sizeof((*seccomp_syscalls_argdesc)[0]), GFP_KERNEL);
+	if (unlikely(!seccomp_syscalls_argdesc)) {
+		WARN_ON(1);
+		return;
+	}
+	for (i = 0; i < NR_syscalls; i++) {
+		addr = sys_call_table[i];
+		argdesc = find_syscall_argdesc(__start_syscalls_argdesc,
+				__stop_syscalls_argdesc, addr);
+		if (!argdesc)
+			continue;
+
+		(*seccomp_syscalls_argdesc)[i] = *argdesc;
+	}
+
+#ifdef CONFIG_COMPAT
+	compat_seccomp_syscalls_argdesc = kcalloc(IA32_NR_syscalls,
+			sizeof((*compat_seccomp_syscalls_argdesc)[0]),
+			GFP_KERNEL);
+	if (unlikely(!compat_seccomp_syscalls_argdesc)) {
+		WARN_ON(1);
+		return;
+	}
+	for (i = 0; i < IA32_NR_syscalls; i++) {
+		addr = ia32_sys_call_table[i];
+		argdesc = find_syscall_argdesc(__start_compat_syscalls_argdesc,
+				__stop_compat_syscalls_argdesc, addr);
+		if (!argdesc)
+			continue;
+
+		(*compat_seccomp_syscalls_argdesc)[i] = *argdesc;
+	}
+#endif	/* CONFIG_COMPAT */
+}
+
+void __init seccomp_init(void)
+{
+	pr_info("seccomp: Becoming ready for sandboxing\n");
+	init_argdesc();
+}

This isn't using the LSM infrastructure at all, is it?
It looks like the only reason you're calling it a security
module is to get the initialization code called in
security_init().

Let me amend my previous comment, which was to change
the name of seccomp_init(). Leave it as is, but add a
comment before it that explains why you've put the
call in the midst of the security module initialization.

The patch "[RFC v1 16/17] security/seccomp: Protect against filesystem TOCTOU" add LSM hooks, so it make sense to follow your first comment and rename seccomp_init() to seccomp_add_hooks().

 Mickaël