Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1626073 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 3/7] kprobes: validate the symbol name length "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-04-19 15:00 +0200
    Re: [PATCH v3 3/7] kprobes: validate the symbol name length Masami Hiramatsu <mhiramat@kernel.org> - 2017-04-19 17:40 +0200
      Re: [PATCH v3 3/7] kprobes: validate the symbol name length "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-04-19 18:40 +0200
        Re: [PATCH v3 3/7] kprobes: validate the symbol name length Masami Hiramatsu <mhiramat@kernel.org> - 2017-04-21 15:50 +0200
          Re: [PATCH v3 3/7] kprobes: validate the symbol name length "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-04-23 17:50 +0200
    Re: [PATCH v3 3/7] kprobes: validate the symbol name length Michael Ellerman <mpe@ellerman.id.au> - 2017-04-20 08:10 +0200
      Re: [PATCH v3 3/7] kprobes: validate the symbol name length "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-04-20 09:30 +0200
    [PATCH v4 4/7] powerpc/kprobes: Use safer string functions in kprobe_lookup_name() "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-04-21 14:40 +0200
      Re: [PATCH v4 4/7] powerpc/kprobes: Use safer string functions in  kprobe_lookup_name() Paul Clarke <pc@us.ibm.com> - 2017-04-21 15:40 +0200
        Re: [PATCH v4 4/7] powerpc/kprobes: Use safer string functions in  kprobe_lookup_name() Paul Clarke <pc@us.ibm.com> - 2017-04-21 15:40 +0200
        Re: [PATCH v4 4/7] powerpc/kprobes: Use safer string functions in  kprobe_lookup_name() Paul Clarke <pc@us.ibm.com> - 2017-04-21 16:00 +0200
          Re: [PATCH v4 4/7] powerpc/kprobes: Use safer string functions in  kprobe_lookup_name() "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-04-23 19:20 +0200
    [PATCH v4 3/7] kprobes: validate the symbol name provided during probe registration "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-04-21 14:40 +0200
      Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during  probe registration Paul Clarke <pc@us.ibm.com> - 2017-04-21 15:20 +0200
        Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during  probe registration "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-04-21 15:30 +0200
      Re: [PATCH v4 3/7] kprobes: validate the symbol name provided  during probe registration Masami Hiramatsu <mhiramat@kernel.org> - 2017-04-21 16:00 +0200
      Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during probe registration Michael Ellerman <mpe@ellerman.id.au> - 2017-04-22 08:00 +0200
        Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during  probe registration "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-04-23 19:50 +0200

#1626073 — [PATCH v3 3/7] kprobes: validate the symbol name length

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(+)

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;
 
-- 
2.12.1

[toc] | [next] | [standalone]

#1626394

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,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1626492

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>
> 
> 

[toc] | [prev] | [next] | [standalone]

#1628261

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>
> > 
> > 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1629043

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

[toc] | [prev] | [next] | [standalone]

#1627013

"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

[toc] | [prev] | [next] | [standalone]

#1627161

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

[toc] | [prev] | [next] | [standalone]

#1628206 — [PATCH v4 4/7] powerpc/kprobes: Use safer string functions in kprobe_lookup_name()

Convert usage of strchr()/strncpy()/strncat() to
strnchr()/memcpy()/strlcat() for simpler and safer string manipulation.

Reported-by: David Laight <David.Laight@ACULAB.COM>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
Changes: Additionally convert the strchr().


 arch/powerpc/kernel/kprobes.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 97b5eed1f76d..c73fb6e3b43f 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -65,28 +65,27 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
 	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
 	const char *modsym;
 	bool dot_appended = false;
