On Sun, 23 Apr 2017 15:44:32 +0000
"Naveen N. Rao" <naveen.n....@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 <mhira...@kernel.org>
