[v3,3/7] kprobes: validate the symbol name length

Submitted by Naveen N. Rao on April 19, 2017, 12:51 p.m.

Details

Message ID 6e14d22994530fb5200c74d1593e73541d3b8028.1492604782.git.naveen.n.rao@linux.vnet.ibm.com
State Superseded
Headers show

Commit Message

Naveen N. Rao April 19, 2017, 12:51 p.m.
When a kprobe is being registered, we use the symbol_name field to
lookup the address where the probe should be placed. Since this is a
user-provided field, let's ensure that the length of the string is
within expected limits.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 include/linux/kprobes.h     |  1 +
 kernel/kprobes.c            | 24 ++++++++++++++++++++++++
 kernel/trace/trace_kprobe.c |  4 ++++
 3 files changed, 29 insertions(+)

Comments

Masami Hiramatsu April 19, 2017, 2:37 p.m.
On Wed, 19 Apr 2017 18:21:02 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> When a kprobe is being registered, we use the symbol_name field to
> lookup the address where the probe should be placed. Since this is a
> user-provided field, let's ensure that the length of the string is
> within expected limits.

Would we really need this? Of course it may filter out longer
strings... anyway such name should be rejected by kallsyms.

[...]
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 6a128f3a7ed1..bb86681c8a10 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1382,6 +1382,28 @@ bool within_kprobe_blacklist(unsigned long addr)
>  	return false;
>  }
>  
> +bool is_valid_kprobe_symbol_name(const char *name)

This just check the length of symbol_name buffer, and can contain
some invalid chars.

