LinuxLists.cc - [PATCH] kprobes: be more permissive when user specifies both symbol name and address

2014-04-14 10:41:08

Subject: [PATCH] kprobes: be more permissive when user specifies both symbol name and address

Currently, if user specifies both symbol name and address, we just
bail out.

This might be too rude. This patch makes it give more tolerance.
If both are specified, let symbol name take precedence; upon failure,
try address. And print a warning message if user specify an address
to inform him that using symbol is more preferred.

Signed-off-by: Jianyu Zhan <[email protected]>
---
Documentation/kprobes.txt | 4 +++-
kernel/kprobes.c | 33 +++++++++++++++++++++++++++++----
2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
index 0cfb00f..ecf901b 100644
--- a/Documentation/kprobes.txt
+++ b/Documentation/kprobes.txt
@@ -344,7 +344,9 @@ to install a probepoint is known. This field is used to calculate the
probepoint.

3. Specify either the kprobe "symbol_name" OR the "addr". If both are
-specified, kprobe registration will fail with -EINVAL.
+specified, searching for "symbol_name" takes precedence; upon failure,
+then try "addr". If neither is specified, kprobe registration will fail
+with -EINVAL.

4. With CISC architectures (such as i386 and x86_64), the kprobes code
does not validate if the kprobe.addr is at an instruction boundary.
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index ceeadfc..2444a7a 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1354,17 +1354,42 @@ static int __kprobes in_kprobes_functions(unsigned long addr)
static kprobe_opcode_t __kprobes *kprobe_addr(struct kprobe *p)
{
kprobe_opcode_t *addr = p->addr;
+ char namebuf[KSYM_NAME_LEN];
+ const char *sym_name = NULL;

- if ((p->symbol_name && p->addr) ||
- (!p->symbol_name && !p->addr))
+ if (!p->symbol_name && !p->addr)
goto invalid;

if (p->symbol_name) {
kprobe_lookup_name(p->symbol_name, addr);
- if (!addr)
- return ERR_PTR(-ENOENT);
+ if (addr) {
+ if (p->addr && p->addr != addr)
+ printk(KERN_WARNING "Warning: kprobe adrress for %s "
+ "mismatch, should be %p, not %p.\n",
+ p->symbol_name, addr, p->addr);
+ goto found;
+ }
+ }
+ if (p->addr) {
+ printk(KERN_WARNING "Warning: kprobes symbol name is now supported."
+ "Please use it instead.\n");
+ sym_name = kallsyms_lookup((unsigned long)(p->addr),
+ NULL, NULL, NULL, namebuf);
+ if (sym_name) {
+ if (p->symbol_name && strncmp(sym_name, p->symbol_name,
+ KSYM_NAME_LEN))
+ printk(KERN_WARNING "Warning: found %s at addres "
+ "%p, but not %s.\n",
+ p->symbol_name, addr, sym_name);
+
+ addr = p->addr;
+ goto found;
+ }
+
}
+ return ERR_PTR(-ENOENT);

+found:
addr = (kprobe_opcode_t *)(((char *)addr) + p->offset);
if (addr)
return addr;
--
1.9.0.GIT

2014-04-14 15:00:27

by 平松雅巳 / HIRAMATU，MASAMI

[permalink] [raw]

Subject: Re: [PATCH] kprobes: be more permissive when user specifies both symbol name and address

(2014/04/14 19:40), Jianyu Zhan wrote:
> Currently, if user specifies both symbol name and address, we just
> bail out.
>
> This might be too rude. This patch makes it give more tolerance.
> If both are specified, let symbol name take precedence; upon failure,
> try address. And print a warning message if user specify an address
> to inform him that using symbol is more preferred.

No, please don't do such thing. Using addr and symbol for double checking
is good, but if user sets "func1" and if it is not found, it should be
an error.
This is for the fail-safe and foolproof principle.
See below comments too.

> Signed-off-by: Jianyu Zhan <[email protected]>
> ---
> Documentation/kprobes.txt | 4 +++-
> kernel/kprobes.c | 33 +++++++++++++++++++++++++++++----
> 2 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
> index 0cfb00f..ecf901b 100644
> --- a/Documentation/kprobes.txt
> +++ b/Documentation/kprobes.txt
> @@ -344,7 +344,9 @@ to install a probepoint is known. This field is used to calculate the
> probepoint.
>
> 3. Specify either the kprobe "symbol_name" OR the "addr". If both are
> -specified, kprobe registration will fail with -EINVAL.
> +specified, searching for "symbol_name" takes precedence; upon failure,
> +then try "addr". If neither is specified, kprobe registration will fail
> +with -EINVAL.
>
> 4. With CISC architectures (such as i386 and x86_64), the kprobes code
> does not validate if the kprobe.addr is at an instruction boundary.
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index ceeadfc..2444a7a 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1354,17 +1354,42 @@ static int __kprobes in_kprobes_functions(unsigned long addr)
> static kprobe_opcode_t __kprobes *kprobe_addr(struct kprobe *p)
> {
> kprobe_opcode_t *addr = p->addr;
> + char namebuf[KSYM_NAME_LEN];
> + const char *sym_name = NULL;
>
> - if ((p->symbol_name && p->addr) ||
> - (!p->symbol_name && !p->addr))
> + if (!p->symbol_name && !p->addr)
> goto invalid;
>
> if (p->symbol_name) {
> kprobe_lookup_name(p->symbol_name, addr);
> - if (!addr)
> - return ERR_PTR(-ENOENT);
> + if (addr) {
> + if (p->addr && p->addr != addr)
> + printk(KERN_WARNING "Warning: kprobe adrress for %s "
> + "mismatch, should be %p, not %p.\n",
> + p->symbol_name, addr, p->addr);
> + goto found;
> + }

This should be an error.

> + }
> + if (p->addr) {
> + printk(KERN_WARNING "Warning: kprobes symbol name is now supported."
> + "Please use it instead.\n");

No, don't put this warning. Since the local symbols can have same name in the
kallsyms, sometimes p->addr is the only solution. From the same reason, if you
need a double check, you should check p->addr first. sometimes kprobe_lookup_name()
may return unwilling address (same symbol but another instance).

So the correct double-check should be;

----
if (p->addr) {
if (p->symbol) {
sym = kallsyms_lookup(p->addr, ... &offs ...);
if (strcmp(sym,p->symbol) != 0 || offs != p->offset) {
pr_warning("Error! ...");
goto fail;
}
}
} else if (p->symbol) {
kprobe_lookup_name(p->symbol_name, addr);
if (!addr)
goto fail;
} else
goto fail;
----

> + sym_name = kallsyms_lookup((unsigned long)(p->addr),
> + NULL, NULL, NULL, namebuf);
> + if (sym_name) {
> + if (p->symbol_name && strncmp(sym_name, p->symbol_name,
> + KSYM_NAME_LEN))
> + printk(KERN_WARNING "Warning: found %s at addres "
> + "%p, but not %s.\n",
> + p->symbol_name, addr, sym_name);
> +
> + addr = p->addr;

Here, we also treat this as an error.

> + goto found;
> + }
> +
> }
> + return $B#c(B
>
> +found:
> addr = (kprobe_opcode_t *)(((char *)addr) + p->offset);
> if (addr)
> return addr;
>

Thank you,

--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

2014-04-14 15:08:59

by 平松雅巳 / HIRAMATU，MASAMI

[permalink] [raw]

Subject: Re: [PATCH] kprobes: be more permissive when user specifies both symbol name and address

2014-04-15 08:12:00

by Jianyu Zhan

[permalink] [raw]

Subject: Re: [PATCH] kprobes: be more permissive when user specifies both symbol name and address

On Mon, Apr 14, 2014 at 11:00 PM, Masami Hiramatsu
<[email protected]> wrote:
> if (p->addr) {
> if (p->symbol) {
> sym = kallsyms_lookup(p->addr, ... &offs ...);
> if (strcmp(sym,p->symbol) != 0 || offs != p->offset) {
> pr_warning("Error! ...");
> goto fail;
> }
> }
> } else if (p->symbol) {
> kprobe_lookup_name(p->symbol_name, addr);
> if (!addr)
> goto fail;
> } else
> goto fail;

Hmm, let's clasify all conditions.

1. Only symbol, check it, if not found, fail.
2. Only address, check it, if not found, fail.
3. Both, check address,
3.1 not found, fail, because some symbols might have muplitple instances,
we don't bother to check symbol name.
3.2 found, check if symbol mismatch, if yes, fail.

Is this reasonable? Next mail is a renewed patch following this priciple.

Regards,
Jianyu Zhan

2014-04-15 08:27:15

by 平松雅巳 / HIRAMATU，MASAMI

[permalink] [raw]

Subject: Re: [PATCH] kprobes: be more permissive when user specifies both symbol name and address

(2014/04/15 17:11), Zhan Jianyu wrote:
> On Mon, Apr 14, 2014 at 11:00 PM, Masami Hiramatsu
> <[email protected]> wrote:
>> if (p->addr) {
>> if (p->symbol) {
>> sym = kallsyms_lookup(p->addr, ... &offs ...);
>> if (strcmp(sym,p->symbol) != 0 || offs != p->offset) {
>> pr_warning("Error! ...");
>> goto fail;
>> }
>> }
>> } else if (p->symbol) {
>> kprobe_lookup_name(p->symbol_name, addr);
>> if (!addr)
>> goto fail;
>> } else
>> goto fail;
>
>
> Hmm, let's clasify all conditions.
>
> 1. Only symbol, check it, if not found, fail.
> 2. Only address, check it, if not found, fail.
> 3. Both, check address,
> 3.1 not found, fail, because some symbols might have muplitple instances,
> we don't bother to check symbol name.
> 3.2 found, check if symbol mismatch, if yes, fail.

Plus, if the p->offset and offs are different, fail too.

> Is this reasonable? Next mail is a renewed patch following this priciple.

OK, let me see. :)

Thank you,

--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

2014-04-15 08:33:47

by Jianyu Zhan

[permalink] [raw]

Subject: Re: [PATCH] kprobes: be more permissive when user specifies both symbol name and address

On Tue, Apr 15, 2014 at 4:27 PM, Masami Hiramatsu
<[email protected]> wrote:
>
> Plus, if the p->offset and offs are different, fail too.

Oh, yeah, I have did this check it patch too. BTW, I found that the
offset has different types
in kallsyms_lookup(which is unsigned long) and in struct kprobe(which
is unsigned int)

Regards,
Jianyu Zhan