Re: [PATCH v4 4/7] powerpc/kprobes: Use safer string functions in kprobe_lookup_name()

[Paul Clarke]

On 04/21/2017 07:33 AM, Naveen N. Rao wrote:
> Convert usage of strchr()/strncpy()/strncat() to
> strnchr()/memcpy()/strlcat() for simpler and safer string manipulation.
> 
> Reported-by: David Laight <David.Laight@xxxxxxxxxx>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@xxxxxxxxxxxxxxxxxx>
> ---
> Changes: Additionally convert the strchr().
> 
> 
>  arch/powerpc/kernel/kprobes.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 97b5eed1f76d..c73fb6e3b43f 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -65,28 +65,27 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
>  	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
>  	const char *modsym;
>  	bool dot_appended = false;
> -	if ((modsym = strchr(name, ':')) != NULL) {
> +	if ((modsym = strnchr(name, ':', MODULE_NAME_LEN)) != NULL) {
>  		modsym++;
>  		if (*modsym != '\0' && *modsym != '.') {
>  			/* Convert to <module:.symbol> */
> -			strncpy(dot_name, name, modsym - name);
> +			memcpy(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);
> +			strlcat(dot_name, modsym, sizeof(dot_name));

Would it be more efficient here to replace this:
--
dot_name[modsym - name + 1] = '\0';
strlcat(dot_name, modsym, sizeof(dot_name));
--

with this:
strncpy(&dot_name[modsym - name + 1], modsym, KSYM_NAME_LEN);

(So you aren't rescanning dot_name to find the end, when you already know the end position?)

>  			dot_appended = true;
>  		} else {
>  			dot_name[0] = '\0';
> -			strncat(dot_name, name, sizeof(dot_name) - 1);
> +			strlcat(dot_name, name, sizeof(dot_name));

and here do:
strncpy(dot_name, name, sizeof(dot_name));

(and remove the null termination immediately above)

>  		}
>  	} else if (name[0] != '.') {
>  		dot_name[0] = '.';
>  		dot_name[1] = '\0';
> -		strncat(dot_name, name, KSYM_NAME_LEN - 2);
> +		strlcat(dot_name, name, sizeof(dot_name));

and here do:
strncpy(&dot_name[1], name, sizeof(dot_name));

(and remove the null termination immediately above)

>  		dot_appended = true;
>  	} else {
>  		dot_name[0] = '\0';
> -		strncat(dot_name, name, KSYM_NAME_LEN - 1);
> +		strlcat(dot_name, name, sizeof(dot_name));

and here do:
strncpy(dot_name, name, sizeof(dot_name));

(and remove the null termination immediately above)

>  	}

PC