[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 at 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 at ACULAB.COM>
> > Signed-off-by: Naveen N. Rao <naveen.n.rao at 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
>