diff mbox series

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

Message ID 88995231cc2266dd11fac018bb2027bfbf4d255b.1508776485.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive)
State Accepted
Commit acdfe93101d0f5b968d1bd7ab125517bf7444047
Headers show
Series [1/4] powerpc/kprobes: Disable preemption before invoking probe handler for optprobes | expand

Commit Message

Naveen N. Rao Oct. 23, 2017, 4:37 p.m. UTC 
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(-)

Comments

Masami Hiramatsu (Google) Oct. 25, 2017, 4:35 p.m. UTC | #1 
On Mon, 23 Oct 2017 22:07:41 +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().
> 

What would you mean "safer" here? using strnchr()?
Could you please show at least an example case that causes problem in original code.
And have one comment below:

> 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 a14c61855705..bbb4795660f8 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;

It is odd, you can call kallsyms_lookup_name(dot_name) here.

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

Then we can remove dot_appended.

Thank you,

> -	}
>  #else
>  	addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
>  #endif
> -- 
> 2.14.2
>
Naveen N. Rao Oct. 27, 2017, 11:34 a.m. UTC | #2 
On 2017/10/25 04:35PM, Masami Hiramatsu wrote:
> On Mon, 23 Oct 2017 22:07:41 +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().
> > 
> 
> What would you mean "safer" here? using strnchr()?
> Could you please show at least an example case that causes problem in original code.

Yes, essentially about using bounded string operations. Since the 'name' 
parameter is provided by user, it's better to ensure it is within 
limits. Granted, this isn't a big deal since user needs to be root for 
accessing this API, but we felt it is good to clean up this code.

> And have one comment below:
> 
> > 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 a14c61855705..bbb4795660f8 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;
> 
> It is odd, you can call kallsyms_lookup_name(dot_name) here.

We'll then have to duplicate the strscpy() here.

Thanks for the review,
- Naveen

> 
> > -	} 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);
> 
> Then we can remove dot_appended.
> 
> Thank you,
> 
> > -	}
> >  #else
> >  	addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
> >  #endif
> > -- 
> > 2.14.2
> > 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>
>
diff mbox series

Patch

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index a14c61855705..bbb4795660f8 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