-	if ((modsym = strchr(name, ':')) != NULL) {
+	if ((modsym = strnchr(name, ':', MODULE_NAME_LEN)) != NULL) {
 		modsym++;
 		if (*modsym != '\0' && *modsym != '.') {
 			/* Convert to <module:.symbol> */
-			strncpy(dot_name, name, modsym - name);
+			memcpy(dot_name, name, modsym - name);
 			dot_name[modsym - name] = '.';
 			dot_name[modsym - name + 1] = '\0';
-			strncat(dot_name, modsym,
-				sizeof(dot_name) - (modsym - name) - 2);
+			strlcat(dot_name, modsym, sizeof(dot_name));
 			dot_appended = true;
 		} else {
 			dot_name[0] = '\0';
-			strncat(dot_name, name, sizeof(dot_name) - 1);
+			strlcat(dot_name, name, sizeof(dot_name));
 		}
 	} else if (name[0] != '.') {
 		dot_name[0] = '.';
 		dot_name[1] = '\0';
-		strncat(dot_name, name, KSYM_NAME_LEN - 2);
+		strlcat(dot_name, name, sizeof(dot_name));
 		dot_appended = true;
 	} else {
 		dot_name[0] = '\0';
-		strncat(dot_name, name, KSYM_NAME_LEN - 1);
+		strlcat(dot_name, name, sizeof(dot_name));
 	}
 	addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
 	if (!addr && dot_appended) {
-- 
2.12.1

[toc] | [prev] | [next] | [standalone]

#1628254 — Re: [PATCH v4 4/7] powerpc/kprobes: Use safer string functions in kprobe_lookup_name()

On 04/21/2017 07:33 AM, Naveen N. Rao wrote:
> Convert usage of strchr()/strncpy()/strncat() to
> strnchr()/memcpy()/strlcat() for simpler and safer string manipulation.
> 
> Reported-by: David Laight <David.Laight@ACULAB.COM>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> Changes: Additionally convert the strchr().
> 
> 
>  arch/powerpc/kernel/kprobes.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 97b5eed1f76d..c73fb6e3b43f 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -65,28 +65,27 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
>  	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
>  	const char *modsym;
>  	bool dot_appended = false;
> -	if ((modsym = strchr(name, ':')) != NULL) {
> +	if ((modsym = strnchr(name, ':', MODULE_NAME_LEN)) != NULL) {
>  		modsym++;
>  		if (*modsym != '\0' && *modsym != '.') {
>  			/* Convert to <module:.symbol> */
> -			strncpy(dot_name, name, modsym - name);
> +			memcpy(dot_name, name, modsym - name);
>  			dot_name[modsym - name] = '.';
>  			dot_name[modsym - name + 1] = '\0';
> -			strncat(dot_name, modsym,
> -				sizeof(dot_name) - (modsym - name) - 2);
> +			strlcat(dot_name, modsym, sizeof(dot_name));

Would it be more efficient here to replace this:
--
dot_name[modsym - name + 1] = '\0';
strlcat(dot_name, modsym, sizeof(dot_name));
--

with this:
strncpy(&dot_name[modsym - name + 1], modsym, KSYM_NAME_LEN);

(So you aren't rescanning dot_name to find the end, when you already know the end position?)

>  			dot_appended = true;
>  		} else {
>  			dot_name[0] = '\0';
> -			strncat(dot_name, name, sizeof(dot_name) - 1);
> +			strlcat(dot_name, name, sizeof(dot_name));

and here do:
strncpy(dot_name, name, sizeof(dot_name));

(and remove the null termination immediately above)

>  		}
>  	} else if (name[0] != '.') {
>  		dot_name[0] = '.';
>  		dot_name[1] = '\0';
> -		strncat(dot_name, name, KSYM_NAME_LEN - 2);
> +		strlcat(dot_name, name, sizeof(dot_name));

and here do:
strncpy(&dot_name[1], name, sizeof(dot_name));

(and remove the null termination immediately above)

>  		dot_appended = true;
>  	} else {
>  		dot_name[0] = '\0';
> -		strncat(dot_name, name, KSYM_NAME_LEN - 1);
> +		strlcat(dot_name, name, sizeof(dot_name));

and here do:
strncpy(dot_name, name, sizeof(dot_name));

(and remove the null termination immediately above)

>  	}

PC

[toc] | [prev] | [next] | [standalone]

#1628256 — Re: [PATCH v4 4/7] powerpc/kprobes: Use safer string functions in kprobe_lookup_name()

On 04/21/2017 08:33 AM, Paul Clarke wrote:
> On 04/21/2017 07:33 AM, Naveen N. Rao wrote:
>>  	} else if (name[0] != '.') {
>>  		dot_name[0] = '.';
>>  		dot_name[1] = '\0';
>> -		strncat(dot_name, name, KSYM_NAME_LEN - 2);
>> +		strlcat(dot_name, name, sizeof(dot_name));
> 
> and here do:
> strncpy(&dot_name[1], name, sizeof(dot_name));

oops.  s/sizeof(dot_name)/sizeof(dot_name) - 1/

PC

[toc] | [prev] | [next] | [standalone]

#1628268 — Re: [PATCH v4 4/7] powerpc/kprobes: Use safer string functions in kprobe_lookup_name()

Sent too soon.  The suggestions don't guarantee null termination.  Refined, below. (Sorry for the noise.)

On 04/21/2017 08:33 AM, Paul Clarke wrote:
> On 04/21/2017 07:33 AM, Naveen N. Rao wrote:
>> Convert usage of strchr()/strncpy()/strncat() to
>> strnchr()/memcpy()/strlcat() for simpler and safer string manipulation.

>> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
>> index 97b5eed1f76d..c73fb6e3b43f 100644
>> --- a/arch/powerpc/kernel/kprobes.c
>> +++ b/arch/powerpc/kernel/kprobes.c
>> @@ -65,28 +65,27 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
>>  	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
>>  	const char *modsym;
>>  	bool dot_appended = false;
>> -	if ((modsym = strchr(name, ':')) != NULL) {
>> +	if ((modsym = strnchr(name, ':', MODULE_NAME_LEN)) != NULL) {
>>  		modsym++;
>>  		if (*modsym != '\0' && *modsym != '.') {
>>  			/* Convert to <module:.symbol> */
>> -			strncpy(dot_name, name, modsym - name);
>> +			memcpy(dot_name, name, modsym - name);
>>  			dot_name[modsym - name] = '.';
>>  			dot_name[modsym - name + 1] = '\0';
>> -			strncat(dot_name, modsym,
>> -				sizeof(dot_name) - (modsym - name) - 2);
>> +			strlcat(dot_name, modsym, sizeof(dot_name));
> 
> Would it be more efficient here to replace this:
> --
> dot_name[modsym - name + 1] = '\0';
> strlcat(dot_name, modsym, sizeof(dot_name));
> --
> 
> with this:
> strncpy(&dot_name[modsym - name + 1], modsym, KSYM_NAME_LEN);

keep the null termination, and just adjust the starting point for strlcat:
--
dot_name[modsym - name + 1] = '\0';
strlcat(&dot_name[modsym - name + 1], modsym, KSYM_NAME_LEN);
--

> (So you aren't rescanning dot_name to find the end, when you already know the end position?)
> 
>>  			dot_appended = true;
>>  		} else {
>>  			dot_name[0] = '\0';
>> -			strncat(dot_name, name, sizeof(dot_name) - 1);
>> +			strlcat(dot_name, name, sizeof(dot_name));
> 
> and here do:
> strncpy(dot_name, name, sizeof(dot_name));
> 
> (and remove the null termination immediately above)

nevermind.  :-)

> 
>>  		}
>>  	} else if (name[0] != '.') {
>>  		dot_name[0] = '.';
>>  		dot_name[1] = '\0';
>> -		strncat(dot_name, name, KSYM_NAME_LEN - 2);
>> +		strlcat(dot_name, name, sizeof(dot_name));
> 
> and here do:
> strncpy(&dot_name[1], name, sizeof(dot_name));
> 
> (and remove the null termination immediately above)
> 

nevermind.  :-)

>>  		dot_appended = true;
>>  	} else {
>>  		dot_name[0] = '\0';
>> -		strncat(dot_name, name, KSYM_NAME_LEN - 1);
>> +		strlcat(dot_name, name, sizeof(dot_name));
> 
> and here do:
> strncpy(dot_name, name, sizeof(dot_name));
> 
> (and remove the null termination immediately above)
> 
>>  	}

nevermind.  :-)

PC

[toc] | [prev] | [next] | [standalone]

#1629051 — Re: [PATCH v4 4/7] powerpc/kprobes: Use safer string functions in kprobe_lookup_name()

Excerpts from Paul Clarke's message of April 21, 2017 19:22:
> Sent too soon.  The suggestions don't guarantee null termination.  Refined, below. (Sorry for the noise.)

Yeah, the string operations here are a bit of a minefield...

> 
> On 04/21/2017 08:33 AM, Paul Clarke wrote:
>> On 04/21/2017 07:33 AM, Naveen N. Rao wrote:
>>> Convert usage of strchr()/strncpy()/strncat() to
>>> strnchr()/memcpy()/strlcat() for simpler and safer string manipulation.
> 
>>> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
>>> index 97b5eed1f76d..c73fb6e3b43f 100644
>>> --- a/arch/powerpc/kernel/kprobes.c
>>> +++ b/arch/powerpc/kernel/kprobes.c
>>> @@ -65,28 +65,27 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
>>>  	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
>>>  	const char *modsym;
>>>  	bool dot_appended = false;
>>> -	if ((modsym = strchr(name, ':')) != NULL) {
>>> +	if ((modsym = strnchr(name, ':', MODULE_NAME_LEN)) != NULL) {

... as I just realized this is an epic FAIL! ;/

I will take my time to redo this.

- Naveen

[toc] | [prev] | [next] | [standalone]

#1628209 — [PATCH v4 3/7] kprobes: validate the symbol name provided during probe registration

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>
---
Masami, Michael,
Here's a simplified version that should hopefully be fine. I have
simplified the logic to keep the code compact and simple.

- Naveen


 include/linux/kprobes.h     |  1 +
 kernel/kprobes.c            | 30 ++++++++++++++++++++++++++++++
 kernel/trace/trace_kprobe.c |  4 ++++
 3 files changed, 35 insertions(+)

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..ff9b1ac72a38 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1383,6 +1383,34 @@ bool within_kprobe_blacklist(unsigned long addr)
 }
 
 /*
+ * We mainly want to ensure that the provided string is of a reasonable length
+ * and is of the form [<mod_name>:]<sym_name>, so that this is safe to process
+ * further.
+ * We don't worry about invalid characters as those will just prevent
+ * matching existing kallsyms.
+ */
+bool is_valid_kprobe_symbol_name(const char *name)
+{
+	size_t sym_len;
+	const char *s;
+
+	s = strnchr(name, ':', MODULE_NAME_LEN + KSYM_NAME_LEN + 1);
+	if (s) {
+		sym_len = (size_t)(s - name);
+		if (sym_len <= 0  || sym_len >= MODULE_NAME_LEN)
+			return false;
+		s++;
+	} else
+		s = name;
+
+	sym_len = strnlen(s, KSYM_NAME_LEN);
+	if (sym_len <= 0 || sym_len >= KSYM_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.
  * This returns encoded errors if it fails to look up symbol or invalid
@@ -1397,6 +1425,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

[toc] | [prev] | [next] | [standalone]

#1628242 — Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during probe registration

a nit or two, below...

On 04/21/2017 07:32 AM, Naveen N. Rao wrote:
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 6a128f3a7ed1..ff9b1ac72a38 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1383,6 +1383,34 @@ bool within_kprobe_blacklist(unsigned long addr)
>  }
> 
>  /*
> + * We mainly want to ensure that the provided string is of a reasonable length
> + * and is of the form [<mod_name>:]<sym_name>, so that this is safe to process
> + * further.
> + * We don't worry about invalid characters as those will just prevent
> + * matching existing kallsyms.
> + */
> +bool is_valid_kprobe_symbol_name(const char *name)
> +{
> +	size_t sym_len;
> +	const char *s;
> +
> +	s = strnchr(name, ':', MODULE_NAME_LEN + KSYM_NAME_LEN + 1);
> +	if (s) {
> +		sym_len = (size_t)(s - name);
> +		if (sym_len <= 0  || sym_len >= MODULE_NAME_LEN)

"sym_len <= 0" looks odd here, since sym_len is likely unsigned and would never be less than zero, anyway.

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

here, too.

> +		return false;
> +
> +	return true;
> +}

PC

[toc] | [prev] | [next] | [standalone]

#1628253 — Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during probe registration

Excerpts from Paul Clarke's message of April 21, 2017 18:41:
> a nit or two, below...
> 
> On 04/21/2017 07:32 AM, Naveen N. Rao wrote:
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index 6a128f3a7ed1..ff9b1ac72a38 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -1383,6 +1383,34 @@ bool within_kprobe_blacklist(unsigned long addr)
>>  }
>> 
>>  /*
>> + * We mainly want to ensure that the provided string is of a reasonable length
>> + * and is of the form [<mod_name>:]<sym_name>, so that this is safe to process
>> + * further.
>> + * We don't worry about invalid characters as those will just prevent
>> + * matching existing kallsyms.
>> + */
>> +bool is_valid_kprobe_symbol_name(const char *name)
>> +{
>> +	size_t sym_len;
>> +	const char *s;
>> +
>> +	s = strnchr(name, ':', MODULE_NAME_LEN + KSYM_NAME_LEN + 1);
>> +	if (s) {
>> +		sym_len = (size_t)(s - name);
>> +		if (sym_len <= 0  || sym_len >= MODULE_NAME_LEN)
> 
> "sym_len <= 0" looks odd here, since sym_len is likely unsigned and would never be less than zero, anyway.

Ugh.. habits :/
I'll wait for Masami/Michael's feedback before re-spinning.

Thanks for the review,
- Naveen

[toc] | [prev] | [next] | [standalone]

#1628269 — Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during probe registration

On Fri, 21 Apr 2017 18:02:33 +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.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> Masami, Michael,
> Here's a simplified version that should hopefully be fine. I have
> simplified the logic to keep the code compact and simple.

Ok, so as I replied to older version,
 - Ensure name != NULL at first.
 - Define it in kernel/kallsyms.c as a general routine.

Since this validate the string which will be passed to kallsyms,
I think it should be provided by kallsyms.

Thanks, 

> 
> - Naveen
> 
> 
>  include/linux/kprobes.h     |  1 +
>  kernel/kprobes.c            | 30 ++++++++++++++++++++++++++++++
>  kernel/trace/trace_kprobe.c |  4 ++++
>  3 files changed, 35 insertions(+)
> 
> 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..ff9b1ac72a38 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1383,6 +1383,34 @@ bool within_kprobe_blacklist(unsigned long addr)
>  }
>  
>  /*
> + * We mainly want to ensure that the provided string is of a reasonable length
> + * and is of the form [<mod_name>:]<sym_name>, so that this is safe to process
> + * further.
> + * We don't worry about invalid characters as those will just prevent
> + * matching existing kallsyms.
> + */
> +bool is_valid_kprobe_symbol_name(const char *name)
> +{
> +	size_t sym_len;
> +	const char *s;
> +
> +	s = strnchr(name, ':', MODULE_NAME_LEN + KSYM_NAME_LEN + 1);
> +	if (s) {
> +		sym_len = (size_t)(s - name);
> +		if (sym_len <= 0  || sym_len >= MODULE_NAME_LEN)
> +			return false;
> +		s++;
> +	} else
> +		s = name;
> +
> +	sym_len = strnlen(s, KSYM_NAME_LEN);
> +	if (sym_len <= 0 || sym_len >= KSYM_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.
>   * This returns encoded errors if it fails to look up symbol or invalid
> @@ -1397,6 +1425,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
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1628785 — Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during probe registration

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> 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.

What are we actually trying to protect against here?

If you ignore powerpc for a moment, kprobe_lookup_name() is just
kallsyms_lookup_name().

All kallsyms_lookup_name() does with name is strcmp() it against a
legitimate symbol name which is at most KSYM_NAME_LEN.

So I don't think any of this validation helps in that case?

In the powerpc version of kprobe_lookup_name() we do need to do some
string juggling, for which it helps to know the input is sane. But I
think we should just make that code more robust by checking the input
before we do anything with it.

cheers

[toc] | [prev] | [next] | [standalone]

#1629056 — Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during probe registration

Excerpts from Michael Ellerman's message of April 22, 2017 11:25:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
>> 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.
> 
> What are we actually trying to protect against here?
> 
> If you ignore powerpc for a moment, kprobe_lookup_name() is just
> kallsyms_lookup_name().
> 
> All kallsyms_lookup_name() does with name is strcmp() it against a
> legitimate symbol name which is at most KSYM_NAME_LEN.
> 
> So I don't think any of this validation helps in that case?

It does:
https://patchwork.kernel.org/patch/9695139/

It is far too easy to cause a OOPS due to the above, though this is 
root-only (for modules).

As I stated earlier, I think it is always a good practice to validate 
inputs right on entry, rather than later. Code gets refactored, 
different arch support gets added, and so on.

Doing this validation here ensures we don't have to worry about how we 
process this later, or if another arch has to over-ride 
kprobe_lookup_name().

> 
> In the powerpc version of kprobe_lookup_name() we do need to do some
> string juggling, for which it helps to know the input is sane. But I
> think we should just make that code more robust by checking the input
> before we do anything with it.

Ok, I will fold those tests in with the powerpc implementation for now 
and consider a patch against kallsyms_lookup_name(), like Masami 
recommends.


- Naveen

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web