| Message ID | 20170425160711.15332-1-naveen.n.rao@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, 25 Apr 2017 21:37:11 +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(). > > 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 160ae0fa7d0d..5a17e6467be9 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) Here, maybe you can skip the case of ret == 0. (Or, would we have a symbol which only has "."?) Others look good to me. Thank you, > + 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.1 >
Excerpts from Masami Hiramatsu's message of April 26, 2017 10:11: > On Tue, 25 Apr 2017 21:37:11 +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(). >> >> 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 160ae0fa7d0d..5a17e6467be9 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) > > Here, maybe you can skip the case of ret == 0. (Or, would we have > a symbol which only has "."?) Ah, indeed. Good point. We just need the test to be (ret > 0). Michael, If the rest of the patch is fine by you, would you be ok to make the small change above? If not, please let me know and I'll re-spin. Thanks. > > Others look good to me. Thanks for the review, - Naveen
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes: > Excerpts from Masami Hiramatsu's message of April 26, 2017 10:11: >> On Tue, 25 Apr 2017 21:37:11 +0530 >> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: >>> - 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) >> >> Here, maybe you can skip the case of ret == 0. (Or, would we have >> a symbol which only has "."?) > > Ah, indeed. Good point. We just need the test to be (ret > 0). > > Michael, > If the rest of the patch is fine by you, would you be ok to make the > small change above? If not, please let me know and I'll re-spin. Thanks. I'd rather you change it, test and then resend. cheers
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c index 160ae0fa7d0d..5a17e6467be9 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
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(-)