[PATCH 0/2] kallsyms: handle special absolute symbols

[PATCH 0/2] kallsyms: handle special absolute symbols

[Kees Cook]

This is an attempt at a more correct solution to the patch sent
earlier "kallsyms: fix absolute addresses for kASLR".

The main problem is that the entire per_cpu area needs to be treated 
as absolute by kallsyms. This series adds the logic to recognize the
per_cpu memory range and treat it as absolute in the kallsyms tool.

-Kees

[PATCH 0/2] kallsyms: handle special absolute symbols

[Kees Cook]

This is an attempt at a more correct solution to the patch sent
earlier "kallsyms: fix absolute addresses for kASLR".

The main problem is that the entire per_cpu area needs to be treated 
as absolute by kallsyms. This series adds the logic to recognize the
per_cpu memory range and treat it as absolute in the kallsyms tool.

-Kees

[PATCH 1/2] kallsyms: generalize address range checking

[Kees Cook]

This refactors the address range checks to be generalized instead of
specific to text range checks, in preparation for other range checks.
Also extracts logic for "is the symbol absolute" into a function.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/kallsyms.c |   53 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index 276e84b8a8e5..3c6224728960 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -36,13 +36,13 @@ struct sym_entry {
 	unsigned char *sym;
 };
 
-struct text_range {
-	const char *stext, *etext;
+struct addr_range {
+	const char *start_sym, *end_sym;
 	unsigned long long start, end;
 };
 
 static unsigned long long _text;
-static struct text_range text_ranges[] = {
+static struct addr_range text_ranges[] = {
 	{ "_stext",     "_etext"     },
 	{ "_sinittext", "_einittext" },
 	{ "_stext_l1",  "_etext_l1"  },	/* Blackfin on-chip L1 inst SRAM */
@@ -83,19 +83,20 @@ static inline int is_arm_mapping_symbol(const char *str)
 	       && (str[2] = '\0' || str[2] = '.');
 }
 
-static int read_symbol_tr(const char *sym, unsigned long long addr)
+static int check_symbol_range(const char *sym, unsigned long long addr,
+			      struct addr_range *ranges, int entries)
 {
 	size_t i;
-	struct text_range *tr;
+	struct addr_range *ar;
 
-	for (i = 0; i < ARRAY_SIZE(text_ranges); ++i) {
-		tr = &text_ranges[i];
+	for (i = 0; i < entries; ++i) {
+		ar = &ranges[i];
 
-		if (strcmp(sym, tr->stext) = 0) {
-			tr->start = addr;
+		if (strcmp(sym, ar->start_sym) = 0) {
+			ar->start = addr;
 			return 0;
-		} else if (strcmp(sym, tr->etext) = 0) {
-			tr->end = addr;
+		} else if (strcmp(sym, ar->end_sym) = 0) {
+			ar->end = addr;
 			return 0;
 		}
 	}
@@ -130,7 +131,8 @@ static int read_symbol(FILE *in, struct sym_entry *s)
 	/* Ignore most absolute/undefined (?) symbols. */
 	if (strcmp(sym, "_text") = 0)
 		_text = s->addr;
-	else if (read_symbol_tr(sym, s->addr) = 0)
+	else if (check_symbol_range(sym, s->addr, text_ranges,
+				    ARRAY_SIZE(text_ranges)) = 0)
 		/* nothing to do */;
 	else if (toupper(stype) = 'A')
 	{
@@ -167,15 +169,16 @@ static int read_symbol(FILE *in, struct sym_entry *s)
 	return 0;
 }
 
-static int symbol_valid_tr(struct sym_entry *s)
+static int symbol_in_range(struct sym_entry *s, struct addr_range *ranges,
+			   int entries)
 {
 	size_t i;
-	struct text_range *tr;
+	struct addr_range *ar;
 
-	for (i = 0; i < ARRAY_SIZE(text_ranges); ++i) {
-		tr = &text_ranges[i];
+	for (i = 0; i < entries; ++i) {
+		ar = &ranges[i];
 
-		if (s->addr >= tr->start && s->addr <= tr->end)
+		if (s->addr >= ar->start && s->addr <= ar->end)
 			return 1;
 	}
 
@@ -214,7 +217,8 @@ static int symbol_valid(struct sym_entry *s)
 	/* if --all-symbols is not specified, then symbols outside the text
 	 * and inittext sections are discarded */
 	if (!all_symbols) {
-		if (symbol_valid_tr(s) = 0)
+		if (symbol_in_range(s, text_ranges,
+				    ARRAY_SIZE(text_ranges)) = 0)
 			return 0;
 		/* Corner case.  Discard any symbols with the same value as
 		 * _etext _einittext; they can move between pass 1 and 2 when
@@ -223,9 +227,11 @@ static int symbol_valid(struct sym_entry *s)
 		 * rules.
 		 */
 		if ((s->addr = text_range_text->end &&
-				strcmp((char *)s->sym + offset, text_range_text->etext)) ||
+				strcmp((char *)s->sym + offset,
+				       text_range_text->end_sym)) ||
 		    (s->addr = text_range_inittext->end &&
-				strcmp((char *)s->sym + offset, text_range_inittext->etext)))
+				strcmp((char *)s->sym + offset,
+				       text_range_inittext->end_sym)))
 			return 0;
 	}
 
@@ -298,6 +304,11 @@ static int expand_symbol(unsigned char *data, int len, char *result)
 	return total;
 }
 
+static int symbol_absolute(struct sym_entry *s)
+{
+	return toupper(s->sym[0]) = 'A';
+}
+
 static void write_src(void)
 {
 	unsigned int i, k, off;
@@ -325,7 +336,7 @@ static void write_src(void)
 	 */
 	output_label("kallsyms_addresses");
 	for (i = 0; i < table_cnt; i++) {
-		if (toupper(table[i].sym[0]) != 'A') {
+		if (!symbol_absolute(&table[i])) {
 			if (_text <= table[i].addr)
 				printf("\tPTR\t_text + %#llx\n",
 					table[i].addr - _text);
-- 
1.7.9.5

[PATCH 1/2] kallsyms: generalize address range checking

[Kees Cook]

This refactors the address range checks to be generalized instead of
specific to text range checks, in preparation for other range checks.
Also extracts logic for "is the symbol absolute" into a function.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/kallsyms.c |   53 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index 276e84b8a8e5..3c6224728960 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -36,13 +36,13 @@ struct sym_entry {
 	unsigned char *sym;
 };
 
-struct text_range {
-	const char *stext, *etext;
+struct addr_range {
+	const char *start_sym, *end_sym;
 	unsigned long long start, end;
 };
 
 static unsigned long long _text;
-static struct text_range text_ranges[] = {
+static struct addr_range text_ranges[] = {
 	{ "_stext",     "_etext"     },
 	{ "_sinittext", "_einittext" },
 	{ "_stext_l1",  "_etext_l1"  },	/* Blackfin on-chip L1 inst SRAM */
@@ -83,19 +83,20 @@ static inline int is_arm_mapping_symbol(const char *str)
 	       && (str[2] == '\0' || str[2] == '.');
 }
 
-static int read_symbol_tr(const char *sym, unsigned long long addr)
+static int check_symbol_range(const char *sym, unsigned long long addr,
+			      struct addr_range *ranges, int entries)
 {
 	size_t i;
-	struct text_range *tr;
+	struct addr_range *ar;
 
-	for (i = 0; i < ARRAY_SIZE(text_ranges); ++i) {
-		tr = &text_ranges[i];
+	for (i = 0; i < entries; ++i) {
+		ar = &ranges[i];
 
-		if (strcmp(sym, tr->stext) == 0) {
-			tr->start = addr;
+		if (strcmp(sym, ar->start_sym) == 0) {
+			ar->start = addr;
 			return 0;
-		} else if (strcmp(sym, tr->etext) == 0) {
-			tr->end = addr;
+		} else if (strcmp(sym, ar->end_sym) == 0) {
+			ar->end = addr;
 			return 0;
 		}
 	}
@@ -130,7 +131,8 @@ static int read_symbol(FILE *in, struct sym_entry *s)
 	/* Ignore most absolute/undefined (?) symbols. */
 	if (strcmp(sym, "_text") == 0)
 		_text = s->addr;
-	else if (read_symbol_tr(sym, s->addr) == 0)
+	else if (check_symbol_range(sym, s->addr, text_ranges,
+				    ARRAY_SIZE(text_ranges)) == 0)
 		/* nothing to do */;
 	else if (toupper(stype) == 'A')
 	{
@@ -167,15 +169,16 @@ static int read_symbol(FILE *in, struct sym_entry *s)
 	return 0;
 }
 
-static int symbol_valid_tr(struct sym_entry *s)
+static int symbol_in_range(struct sym_entry *s, struct addr_range *ranges,
+			   int entries)
 {
 	size_t i;
-	struct text_range *tr;
+	struct addr_range *ar;
 
-	for (i = 0; i < ARRAY_SIZE(text_ranges); ++i) {
-		tr = &text_ranges[i];
+	for (i = 0; i < entries; ++i) {
+		ar = &ranges[i];
 
-		if (s->addr >= tr->start && s->addr <= tr->end)
+		if (s->addr >= ar->start && s->addr <= ar->end)
 			return 1;
 	}
 
@@ -214,7 +217,8 @@ static int symbol_valid(struct sym_entry *s)
 	/* if --all-symbols is not specified, then symbols outside the text
 	 * and inittext sections are discarded */
 	if (!all_symbols) {
-		if (symbol_valid_tr(s) == 0)
+		if (symbol_in_range(s, text_ranges,
+				    ARRAY_SIZE(text_ranges)) == 0)
 			return 0;
 		/* Corner case.  Discard any symbols with the same value as
 		 * _etext _einittext; they can move between pass 1 and 2 when
@@ -223,9 +227,11 @@ static int symbol_valid(struct sym_entry *s)
 		 * rules.
 		 */
 		if ((s->addr == text_range_text->end &&
-				strcmp((char *)s->sym + offset, text_range_text->etext)) ||
+				strcmp((char *)s->sym + offset,
+				       text_range_text->end_sym)) ||
 		    (s->addr == text_range_inittext->end &&
-				strcmp((char *)s->sym + offset, text_range_inittext->etext)))
+				strcmp((char *)s->sym + offset,
+				       text_range_inittext->end_sym)))
 			return 0;
 	}
 
@@ -298,6 +304,11 @@ static int expand_symbol(unsigned char *data, int len, char *result)
 	return total;
 }
 
+static int symbol_absolute(struct sym_entry *s)
+{
+	return toupper(s->sym[0]) == 'A';
+}
+
 static void write_src(void)
 {
 	unsigned int i, k, off;
@@ -325,7 +336,7 @@ static void write_src(void)
 	 */
 	output_label("kallsyms_addresses");
 	for (i = 0; i < table_cnt; i++) {
-		if (toupper(table[i].sym[0]) != 'A') {
+		if (!symbol_absolute(&table[i])) {
 			if (_text <= table[i].addr)
 				printf("\tPTR\t_text + %#llx\n",
 					table[i].addr - _text);
-- 
1.7.9.5

[PATCH 2/2] kallsyms: handle special absolute symbols

[Kees Cook]

This forces the entire per_cpu range to be reported as absolute without
losing their linker symbol types. Without this, the variables are
incorrectly shown as relocated under kASLR.

Several kallsyms output in different boot states for comparison of
various symbols:

$ egrep ' (gdt_|_(stext|_per_cpu_))' /root/kallsyms.nokaslr
0000000000000000 D __per_cpu_start
0000000000004000 D gdt_page
0000000000014280 D __per_cpu_end
ffffffff810001c8 T _stext
ffffffff81ee53c0 D __per_cpu_offset
$ egrep ' (gdt_|_(stext|_per_cpu_))' /root/kallsyms.kaslr1
000000001f200000 D __per_cpu_start
000000001f204000 D gdt_page
000000001f214280 D __per_cpu_end
ffffffffa02001c8 T _stext
ffffffffa10e53c0 D __per_cpu_offset
$ egrep ' (gdt_|_(stext|_per_cpu_))' /root/kallsyms.kaslr2
000000000d400000 D __per_cpu_start
000000000d404000 D gdt_page
000000000d414280 D __per_cpu_end
ffffffff8e4001c8 T _stext
ffffffff8f2e53c0 D __per_cpu_offset
$ egrep ' (gdt_|_(stext|_per_cpu_))' /root/kallsyms.kaslr-fixed
0000000000000000 D __per_cpu_start
0000000000004000 D gdt_page
0000000000014280 D __per_cpu_end
ffffffffadc001c8 T _stext
ffffffffaeae53c0 D __per_cpu_offset

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/kallsyms.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index 3c6224728960..4dcda98427cb 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -34,6 +34,7 @@ struct sym_entry {
 	unsigned int len;
 	unsigned int start_pos;
 	unsigned char *sym;
+	int force_absolute;
 };
 
 struct addr_range {
@@ -51,6 +52,10 @@ static struct addr_range text_ranges[] = {
 #define text_range_text     (&text_ranges[0])
 #define text_range_inittext (&text_ranges[1])
 
+static struct addr_range abs_ranges[] = {
+	{ "__per_cpu_start",	"__per_cpu_end", -1ULL, 0 },
+};
+
 static struct sym_entry *table;
 static unsigned int table_size, table_cnt;
 static int all_symbols = 0;
@@ -165,6 +170,10 @@ static int read_symbol(FILE *in, struct sym_entry *s)
 	}
 	strcpy((char *)s->sym + 1, str);
 	s->sym[0] = stype;
+	s->force_absolute = 0;
+
+	/* Check if we've found special absolute symbol range. */
+	check_symbol_range(sym, s->addr, abs_ranges, ARRAY_SIZE(abs_ranges));
 
 	return 0;
 }
@@ -210,6 +219,10 @@ static int symbol_valid(struct sym_entry *s)
 	if (s->addr < kernel_start_addr)
 		return 0;
 
+	/* Force special absolute symbols into being absolute. */
+	if (symbol_in_range(s, abs_ranges, ARRAY_SIZE(abs_ranges)) = 1)
+		s->force_absolute = 1;
+
 	/* skip prefix char */
 	if (symbol_prefix_char && *(s->sym + 1) = symbol_prefix_char)
 		offset++;
@@ -306,7 +319,7 @@ static int expand_symbol(unsigned char *data, int len, char *result)
 
 static int symbol_absolute(struct sym_entry *s)
 {
-	return toupper(s->sym[0]) = 'A';
+	return s->force_absolute || toupper(s->sym[0]) = 'A';
 }
 
 static void write_src(void)
-- 
1.7.9.5

[PATCH 2/2] kallsyms: handle special absolute symbols

[Kees Cook]

This forces the entire per_cpu range to be reported as absolute without
losing their linker symbol types. Without this, the variables are
incorrectly shown as relocated under kASLR.

Several kallsyms output in different boot states for comparison of
various symbols:

$ egrep ' (gdt_|_(stext|_per_cpu_))' /root/kallsyms.nokaslr
0000000000000000 D __per_cpu_start
0000000000004000 D gdt_page
0000000000014280 D __per_cpu_end
ffffffff810001c8 T _stext
ffffffff81ee53c0 D __per_cpu_offset
$ egrep ' (gdt_|_(stext|_per_cpu_))' /root/kallsyms.kaslr1
000000001f200000 D __per_cpu_start
000000001f204000 D gdt_page
000000001f214280 D __per_cpu_end
ffffffffa02001c8 T _stext
ffffffffa10e53c0 D __per_cpu_offset
$ egrep ' (gdt_|_(stext|_per_cpu_))' /root/kallsyms.kaslr2
000000000d400000 D __per_cpu_start
000000000d404000 D gdt_page
000000000d414280 D __per_cpu_end
ffffffff8e4001c8 T _stext
ffffffff8f2e53c0 D __per_cpu_offset
$ egrep ' (gdt_|_(stext|_per_cpu_))' /root/kallsyms.kaslr-fixed
0000000000000000 D __per_cpu_start
0000000000004000 D gdt_page
0000000000014280 D __per_cpu_end
ffffffffadc001c8 T _stext
ffffffffaeae53c0 D __per_cpu_offset

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/kallsyms.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index 3c6224728960..4dcda98427cb 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -34,6 +34,7 @@ struct sym_entry {
 	unsigned int len;
 	unsigned int start_pos;
 	unsigned char *sym;
+	int force_absolute;
 };
 
 struct addr_range {
@@ -51,6 +52,10 @@ static struct addr_range text_ranges[] = {
 #define text_range_text     (&text_ranges[0])
 #define text_range_inittext (&text_ranges[1])
 
+static struct addr_range abs_ranges[] = {
+	{ "__per_cpu_start",	"__per_cpu_end", -1ULL, 0 },
+};
+
 static struct sym_entry *table;
 static unsigned int table_size, table_cnt;
 static int all_symbols = 0;
@@ -165,6 +170,10 @@ static int read_symbol(FILE *in, struct sym_entry *s)
 	}
 	strcpy((char *)s->sym + 1, str);
 	s->sym[0] = stype;
+	s->force_absolute = 0;
+
+	/* Check if we've found special absolute symbol range. */
+	check_symbol_range(sym, s->addr, abs_ranges, ARRAY_SIZE(abs_ranges));
 
 	return 0;
 }
@@ -210,6 +219,10 @@ static int symbol_valid(struct sym_entry *s)
 	if (s->addr < kernel_start_addr)
 		return 0;
 
+	/* Force special absolute symbols into being absolute. */
+	if (symbol_in_range(s, abs_ranges, ARRAY_SIZE(abs_ranges)) == 1)
+		s->force_absolute = 1;
+
 	/* skip prefix char */
 	if (symbol_prefix_char && *(s->sym + 1) == symbol_prefix_char)
 		offset++;
@@ -306,7 +319,7 @@ static int expand_symbol(unsigned char *data, int len, char *result)
 
 static int symbol_absolute(struct sym_entry *s)
 {
-	return toupper(s->sym[0]) == 'A';
+	return s->force_absolute || toupper(s->sym[0]) == 'A';
 }
 
 static void write_src(void)
-- 
1.7.9.5

Re: [PATCH 2/2] kallsyms: handle special absolute symbols

[Rusty Russell]

Kees Cook <keescook@chromium.org> writes:
> This forces the entire per_cpu range to be reported as absolute without
> losing their linker symbol types. Without this, the variables are
> incorrectly shown as relocated under kASLR.

I like these patches, thanks!

This one's a bit broken, since the zero-based __per_cpu_start/end thing
is an x86-64-ism.  You really do want them relocated on other
platforms, so I think you'll need do make this conditional via
a --per-cpu-absolute flag to kallsyms (which x86-64 would set).

Dumb Q: why don't we actually present these symbols as absolute in
/proc/kallsyms?  Seems like it would be clearer...

Thanks,
Rusty.

Re: [PATCH 2/2] kallsyms: handle special absolute symbols

[Rusty Russell]

Kees Cook <keescook@chromium.org> writes:
> This forces the entire per_cpu range to be reported as absolute without
> losing their linker symbol types. Without this, the variables are
> incorrectly shown as relocated under kASLR.

I like these patches, thanks!

This one's a bit broken, since the zero-based __per_cpu_start/end thing
is an x86-64-ism.  You really do want them relocated on other
platforms, so I think you'll need do make this conditional via
a --per-cpu-absolute flag to kallsyms (which x86-64 would set).

Dumb Q: why don't we actually present these symbols as absolute in
/proc/kallsyms?  Seems like it would be clearer...

Thanks,
Rusty.

Re: [PATCH 2/2] kallsyms: handle special absolute symbols

[Kees Cook]

On Thu, Mar 6, 2014 at 7:25 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Kees Cook <keescook@chromium.org> writes:
>> This forces the entire per_cpu range to be reported as absolute without
>> losing their linker symbol types. Without this, the variables are
>> incorrectly shown as relocated under kASLR.
>
> I like these patches, thanks!

Oh good! Glad this is getting closer. :)

> This one's a bit broken, since the zero-based __per_cpu_start/end thing
> is an x86-64-ism.  You really do want them relocated on other
> platforms, so I think you'll need do make this conditional via
> a --per-cpu-absolute flag to kallsyms (which x86-64 would set).

Ah, hm. Can this maybe just be dynamically detected (e.g. if
__per_cpu_start = 0?), I'd hate to have another arch run into this
glitch when we could "notice" it and deal with it instead.

> Dumb Q: why don't we actually present these symbols as absolute in
> /proc/kallsyms?  Seems like it would be clearer...

You mean set "sym[0] = 'A'" instead of the force_absolute thing I
added? It seemed like I shouldn't mess with existing information, and
as you say, they're not absolute on all platforms.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 2/2] kallsyms: handle special absolute symbols

[Kees Cook]

On Thu, Mar 6, 2014 at 7:25 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Kees Cook <keescook@chromium.org> writes:
>> This forces the entire per_cpu range to be reported as absolute without
>> losing their linker symbol types. Without this, the variables are
>> incorrectly shown as relocated under kASLR.
>
> I like these patches, thanks!

Oh good! Glad this is getting closer. :)

> This one's a bit broken, since the zero-based __per_cpu_start/end thing
> is an x86-64-ism.  You really do want them relocated on other
> platforms, so I think you'll need do make this conditional via
> a --per-cpu-absolute flag to kallsyms (which x86-64 would set).

Ah, hm. Can this maybe just be dynamically detected (e.g. if
__per_cpu_start == 0?), I'd hate to have another arch run into this
glitch when we could "notice" it and deal with it instead.

> Dumb Q: why don't we actually present these symbols as absolute in
> /proc/kallsyms?  Seems like it would be clearer...

You mean set "sym[0] = 'A'" instead of the force_absolute thing I
added? It seemed like I shouldn't mess with existing information, and
as you say, they're not absolute on all platforms.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 2/2] kallsyms: handle special absolute symbols

[Tejun Heo]

> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> index 3c6224728960..4dcda98427cb 100644
> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c
> @@ -34,6 +34,7 @@ struct sym_entry {
>  	unsigned int len;
>  	unsigned int start_pos;
>  	unsigned char *sym;
> +	int force_absolute;

bool?

Thanks.

-- 
tejun

Re: [PATCH 2/2] kallsyms: handle special absolute symbols

[Tejun Heo]

> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> index 3c6224728960..4dcda98427cb 100644
> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c
> @@ -34,6 +34,7 @@ struct sym_entry {
>  	unsigned int len;
>  	unsigned int start_pos;
>  	unsigned char *sym;
> +	int force_absolute;

bool?

Thanks.

-- 
tejun