On Wed, 19 Apr 2017 16:38:22 +0000
"Naveen N. Rao" <naveen.n.rao@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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;