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

[PATCH v2] 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>
---
Changed to ignore return value of 0 from strscpy(), as suggested by
Masami.

- Naveen

 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..255d28d31ca1 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.2

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

[David Laight]

From: Naveen N. Rao [mailto:naveen.n.rao@linux.vnet.ibm.com]
> Sent: 04 May 2017 11:25
> 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>
> ---
> Changed to ignore return value of 0 from strscpy(), as suggested by
> Masami.

let's see what this code looks like;

>  	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
>  	bool dot_appended =3D false;
> +	const char *c;
> +	ssize_t ret =3D 0;
> +	int len =3D 0;
> +
> +	if ((c =3D strnchr(name, MODULE_NAME_LEN, ':')) !=3D NULL) {

I don't like unnecessary assignments in conditionals.

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

If you don't append a dot, then you can always just lookup
the original string.

>  	}
> +	ret =3D strscpy(dot_name + len, c, KSYM_NAME_LEN);
> +	if (ret > 0)
> +		addr =3D (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);

I'm not sure you need 'ret' here at all.

> +	/* Fallback to the original non-dot symbol lookup */
> +	if (!addr && dot_appended)
>  		addr =3D (kprobe_opcode_t *)kallsyms_lookup_name(name);

We can bikeshed this function to death:

	/* The function name must start with a '.'.
	 * If it doesn't then we insert one. */
	c =3D strnchr(name, MODULE_NAME_LEN, ':');
	if (c && c[1] && c[1] !=3D '.') {
		/* Insert a '.' after the ':' */
		c++;
		len =3D c - name;
		memcpy(dot_name, name, len);
	} else {
		if (name[0] =3D=3D '.')
			goto check_name:
		/* Insert a '.' before name */
		c =3D name;
		len =3D 0;
	}

	dot_name[len++] =3D '.';
	if (strscpy(dot_name + len, c, KSYM_NAME_LEN) > 0) {
		addr =3D (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
		if (addr)
			return addr;
	}
	/* Symbol with extra '.' not found, fallback to original name */

check_name:
	return (kprobe_opcode_t *)kallsyms_lookup_name(name);

	David

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

[Paul Clarke]

On 05/04/2017 05:24 AM, Naveen N. Rao 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>
> ---
> Changed to ignore return value of 0 from strscpy(), as suggested by
> Masami.
> 
> - Naveen
> 
>  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..255d28d31ca1 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) {

Shouldn't this be MODULE_NAME_LEN + 1, since the ':' can come after a module name of length MODULE_NAME_LEN?

> +		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);
>  	}

PC

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

[David Laight]

From: Paul Clarke
> Sent: 04 May 2017 16:07
...
> > +	if ((c =3D strnchr(name, MODULE_NAME_LEN, ':')) !=3D NULL) {
>=20
> Shouldn't this be MODULE_NAME_LEN + 1, since the ':' can come after a mod=
ule name of length
> MODULE_NAME_LEN?

No, because MODULE_NAME_LEN includes the terminating '\0'.

	David

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

['Naveen N. Rao']

On 2017/05/04 12:45PM, David Laight wrote:
> From: Naveen N. Rao [mailto:naveen.n.rao@linux.vnet.ibm.com]
> > Sent: 04 May 2017 11:25
> > 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>
> > ---
> > Changed to ignore return value of 0 from strscpy(), as suggested by
> > Masami.
> 
> let's see what this code looks like;
> 
> >  	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
> >  	bool dot_appended = false;
> > +	const char *c;
> > +	ssize_t ret = 0;
> > +	int len = 0;
> > +
> > +	if ((c = strnchr(name, MODULE_NAME_LEN, ':')) != NULL) {
> 
> I don't like unnecessary assignments in conditionals.
> 
> > +		c++;
> > +		len = c - name;
> > +		memcpy(dot_name, name, len);
> > +	} else
> > +		c = name;
> > +
> > +	if (*c != '\0' && *c != '.') {
> > +		dot_name[len++] = '.';
> >  		dot_appended = true;
> 
> If you don't append a dot, then you can always just lookup
> the original string.
> 
> >  	}
> > +	ret = strscpy(dot_name + len, c, KSYM_NAME_LEN);
> > +	if (ret > 0)
> > +		addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
> 
> I'm not sure you need 'ret' here at all.
> 
> > +	/* Fallback to the original non-dot symbol lookup */
> > +	if (!addr && dot_appended)
> >  		addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
> 
> We can bikeshed this function to death:

Sure, though that was never the intent. For me, this was all about 
ensuring safe string manipulation. And I suppose my patch and your 
version both achieve that.

> 
> 	/* The function name must start with a '.'.
> 	 * If it doesn't then we insert one. */
> 	c = strnchr(name, MODULE_NAME_LEN, ':');
> 	if (c && c[1] && c[1] != '.') {
		 ^^^^^^^
		 while we're here, I might as well point out that that's 
		 un-necessary and probably a few more things below... 
		 ;-)

- Naveen

> 		/* Insert a '.' after the ':' */
> 		c++;
> 		len = c - name;
> 		memcpy(dot_name, name, len);
> 	} else {
> 		if (name[0] == '.')
> 			goto check_name:
> 		/* Insert a '.' before name */
> 		c = name;
> 		len = 0;
> 	}
> 
> 	dot_name[len++] = '.';
> 	if (strscpy(dot_name + len, c, KSYM_NAME_LEN) > 0) {
> 		addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
> 		if (addr)
> 			return addr;
> 	}
> 	/* Symbol with extra '.' not found, fallback to original name */
> 
> check_name:
> 	return (kprobe_opcode_t *)kallsyms_lookup_name(name);
> 
> 	David
>