On Sun, 23 Apr 2017 15:44:32 +0000
"Naveen N. Rao" [off-list ref] wrote:

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

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

quoted

quoted

+	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 [off-list ref]