linux-kernel - Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during probe registration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1492781012.pcpyah5v9j.astroid@naverao1-tp.none>
Date:   Fri, 21 Apr 2017 13:25:00 +0000
From:   "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
To:     Masami Hiramatsu <mhiramat@...nel.org>,
        Michael Ellerman <mpe@...erman.id.au>, pc@...ibm.com
Cc:     linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH v4 3/7] kprobes: validate the symbol name provided during
 probe registration

Excerpts from Paul Clarke's message of April 21, 2017 18:41:
> a nit or two, below...
> 
> On 04/21/2017 07:32 AM, Naveen N. Rao wrote:
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index 6a128f3a7ed1..ff9b1ac72a38 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -1383,6 +1383,34 @@ bool within_kprobe_blacklist(unsigned long addr)
>>  }
>> 
>>  /*
>> + * We mainly want to ensure that the provided string is of a reasonable length
>> + * and is of the form [<mod_name>:]<sym_name>, so that this is safe to process
>> + * further.
>> + * We don't worry about invalid characters as those will just prevent
>> + * matching existing kallsyms.
>> + */
>> +bool is_valid_kprobe_symbol_name(const char *name)
>> +{
>> +	size_t sym_len;
>> +	const char *s;
>> +
>> +	s = strnchr(name, ':', MODULE_NAME_LEN + KSYM_NAME_LEN + 1);
>> +	if (s) {
>> +		sym_len = (size_t)(s - name);
>> +		if (sym_len <= 0  || sym_len >= MODULE_NAME_LEN)
> 
> "sym_len <= 0" looks odd here, since sym_len is likely unsigned and would never be less than zero, anyway.

Ugh.. habits :/
I'll wait for Masami/Michael's feedback before re-spinning.

Thanks for the review,
- Naveen