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

Groups > linux.kernel > #1629043

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

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[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

csiph-web