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

[Naveen N. Rao]

On 2017/10/25 04:35PM, Masami Hiramatsu wrote:
> On Mon, 23 Oct 2017 22:07:41 +0530
> "Naveen N. Rao" <naveen.n.rao at 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 at ACULAB.COM>
> > Signed-off-by: Naveen N. Rao <naveen.n.rao at 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 at kernel.org>
>