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@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 
> > Signed-off-by: Naveen N. Rao 
> > ---
> > 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
> 

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 = 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?

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

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 
> Signed-off-by: Naveen N. Rao 
> ---
> 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  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  */
> - 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: Naveen N. Rao [mailto:naveen.n@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 
> Signed-off-by: Naveen N. Rao 
> ---
> 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:

/* 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] != '.') {
/* 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

[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 
Signed-off-by: Naveen N. Rao 
---
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  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  */
-   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