Re: [kernel-hardening] [PATCH v4] scripts: add leaking_addresses.pl

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 2199 bytes --]

On Tue, Nov 7, 2017 at 3:36 PM, Laura Abbott <labbott@redhat.com> wrote:
>
> I'd probably put /proc/kallsyms and /proc/modules on the omit list
> since those are designed to leak addresses to userspace.

Well, they are indeed designed to leak addresses, but not a lot of
people should care.

So I think we could tighten them up.

For example, maybe /proc/kallsyms could just default to not showing
values to non-root users.

We *did* originally try to use "kptr_restrict" with a default value of
1, it's just that it was never fixable on a case-by-case basis as
people started saying "that breaks my flow, because xyz".

But if we do it for one file at a time, we probably *can* try to fix complaints.

Something like the attached TOTALLY UNTESTED patch. It's meant more as
an RFC, not for application, but it's also meant to show how we can
tailor the behavior for specific workflow issues.

So take that "kallsyms_for_perf()" thing as an example of how we can
say "hey, if you already have access to kernel profiling anyway,
there's no point in hiding kallsyms".

And there may be other similar things we can do.

The situation with /proc/modules should be similar. Using
kptr_restrict was a big hammer and might have broken something
unrelated, but did anybody actually care about the particular case of
/proc/modules not showing the module address to normal users? probably
not. "lsmod" certainly doesn't care, and that's what people really
want.

Both /proc/kallsyms and /proc/modules _used_ to be really important
for oops reporting, but that was long ago when the kernel didn't
report symbol information of its own. So we have historical reasons
for people to be able to read those files, but those are mainly things
that aren't relevant (or even possible) on modern kernels anyway.

So I don'r think we should omit /proc/kallsyms and /proc/modules - we
should just fix them.

The attached patch may not be good enough as is, but maybe something
_like_ it will work well enough that people won't care?

(And do note the "TOTALLY UNTESTED". It seems to compile. But maybe I
got some test exactly the wrong way around and it doesn't actually
_work_. Caveat testor).

                     Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 2543 bytes --]

 kernel/kallsyms.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 127e7cfafa55..5b1299c1e4b0 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -480,6 +480,7 @@ struct kallsym_iter {
 	char name[KSYM_NAME_LEN];
 	char module_name[MODULE_NAME_LEN];
 	int exported;
+	int show_value;
 };
 
 static int get_ksymbol_mod(struct kallsym_iter *iter)
@@ -580,14 +581,23 @@ static void s_stop(struct seq_file *m, void *p)
 {
 }
 
+#ifndef CONFIG_64BIT
+# define KALLSYM_FMT "%08lx"
+#else
+# define KALLSYM_FMT "%016lx"
+#endif
+
 static int s_show(struct seq_file *m, void *p)
 {
+	unsigned long value;
 	struct kallsym_iter *iter = m->private;
 
 	/* Some debugging symbols have no name.  Ignore them. */
 	if (!iter->name[0])
 		return 0;
 
+	value = iter->show_value ? iter->value : 0;
+
 	if (iter->module_name[0]) {
 		char type;
 
@@ -597,10 +607,10 @@ static int s_show(struct seq_file *m, void *p)
 		 */
 		type = iter->exported ? toupper(iter->type) :
 					tolower(iter->type);
-		seq_printf(m, "%pK %c %s\t[%s]\n", (void *)iter->value,
+		seq_printf(m, KALLSYM_FMT " %c %s\t[%s]\n", value,
 			   type, iter->name, iter->module_name);
 	} else
-		seq_printf(m, "%pK %c %s\n", (void *)iter->value,
+		seq_printf(m, KALLSYM_FMT " %c %s\n", value,
 			   iter->type, iter->name);
 	return 0;
 }
@@ -612,6 +622,40 @@ static const struct seq_operations kallsyms_op = {
 	.show = s_show
 };
 
+static inline int kallsyms_for_perf(void)
+{
+#ifdef CONFIG_PERF_EVENTS
+	extern int sysctl_perf_event_paranoid;
+	if (sysctl_perf_event_paranoid <= 0)
+		return 1;
+#endif
+	return 0;
+}
+
+/*
+ * We show kallsyms information even to normal users if we've enabled
+ * kernel profiling and are explicitly not paranoid (so kptr_restrict
+ * is clear, and sysctl_perf_event_paranoid isn't set).
+ *
+ * Otherwise, require CAP_SYSLOG (assuming kptr_restrict isn't set to
+ * block even that).
+ */
+static int kallsyms_show_value(void)
+{
+	switch (kptr_restrict) {
+	case 0:
+		if (kallsyms_for_perf())
+			return 1;
+	/* fallthrough */
+	case 1:
+		if (has_capability_noaudit(current, CAP_SYSLOG))
+			return 1;
+	/* fallthrough */
+	default:
+		return 0;
+	}
+}
+
 static int kallsyms_open(struct inode *inode, struct file *file)
 {
 	/*
@@ -625,6 +669,7 @@ static int kallsyms_open(struct inode *inode, struct file *file)
 		return -ENOMEM;
 	reset_iter(iter, 0);
 
+	iter->show_value = kallsyms_show_value();
 	return 0;
 }