> +{
> +	size_t sym_len;
> +	char *s;
> +
> +	s = strchr(name, ':');
> +	if (s) {
> +		sym_len = strnlen(s+1, KSYM_NAME_LEN);

If you use strnlen() here, you just need to ensure sym_len < KSYM_NAME_LEN.

> +		if (sym_len <= 0 || sym_len >= KSYM_NAME_LEN)
> +			return false;
> +		sym_len = (size_t)(s - name);
> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
> +			return false;
> +	} else {
> +		sym_len = strnlen(name, MODULE_NAME_LEN);
> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)

Would you mean KSYM_NAME_LEN here?

> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  /*
>   * If we have a symbol_name argument, look it up and add the offset field
>   * to it. This way, we can specify a relative address to a symbol.
> @@ -1397,6 +1419,8 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
>  		goto invalid;
>  
>  	if (p->symbol_name) {
> +		if (!is_valid_kprobe_symbol_name(p->symbol_name))
> +			return ERR_PTR(-EINVAL);
>  		addr = kprobe_lookup_name(p->symbol_name, p->offset);
>  		if (!addr)
>  			return ERR_PTR(-ENOENT);
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 5f688cc724f0..bf73e5f31128 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -704,6 +704,10 @@ static int create_trace_kprobe(int argc, char **argv)
>  			pr_info("Return probe must be used without offset.\n");
>  			return -EINVAL;
>  		}
> +		if (!is_valid_kprobe_symbol_name(symbol)) {
> +			pr_info("Symbol name is too long.\n");
> +			return -EINVAL;
> +		}
>  	}
>  	argc -= 2; argv += 2;
>  
> -- 
> 2.12.1
> 

Thanks,
Naveen N. Rao April 19, 2017, 4:38 p.m.
Excerpts from Masami Hiramatsu's message of April 19, 2017 20:07:
> On Wed, 19 Apr 2017 18:21:02 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> When a kprobe is being registered, we use the symbol_name field to
>> lookup the address where the probe should be placed. Since this is a
>> user-provided field, let's ensure that the length of the string is
>> within expected limits.
> 
> Would we really need this? Of course it may filter out longer
> strings... anyway such name should be rejected by kallsyms.

I felt this would be good to have generically, as kallsyms does many 
string operations on the symbol name, including an unbounded strchr().

> 
> [...]
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index 6a128f3a7ed1..bb86681c8a10 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -1382,6 +1382,28 @@ bool within_kprobe_blacklist(unsigned long addr)
>>  	return false;
>>  }
>>  
>> +bool is_valid_kprobe_symbol_name(const char *name)
> 
> This just check the length of symbol_name buffer, and can contain
> some invalid chars.

Yes, I kept the function name generic incase we would like to do more 
validation in future, plus it's shorter than 
is_valid_kprobe_symbol_name_len() ;-)

> 
>> +{
>> +	size_t sym_len;
>> +	char *s;
>> +
>> +	s = strchr(name, ':');

Hmm.. this should be strnchr(). I re-factored the code that moved the 
strnlen() above this below. I'll fix this.

>> +	if (s) {
>> +		sym_len = strnlen(s+1, KSYM_NAME_LEN);
> 
> If you use strnlen() here, you just need to ensure sym_len < KSYM_NAME_LEN.

Hmm.. not sure I follow. Are you saying the check for sym_len <= 0 is 
not needed?

> 
>> +		if (sym_len <= 0 || sym_len >= KSYM_NAME_LEN)
>> +			return false;
>> +		sym_len = (size_t)(s - name);
>> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
>> +			return false;
>> +	} else {
>> +		sym_len = strnlen(name, MODULE_NAME_LEN);
>> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
> 
> Would you mean KSYM_NAME_LEN here?

Oops... nice catch, Thanks!

- Naveen

> 
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>>  /*
>>   * If we have a symbol_name argument, look it up and add the offset field
>>   * to it. This way, we can specify a relative address to a symbol.
>> @@ -1397,6 +1419,8 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
>>  		goto invalid;
>>  
>>  	if (p->symbol_name) {
>> +		if (!is_valid_kprobe_symbol_name(p->symbol_name))
>> +			return ERR_PTR(-EINVAL);
>>  		addr = kprobe_lookup_name(p->symbol_name, p->offset);
>>  		if (!addr)
>>  			return ERR_PTR(-ENOENT);
>> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
>> index 5f688cc724f0..bf73e5f31128 100644
>> --- a/kernel/trace/trace_kprobe.c
>> +++ b/kernel/trace/trace_kprobe.c
>> @@ -704,6 +704,10 @@ static int create_trace_kprobe(int argc, char **argv)
>>  			pr_info("Return probe must be used without offset.\n");
>>  			return -EINVAL;
>>  		}
>> +		if (!is_valid_kprobe_symbol_name(symbol)) {
>> +			pr_info("Symbol name is too long.\n");
>> +			return -EINVAL;
>> +		}
>>  	}
>>  	argc -= 2; argv += 2;
>>  
>> -- 
>> 2.12.1
>> 
> 
> Thanks,
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>
> 
>
Michael Ellerman April 20, 2017, 6:08 a.m.
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 6a128f3a7ed1..bb86681c8a10 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1382,6 +1382,28 @@ bool within_kprobe_blacklist(unsigned long addr)
>  	return false;
>  }
>  
> +bool is_valid_kprobe_symbol_name(const char *name)
> +{
> +	size_t sym_len;
> +	char *s;
> +
> +	s = strchr(name, ':');
> +	if (s) {
> +		sym_len = strnlen(s+1, KSYM_NAME_LEN);
> +		if (sym_len <= 0 || sym_len >= KSYM_NAME_LEN)
> +			return false;
> +		sym_len = (size_t)(s - name);
> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
> +			return false;
> +	} else {
> +		sym_len = strnlen(name, MODULE_NAME_LEN);
> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
> +			return false;
> +	}

I think this is probably more elaborate than it needs to be.

Why not just check the string is <= (MODULE_NAME_LEN + KSYM_NAME_LEN) ?

cheers
Naveen N. Rao April 20, 2017, 7:19 a.m.
Excerpts from Michael Ellerman's message of April 20, 2017 11:38:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index 6a128f3a7ed1..bb86681c8a10 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -1382,6 +1382,28 @@ bool within_kprobe_blacklist(unsigned long addr)
>>  	return false;
>>  }
>>  
>> +bool is_valid_kprobe_symbol_name(const char *name)
>> +{
>> +	size_t sym_len;
>> +	char *s;
>> +
>> +	s = strchr(name, ':');
>> +	if (s) {
>> +		sym_len = strnlen(s+1, KSYM_NAME_LEN);
>> +		if (sym_len <= 0 || sym_len >= KSYM_NAME_LEN)
>> +			return false;
>> +		sym_len = (size_t)(s - name);
>> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
>> +			return false;
>> +	} else {
>> +		sym_len = strnlen(name, MODULE_NAME_LEN);
>> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
>> +			return false;
>> +	}
> 
> I think this is probably more elaborate than it needs to be.
> 
> Why not just check the string is <= (MODULE_NAME_LEN + KSYM_NAME_LEN) ?

Yes, that would be sufficient for now.

It's probably just me being paranoid, but I felt it's good to have 
stricter checks for user-provided strings, to guard against future 
changes to how we process this.

- Naveen
Masami Hiramatsu April 21, 2017, 1:42 p.m.
On Wed, 19 Apr 2017 16:38:22 +0000
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> Excerpts from Masami Hiramatsu's message of April 19, 2017 20:07:
> > On Wed, 19 Apr 2017 18:21:02 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> >> When a kprobe is being registered, we use the symbol_name field to
> >> lookup the address where the probe should be placed. Since this is a
> >> user-provided field, let's ensure that the length of the string is
> >> within expected limits.
> > 
> > Would we really need this? Of course it may filter out longer
> > strings... anyway such name should be rejected by kallsyms.
> 
> I felt this would be good to have generically, as kallsyms does many 
> string operations on the symbol name, including an unbounded strchr().

OK, so this is actually for performance reason.

> 
> > 
> > [...]
> >> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> >> index 6a128f3a7ed1..bb86681c8a10 100644
> >> --- a/kernel/kprobes.c
> >> +++ b/kernel/kprobes.c
> >> @@ -1382,6 +1382,28 @@ bool within_kprobe_blacklist(unsigned long addr)
> >>  	return false;
> >>  }
> >>  
> >> +bool is_valid_kprobe_symbol_name(const char *name)
> > 
> > This just check the length of symbol_name buffer, and can contain
> > some invalid chars.
> 
> Yes, I kept the function name generic incase we would like to do more 
> validation in future, plus it's shorter than 
> is_valid_kprobe_symbol_name_len() ;-)

OK, if this is enough general, we'd better define this in
kernel/kallsyms.c or in kallsyms.h. Of course the function
should be called is_valid_symbol_name(). :-)

> >> +{
> >> +	size_t sym_len;
> >> +	char *s;
> >> +
> >> +	s = strchr(name, ':');
> 
> Hmm.. this should be strnchr(). I re-factored the code that moved the 
> strnlen() above this below. I'll fix this.
> 
> >> +	if (s) {
> >> +		sym_len = strnlen(s+1, KSYM_NAME_LEN);
> > 
> > If you use strnlen() here, you just need to ensure sym_len < KSYM_NAME_LEN.
> 
> Hmm.. not sure I follow. Are you saying the check for sym_len <= 0 is 
> not needed?

You can check sym_len != 0, but anyway, here we concern about
"longer" string (for performance reason), we can focus on
such case.
(BTW, could you also check the name != NULL at first?)

So, what I think it can be;

if (strnlen(s+1, KSYM_NAME_LEN) == KSYM_NAME_LEN ||
    (size_t)(s - name) >= MODULE_NAME_LEN)
	return false;

Thanks,

> >> +		if (sym_len <= 0 || sym_len >= KSYM_NAME_LEN)
> >> +			return false;
> >> +		sym_len = (size_t)(s - name);
> >> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
> >> +			return false;
> >> +	} else {
> >> +		sym_len = strnlen(name, MODULE_NAME_LEN);
> >> +		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
> > 
> > Would you mean KSYM_NAME_LEN here?
> 
> Oops... nice catch, Thanks!
> 
> - Naveen
> 
> > 
> >> +			return false;
> >> +	}
> >> +
> >> +	return true;
> >> +}
> >> +
> >>  /*
> >>   * If we have a symbol_name argument, look it up and add the offset field
> >>   * to it. This way, we can specify a relative address to a symbol.
> >> @@ -1397,6 +1419,8 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
> >>  		goto invalid;
> >>  
> >>  	if (p->symbol_name) {
> >> +		if (!is_valid_kprobe_symbol_name(p->symbol_name))
> >> +			return ERR_PTR(-EINVAL);
> >>  		addr = kprobe_lookup_name(p->symbol_name, p->offset);
> >>  		if (!addr)
> >>  			return ERR_PTR(-ENOENT);
> >> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> >> index 5f688cc724f0..bf73e5f31128 100644
> >> --- a/kernel/trace/trace_kprobe.c
> >> +++ b/kernel/trace/trace_kprobe.c
> >> @@ -704,6 +704,10 @@ static int create_trace_kprobe(int argc, char **argv)
> >>  			pr_info("Return probe must be used without offset.\n");
> >>  			return -EINVAL;
> >>  		}
> >> +		if (!is_valid_kprobe_symbol_name(symbol)) {
> >> +			pr_info("Symbol name is too long.\n");
> >> +			return -EINVAL;
> >> +		}
> >>  	}
> >>  	argc -= 2; argv += 2;
> >>  
> >> -- 
> >> 2.12.1
> >> 
> > 
> > Thanks,
> > 
> > -- 
> > Masami Hiramatsu <mhiramat@kernel.org>
> > 
> > 
>
Naveen N. Rao April 23, 2017, 3:44 p.m.
Excerpts from Masami Hiramatsu's message of April 21, 2017 19:12:
> On Wed, 19 Apr 2017 16:38:22 +0000
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> Excerpts from Masami Hiramatsu's message of April 19, 2017 20:07:
>> > On Wed, 19 Apr 2017 18:21:02 +0530
>> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>> > 
>> >> When a kprobe is being registered, we use the symbol_name field to
>> >> lookup the address where the probe should be placed. Since this is a
>> >> user-provided field, let's ensure that the length of the string is
>> >> within expected limits.
>> > 
>> > Would we really need this? Of course it may filter out longer
>> > strings... anyway such name should be rejected by kallsyms.
>> 
>> I felt this would be good to have generically, as kallsyms does many 
>> string operations on the symbol name, including an unbounded 
>> strchr().
> 
> OK, so this is actually for performance reason.
> 
>> 
>> > 
>> > [...]
>> >> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> >> index 6a128f3a7ed1..bb86681c8a10 100644
>> >> --- a/kernel/kprobes.c
>> >> +++ b/kernel/kprobes.c
>> >> @@ -1382,6 +1382,28 @@ bool within_kprobe_blacklist(unsigned long addr)
>> >>  	return false;
>> >>  }
>> >>  
>> >> +bool is_valid_kprobe_symbol_name(const char *name)
>> > 
>> > This just check the length of symbol_name buffer, and can contain
>> > some invalid chars.
>> 
>> Yes, I kept the function name generic incase we would like to do more 
>> validation in future, plus it's shorter than 
>> is_valid_kprobe_symbol_name_len() ;-)
> 
> OK, if this is enough general, we'd better define this in
> kernel/kallsyms.c or in kallsyms.h. Of course the function
> should be called is_valid_symbol_name(). :-)

I actually think this should be done in kprobes itself. The primary 
intent is to perform such validation right when we first obtain the 
input from the user. In this case, however, kallsyms_lookup_name() is 
also an exported symbol, so I do think some validation there would be 
good to have as well.

> 
>> >> +{
>> >> +	size_t sym_len;
>> >> +	char *s;
>> >> +
>> >> +	s = strchr(name, ':');
>> 
>> Hmm.. this should be strnchr(). I re-factored the code that moved the 
>> strnlen() above this below. I'll fix this.
>> 
>> >> +	if (s) {
>> >> +		sym_len = strnlen(s+1, KSYM_NAME_LEN);
>> > 
>> > If you use strnlen() here, you just need to ensure sym_len < KSYM_NAME_LEN.
>> 
>> Hmm.. not sure I follow. Are you saying the check for sym_len <= 0 is 
>> not needed?
> 
> You can check sym_len != 0, but anyway, here we concern about
> "longer" string (for performance reason), we can focus on
> such case.
> (BTW, could you also check the name != NULL at first?)
> 
> So, what I think it can be;
> 
> if (strnlen(s+1, KSYM_NAME_LEN) == KSYM_NAME_LEN ||
>     (size_t)(s - name) >= MODULE_NAME_LEN)
> 	return false;

Sure, thanks. I clearly need to refactor this code better!

- Naveen

Patch hide | download patch | download mbox

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 1f82a3db00b1..4ee10fef5135 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -405,6 +405,7 @@  int disable_kprobe(struct kprobe *kp);
 int enable_kprobe(struct kprobe *kp);
 
 void dump_kprobe(struct kprobe *kp);
+bool is_valid_kprobe_symbol_name(const char *name);
 
 #else /* !CONFIG_KPROBES: */
 
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 6a128f3a7ed1..bb86681c8a10 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1382,6 +1382,28 @@  bool within_kprobe_blacklist(unsigned long addr)
 	return false;
 }
 
