Excerpts from Masami Hiramatsu's message of April 21, 2017 19:12:

On Wed, 19 Apr 2017 16:38:22 +0000
"Naveen N. Rao" [off-list ref] wrote:
=20

quoted

Excerpts from Masami Hiramatsu's message of April 19, 2017 20:07:

quoted

On Wed, 19 Apr 2017 18:21:02 +0530
"Naveen N. Rao" [off-list ref] wrote:
=20

quoted

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.

=20
Would we really need this? Of course it may filter out longer
strings... anyway such name should be rejected by kallsyms.

=20
I felt this would be good to have generically, as kallsyms does many=20
string operations on the symbol name, including an unbounded=20
strchr().

=20
OK, so this is actually for performance reason.
=20

quoted

=20

quoted

=20
[...]

quoted

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 add=

r)

quoted

quoted

quoted

 	return false;
 }
=20
+bool is_valid_kprobe_symbol_name(const char *name)

=20
This just check the length of symbol_name buffer, and can contain
some invalid chars.

=20
Yes, I kept the function name generic incase we would like to do more=20
validation in future, plus it's shorter than=20
is_valid_kprobe_symbol_name_len() ;-)

=20
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=20
intent is to perform such validation right when we first obtain the=20
input from the user. In this case, however, kallsyms_lookup_name() is=20
also an exported symbol, so I do think some validation there would be=20
good to have as well.

=20

quoted

quoted

quoted

+{
+	size_t sym_len;
+	char *s;
+
+	s =3D strchr(name, ':');

=20
Hmm.. this should be strnchr(). I re-factored the code that moved the=20
strnlen() above this below. I'll fix this.
=20

quoted

quoted

+	if (s) {
+		sym_len =3D strnlen(s+1, KSYM_NAME_LEN);

=20
If you use strnlen() here, you just need to ensure sym_len < KSYM_NAME=

_LEN.

quoted

=20
Hmm.. not sure I follow. Are you saying the check for sym_len <=3D 0 is=20
not needed?

=20
You can check sym_len !=3D 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 !=3D NULL at first?)
=20
So, what I think it can be;
=20
if (strnlen(s+1, KSYM_NAME_LEN) =3D=3D KSYM_NAME_LEN ||
    (size_t)(s - name) >=3D MODULE_NAME_LEN)
	return false;

Sure, thanks. I clearly need to refactor this code better!

- Naveen

=