[PATCH] powerpc/kprobes: refactor kprobe_lookup_name for safer string operations

[PATCH] powerpc/kprobes: refactor kprobe_lookup_name for safer string operations

[Naveen N. Rao]

Use safer string manipulation functions when dealing with a
user-provided string in kprobe_lookup_name().

Reported-by: David Laight <David.Laight@ACULAB.COM>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 47 ++++++++++++++++++-------------------------
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 160ae0fa7d0d..5a17e6467be9 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -53,7 +53,7 @@ bool arch_within_kprobe_blacklist(unsigned long addr)
 
 kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
 {
-	kprobe_opcode_t *addr;
+	kprobe_opcode_t *addr = NULL;
 
 #ifdef PPC64_ELF_ABI_v2
 	/* PPC64 ABIv2 needs local entry point */
@@ -85,36 +85,29 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
 	 * Also handle <module:symbol> format.
 	 */
 	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
-	const char *modsym;
 	bool dot_appended = false;
-	if ((modsym = strchr(name, ':')) != NULL) {
-		modsym++;
-		if (*modsym != '\0' && *modsym != '.') {
-			/* Convert to <module:.symbol> */
-			strncpy(dot_name, name, modsym - name);
-			dot_name[modsym - name] = '.';
-			dot_name[modsym - name + 1] = '\0';
-			strncat(dot_name, modsym,
-				sizeof(dot_name) - (modsym - name) - 2);
-			dot_appended = true;
-		} else {
-			dot_name[0] = '\0';
-			strncat(dot_name, name, sizeof(dot_name) - 1);
-		}
-	} else if (name[0] != '.') {
-		dot_name[0] = '.';
-		dot_name[1] = '\0';
-		strncat(dot_name, name, KSYM_NAME_LEN - 2);
+	const char *c;
+	ssize_t ret = 0;
+	int len = 0;
+
+	if ((c = strnchr(name, MODULE_NAME_LEN, ':')) != NULL) {
+		c++;
+		len = c - name;
+		memcpy(dot_name, name, len);
+	} else
+		c = name;
+
+	if (*c != '\0' && *c != '.') {
+		dot_name[len++] = '.';
 		dot_appended = true;
-	} else {
-		dot_name[0] = '\0';
-		strncat(dot_name, name, KSYM_NAME_LEN - 1);
 	}
-	addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
-	if (!addr && dot_appended) {
-		/* Let's try the original non-dot symbol lookup	*/
+	ret = strscpy(dot_name + len, c, KSYM_NAME_LEN);
+	if (ret >= 0)
+		addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
+
+	/* Fallback to the original non-dot symbol lookup */
+	if (!addr && dot_appended)
 		addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
-	}
 #else
 	addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
 #endif
-- 
2.12.1

Re: [PATCH] powerpc/kprobes: refactor kprobe_lookup_name for safer string operations

[Masami Hiramatsu]

On Tue, 25 Apr 2017 21:37:11 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> Use safer string manipulation functions when dealing with a
> user-provided string in kprobe_lookup_name().
> 
> Reported-by: David Laight <David.Laight@ACULAB.COM>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/kprobes.c | 47 ++++++++++++++++++-------------------------
>  1 file changed, 20 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 160ae0fa7d0d..5a17e6467be9 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -53,7 +53,7 @@ bool arch_within_kprobe_blacklist(unsigned long addr)
>  
>  kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
>  {
> -	kprobe_opcode_t *addr;
> +	kprobe_opcode_t *addr = NULL;
>  
>  #ifdef PPC64_ELF_ABI_v2
>  	/* PPC64 ABIv2 needs local entry point */
> @@ -85,36 +85,29 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
>  	 * Also handle <module:symbol> format.
>  	 */
>  	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
> -	const char *modsym;
>  	bool dot_appended = false;
> -	if ((modsym = strchr(name, ':')) != NULL) {
> -		modsym++;
> -		if (*modsym != '\0' && *modsym != '.') {
> -			/* Convert to <module:.symbol> */
> -			strncpy(dot_name, name, modsym - name);
> -			dot_name[modsym - name] = '.';
> -			dot_name[modsym - name + 1] = '\0';
> -			strncat(dot_name, modsym,
> -				sizeof(dot_name) - (modsym - name) - 2);
> -			dot_appended = true;
> -		} else {
> -			dot_name[0] = '\0';
> -			strncat(dot_name, name, sizeof(dot_name) - 1);
> -		}
> -	} else if (name[0] != '.') {
> -		dot_name[0] = '.';
> -		dot_name[1] = '\0';
> -		strncat(dot_name, name, KSYM_NAME_LEN - 2);
> +	const char *c;
> +	ssize_t ret = 0;
> +	int len = 0;
> +
> +	if ((c = strnchr(name, MODULE_NAME_LEN, ':')) != NULL) {
> +		c++;
> +		len = c - name;
> +		memcpy(dot_name, name, len);
> +	} else
> +		c = name;
> +
> +	if (*c != '\0' && *c != '.') {
> +		dot_name[len++] = '.';
>  		dot_appended = true;
> -	} else {
> -		dot_name[0] = '\0';
> -		strncat(dot_name, name, KSYM_NAME_LEN - 1);
>  	}
> -	addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
> -	if (!addr && dot_appended) {
> -		/* Let's try the original non-dot symbol lookup	*/
> +	ret = strscpy(dot_name + len, c, KSYM_NAME_LEN);
> +	if (ret >= 0)

Here, maybe you can skip the case of ret == 0. (Or, would we have
a symbol which only has "."?)

Others look good to me.

Thank you,

> +		addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
> +
> +	/* Fallback to the original non-dot symbol lookup */
> +	if (!addr && dot_appended)
>  		addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
> -	}
>  #else
>  	addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
>  #endif
> -- 
> 2.12.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] powerpc/kprobes: refactor kprobe_lookup_name for safer string operations

[Naveen N. Rao]

Excerpts from Masami Hiramatsu's message of April 26, 2017 10:11:
> On Tue, 25 Apr 2017 21:37:11 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>=20
>> Use safer string manipulation functions when dealing with a
>> user-provided string in kprobe_lookup_name().
>>=20
>> Reported-by: David Laight <David.Laight@ACULAB.COM>
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/kernel/kprobes.c | 47 ++++++++++++++++++------------------=
-------
>>  1 file changed, 20 insertions(+), 27 deletions(-)
>>=20
>> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes=
.c
>> index 160ae0fa7d0d..5a17e6467be9 100644
>> --- a/arch/powerpc/kernel/kprobes.c
>> +++ b/arch/powerpc/kernel/kprobes.c
>> @@ -53,7 +53,7 @@ bool arch_within_kprobe_blacklist(unsigned long addr)
>> =20
>>  kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offs=
et)
>>  {
>> -	kprobe_opcode_t *addr;
>> +	kprobe_opcode_t *addr =3D NULL;
>> =20
>>  #ifdef PPC64_ELF_ABI_v2
>>  	/* PPC64 ABIv2 needs local entry point */
>> @@ -85,36 +85,29 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name=
, unsigned int offset)
>>  	 * Also handle <module:symbol> format.
>>  	 */
>>  	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
>> -	const char *modsym;
>>  	bool dot_appended =3D false;
>> -	if ((modsym =3D strchr(name, ':')) !=3D NULL) {
>> -		modsym++;
>> -		if (*modsym !=3D '\0' && *modsym !=3D '.') {
>> -			/* Convert to <module:.symbol> */
>> -			strncpy(dot_name, name, modsym - name);
>> -			dot_name[modsym - name] =3D '.';
>> -			dot_name[modsym - name + 1] =3D '\0';
>> -			strncat(dot_name, modsym,
>> -				sizeof(dot_name) - (modsym - name) - 2);
>> -			dot_appended =3D true;
>> -		} else {
>> -			dot_name[0] =3D '\0';
>> -			strncat(dot_name, name, sizeof(dot_name) - 1);
>> -		}
>> -	} else if (name[0] !=3D '.') {
>> -		dot_name[0] =3D '.';
>> -		dot_name[1] =3D '\0';
>> -		strncat(dot_name, name, KSYM_NAME_LEN - 2);
>> +	const char *c;
>> +	ssize_t ret =3D 0;
>> +	int len =3D 0;
>> +
>> +	if ((c =3D strnchr(name, MODULE_NAME_LEN, ':')) !=3D NULL) {
>> +		c++;
>> +		len =3D c - name;
>> +		memcpy(dot_name, name, len);
>> +	} else
>> +		c =3D name;
>> +
>> +	if (*c !=3D '\0' && *c !=3D '.') {
>> +		dot_name[len++] =3D '.';
>>  		dot_appended =3D true;
>> -	} else {
>> -		dot_name[0] =3D '\0';
>> -		strncat(dot_name, name, KSYM_NAME_LEN - 1);
>>  	}
>> -	addr =3D (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
>> -	if (!addr && dot_appended) {
>> -		/* Let's try the original non-dot symbol lookup	*/
>> +	ret =3D strscpy(dot_name + len, c, KSYM_NAME_LEN);
>> +	if (ret >=3D 0)
>=20
> Here, maybe you can skip the case of ret =3D=3D 0. (Or, would we have
> a symbol which only has "."?)

Ah, indeed. Good point. We just need the test to be (ret > 0).

Michael,
If the rest of the patch is fine by you, would you be ok to make the=20
small change above? If not, please let me know and I'll re-spin. Thanks.

>=20
> Others look good to me.

Thanks for the review,
- Naveen

=

Re: [PATCH] powerpc/kprobes: refactor kprobe_lookup_name for safer string operations

[Michael Ellerman]

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> Excerpts from Masami Hiramatsu's message of April 26, 2017 10:11:
>> On Tue, 25 Apr 2017 21:37:11 +0530
>> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>>> -	addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
>>> -	if (!addr && dot_appended) {
>>> -		/* Let's try the original non-dot symbol lookup	*/
>>> +	ret = strscpy(dot_name + len, c, KSYM_NAME_LEN);
>>> +	if (ret >= 0)
>> 
>> Here, maybe you can skip the case of ret == 0. (Or, would we have
>> a symbol which only has "."?)
>
> Ah, indeed. Good point. We just need the test to be (ret > 0).
>
> Michael,
> If the rest of the patch is fine by you, would you be ok to make the 
> small change above? If not, please let me know and I'll re-spin. Thanks.

I'd rather you change it, test and then resend.

cheers