+bool is_valid_kprobe_symbol_name(const char *name)
+{
+	size_t sym_len;
+	char *s;
+
+	s = strchr(name, ':');
+	if (s) {
+		sym_len = strnlen(s+1, KSYM_NAME_LEN);
+		if (sym_len <= 0 || sym_len >= KSYM_NAME_LEN)
+			return false;
+		sym_len = (size_t)(s - name);
+		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
+			return false;
+	} else {
+		sym_len = strnlen(name, MODULE_NAME_LEN);
+		if (sym_len <= 0 || sym_len >= MODULE_NAME_LEN)
+			return false;
+	}
+
+	return true;
+}
+
 /*
  * If we have a symbol_name argument, look it up and add the offset field
  * to it. This way, we can specify a relative address to a symbol.
@@ -1397,6 +1419,8 @@  static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
 		goto invalid;
 
 	if (p->symbol_name) {
+		if (!is_valid_kprobe_symbol_name(p->symbol_name))
+			return ERR_PTR(-EINVAL);
 		addr = kprobe_lookup_name(p->symbol_name, p->offset);
 		if (!addr)
 			return ERR_PTR(-ENOENT);
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 5f688cc724f0..bf73e5f31128 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -704,6 +704,10 @@  static int create_trace_kprobe(int argc, char **argv)
 			pr_info("Return probe must be used without offset.\n");
 			return -EINVAL;
 		}
+		if (!is_valid_kprobe_symbol_name(symbol)) {
+			pr_info("Symbol name is too long.\n");
+			return -EINVAL;
+		}
 	}
 	argc -= 2; argv += 2;