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

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

[Jianyu Zhan]

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, check address first, if the symbol found
does not match the one user specify, print a waring. If not found,
return -ENOENT, because some symbols might have muplitple instances,
we don't bother to check symbol name.

Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
---
 Documentation/kprobes.txt |  4 +++-
 kernel/kprobes.c          | 30 ++++++++++++++++++++++++++----
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
index 0cfb00f..217f976 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, only check "addr", because some symbols might have muplitple
+instances. 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..ac910f4 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1354,17 +1354,39 @@ 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;
+	unsigned long offset;
 
-	if ((p->symbol_name && p->addr) ||
-	    (!p->symbol_name && !p->addr))
+	if (!p->symbol_name && !p->addr)
 		goto invalid;
 
-	if (p->symbol_name) {
+	/* Some symbols might have muplitple instances,
+	 * so if both specified, only check address. */
+	if (unlikely(p->addr && p->symbol_name)) {
+		sym_name = kallsyms_lookup((unsigned long)(p->addr),
+				NULL, &offset, NULL, namebuf);
+		if (!sym_name)
+			return ERR_PTR(-ENOENT);
+
+		if (strncmp(sym_name, p->symbol_name, KSYM_NAME_LEN)
+				|| offset != p->offset) {
+			pr_err("Incorrect symbol or offset, should be "
+				"symbol=%s, offset=%ld.\n", sym_name, offset);
+			goto invalid;
+		}
+	} else if (p->symbol_name) {
+		/* only symbol case */
 		kprobe_lookup_name(p->symbol_name, addr);
 		if (!addr)
 			return ERR_PTR(-ENOENT);
+	} else {
+		/* only address case */
+		sym_name = kallsyms_lookup((unsigned long)(p->addr),
+				NULL, &offset, NULL, namebuf);
+		if (!sym_name)
+			return ERR_PTR(-ENOENT);
 	}
-
 	addr = (kprobe_opcode_t *)(((char *)addr) + p->offset);
 	if (addr)
 		return addr;
-- 
1.9.0.GIT

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

[Masami Hiramatsu]

(2014/04/15 17:10), 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, check address first, if the symbol found
> does not match the one user specify, print a waring. If not found,
> return -ENOENT, because some symbols might have muplitple instances,
> we don't bother to check symbol name.
> 
> Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
> ---
>  Documentation/kprobes.txt |  4 +++-
>  kernel/kprobes.c          | 30 ++++++++++++++++++++++++++----
>  2 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
> index 0cfb00f..217f976 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, only check "addr", because some symbols might have muplitple
> +instances. 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..ac910f4 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1354,17 +1354,39 @@ 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;
> +	unsigned long offset;
>  
> -	if ((p->symbol_name && p->addr) ||
> -	    (!p->symbol_name && !p->addr))
> +	if (!p->symbol_name && !p->addr)
>  		goto invalid;
>  
> -	if (p->symbol_name) {
> +	/* Some symbols might have muplitple instances,
> +	 * so if both specified, only check address. */

Could you fix the comment style as same as others?
If we have multiple lines of comment, it should be

/*
 * aaaaaa
 * bbbbbb
 */

> +	if (unlikely(p->addr && p->symbol_name)) {
> +		sym_name = kallsyms_lookup((unsigned long)(p->addr),
> +				NULL, &offset, NULL, namebuf);
> +		if (!sym_name)
> +			return ERR_PTR(-ENOENT);
> +
> +		if (strncmp(sym_name, p->symbol_name, KSYM_NAME_LEN)
> +				|| offset != p->offset) {
> +			pr_err("Incorrect symbol or offset, should be "
> +				"symbol=%s, offset=%ld.\n", sym_name, offset);
> +			goto invalid;
> +		}
> +	} else if (p->symbol_name) {
> +		/* only symbol case */
>  		kprobe_lookup_name(p->symbol_name, addr);
>  		if (!addr)
>  			return ERR_PTR(-ENOENT);
> +	} else {
> +		/* only address case */
> +		sym_name = kallsyms_lookup((unsigned long)(p->addr),
> +				NULL, &offset, NULL, namebuf);
> +		if (!sym_name)
> +			return ERR_PTR(-ENOENT);

Since we've already have a sanity check of the address range (in kernel_text)
in check_kprobe_address_safe(), you don't need to lookup kallsyms.

Thank you,


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

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

[Masami Hiramatsu]

Sorry for resending...

(2014/04/15 17:10), 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, check address first, if the symbol found
> does not match the one user specify, print a waring. If not found,
> return -ENOENT, because some symbols might have muplitple instances,
> we don't bother to check symbol name.
> 
> Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
> ---
>  Documentation/kprobes.txt |  4 +++-
>  kernel/kprobes.c          | 30 ++++++++++++++++++++++++++----
>  2 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
> index 0cfb00f..217f976 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, only check "addr", because some symbols might have muplitple
> +instances. 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..ac910f4 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1354,17 +1354,39 @@ 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;
> +	unsigned long offset;
>  
> -	if ((p->symbol_name && p->addr) ||
> -	    (!p->symbol_name && !p->addr))
> +	if (!p->symbol_name && !p->addr)
>  		goto invalid;
>  
> -	if (p->symbol_name) {
> +	/* Some symbols might have muplitple instances,
> +	 * so if both specified, only check address. */

Could you fix the comment style as same as others?
If we have multiple lines of comment, it should be

/*
 * aaaaaa
 * bbbbbb
 */

> +	if (unlikely(p->addr && p->symbol_name)) {
> +		sym_name = kallsyms_lookup((unsigned long)(p->addr),
> +				NULL, &offset, NULL, namebuf);
> +		if (!sym_name)
> +			return ERR_PTR(-ENOENT);
> +
> +		if (strncmp(sym_name, p->symbol_name, KSYM_NAME_LEN)
> +				|| offset != p->offset) {
> +			pr_err("Incorrect symbol or offset, should be "
> +				"symbol=%s, offset=%ld.\n", sym_name, offset);
> +			goto invalid;
> +		}
> +	} else if (p->symbol_name) {
> +		/* only symbol case */
>  		kprobe_lookup_name(p->symbol_name, addr);
>  		if (!addr)
>  			return ERR_PTR(-ENOENT);
> +	} else {
> +		/* only address case */
> +		sym_name = kallsyms_lookup((unsigned long)(p->addr),
> +				NULL, &offset, NULL, namebuf);
> +		if (!sym_name)
> +			return ERR_PTR(-ENOENT);

Since we've already have a sanity check of the address range (in kernel_text)
in check_kprobe_address_safe(), you don't need to lookup kallsyms.

Thank you,


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

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

[Jianyu Zhan]

Ok, I've followed your suggestion. Thanks :-)

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, check address first, if the symbol found
does not match the one user specify, print a waring. If not found,
return -ENOENT, because some symbols might have muplitple instances,
we don't bother to check symbol name.

Suggested-by: Masami Hiramatsu masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
---
 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..217f976 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, only check "addr", because some symbols might have muplitple
+instances. 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..6ebd456 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;
+	unsigned long offset;
 
-	if ((p->symbol_name && p->addr) ||
-	    (!p->symbol_name && !p->addr))
+	if (!p->symbol_name && !p->addr)
 		goto invalid;
 
-	if (p->symbol_name) {
+	/*
+	 * Some symbols might have muplitple instances,
+	 * so if both specified, only check address.
+	 */
+	if (unlikely(p->addr && p->symbol_name)) {
+		sym_name = kallsyms_lookup((unsigned long)(p->addr),
+				NULL, &offset, NULL, namebuf);
+		if (!sym_name)
+			return ERR_PTR(-ENOENT);
+
+		if (strncmp(sym_name, p->symbol_name, KSYM_NAME_LEN)
+				|| offset != p->offset) {
+			pr_err("Incorrect symbol or offset, should be "
+				"symbol=%s, offset=%ld.\n", sym_name, offset);
+			goto invalid;
+		}
+	} else if (p->symbol_name) {
+		/* Only symbol case */
 		kprobe_lookup_name(p->symbol_name, addr);
 		if (!addr)
 			return ERR_PTR(-ENOENT);
+	} else {
+		/*
+		 * Only address case.
+		 * Since we later will do sanity check of the
+		 * address range in check_kprobe_address_safe(),
+		 * do nothing here.
+		 */
 	}
-
 	addr = (kprobe_opcode_t *)(((char *)addr) + p->offset);
 	if (addr)
 		return addr;
-- 
1.9.0.GIT

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

[Jianyu Zhan]

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 <nasa4836@gmail.com>
---
 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

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

[Masami Hiramatsu]

(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 <nasa4836@gmail.com>
> ---
>  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 ｃ
>  
> +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: masami.hiramatsu.pt@hitachi.com

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

[Zhan Jianyu]

On Mon, Apr 14, 2014 at 11:00 PM, Masami Hiramatsu
<masami.hiramatsu.pt@hitachi.com> 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

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

[Masami Hiramatsu]

(2014/04/15 17:11), Zhan Jianyu wrote:
> On Mon, Apr 14, 2014 at 11:00 PM, Masami Hiramatsu
> <masami.hiramatsu.pt@hitachi.com> 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: masami.hiramatsu.pt@hitachi.com

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

[Zhan Jianyu]

On Tue, Apr 15, 2014 at 4:27 PM, Masami Hiramatsu
<masami.hiramatsu.pt@hitachi.com> 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

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

[Masami Hiramatsu]

(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 <nasa4836@gmail.com>
> ---
>  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 ｃ
>  
> +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: masami.hiramatsu.pt@hitachi.com