Re: [PATCH 4/4] powerpc/kprobes: refactor kprobe_lookup_name for safer string operations
On 2017/10/25 04:35PM, Masami Hiramatsu wrote:
> On Mon, 23 Oct 2017 22:07:41 +0530
> "Naveen N. Rao" 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
> > Signed-off-by: Naveen N. Rao
> > ---
> > 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 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;
>
> 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
>
Re: [PATCH 4/4] powerpc/kprobes: refactor kprobe_lookup_name for safer string operations
On Mon, 23 Oct 2017 22:07:41 +0530
"Naveen N. Rao" 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.
And have one comment below:
> Reported-by: David Laight
> Signed-off-by: Naveen N. Rao
> ---
> 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 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;
It is odd, you can call kallsyms_lookup_name(dot_name) here.
> - } 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
[PATCH 4/4] powerpc/kprobes: refactor kprobe_lookup_name for safer string operations
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
---
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 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.14.2
