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

[Masami Hiramatsu <mhiramat@kernel.org>]

On Sun, 23 Apr 2017 15:44:32 +0000
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

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

IMHO, it is natural that kallsyms will know what is valid symbols.
Providing validation function by kprobes means kprobes also knows
that, and I concerns that may lead a double standard.

Thanks,

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


-- 
Masami Hiramatsu <mhiramat@kernel.org>