[PATCH 2/2] kprobes: verify jprobe entry point

[PATCH 2/2] kprobes: verify jprobe entry point

[Namhyung Kim]

verify jprobe's entry point is a function entry point
using kallsyms' offset value.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 8f96701..c7295f9 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1334,19 +1334,25 @@ unsigned long __weak arch_deref_entry_point(void *entry)
 int __kprobes register_jprobes(struct jprobe **jps, int num)
 {
 	struct jprobe *jp;
-	int ret = 0, i;
+	int ret = -EINVAL, i;
 
 	if (num <= 0)
-		return -EINVAL;
+		return ret;
 	for (i = 0; i < num; i++) {
 		unsigned long addr;
+		unsigned long size, offset;
+		char namebuf[KSYM_NAME_LEN];
+
 		jp = jps[i];
 		addr = arch_deref_entry_point(jp->entry);
 
-		/* Todo: Verify probepoint is a function entry point */
-		jp->kp.pre_handler = setjmp_pre_handler;
-		jp->kp.break_handler = longjmp_break_handler;
-		ret = register_kprobe(&jp->kp);
+		/* Verify probepoint is a function entry point */
+		if (kallsyms_lookup(addr, &size, &offset, NULL, namebuf) &&
+		    offset = 0) {
+			jp->kp.pre_handler = setjmp_pre_handler;
+			jp->kp.break_handler = longjmp_break_handler;
+			ret = register_kprobe(&jp->kp);
+		}
 
 		if (ret < 0) {
 			if (i > 0)
-- 
1.7.0.4

Re: [PATCH 2/2] kprobes: verify jprobe entry point

[Masami Hiramatsu]

Namhyung Kim wrote:
> verify jprobe's entry point is a function entry point
> using kallsyms' offset value.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 8f96701..c7295f9 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1334,19 +1334,25 @@ unsigned long __weak arch_deref_entry_point(void *entry)
>  int __kprobes register_jprobes(struct jprobe **jps, int num)
>  {
>  	struct jprobe *jp;
> -	int ret = 0, i;
> +	int ret = -EINVAL, i;
>  
>  	if (num <= 0)
> -		return -EINVAL;
> +		return ret;
>  	for (i = 0; i < num; i++) {
>  		unsigned long addr;
> +		unsigned long size, offset;
> +		char namebuf[KSYM_NAME_LEN];
> +

You need to re-initialize "ret" in each iteration,
or "ret" will be 0 except for the 1st iteration.

>  		jp = jps[i];
>  		addr = arch_deref_entry_point(jp->entry);
>  
> -		/* Todo: Verify probepoint is a function entry point */
> -		jp->kp.pre_handler = setjmp_pre_handler;
> -		jp->kp.break_handler = longjmp_break_handler;
> -		ret = register_kprobe(&jp->kp);
> +		/* Verify probepoint is a function entry point */
> +		if (kallsyms_lookup(addr, &size, &offset, NULL, namebuf) &&
> +		    offset = 0) {
> +			jp->kp.pre_handler = setjmp_pre_handler;
> +			jp->kp.break_handler = longjmp_break_handler;
> +			ret = register_kprobe(&jp->kp);

		because, this line assigns 0 to ret if no error.

> +		}

I think here is a good point to do that. (in else block)

>  
>  		if (ret < 0) {
>  			if (i > 0)

Thank you,
-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 2/2] kprobes: verify jprobe entry point

[Namhyung Kim]

2010-08-03 (í™”), 20:27 +0900, Masami Hiramatsu:
> Namhyung Kim wrote:
> > verify jprobe's entry point is a function entry point
> > using kallsyms' offset value.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> > 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 8f96701..c7295f9 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -1334,19 +1334,25 @@ unsigned long __weak arch_deref_entry_point(void *entry)
> >  int __kprobes register_jprobes(struct jprobe **jps, int num)
> >  {
> >  	struct jprobe *jp;
> > -	int ret = 0, i;
> > +	int ret = -EINVAL, i;
> >  
> >  	if (num <= 0)
> > -		return -EINVAL;
> > +		return ret;
> >  	for (i = 0; i < num; i++) {
> >  		unsigned long addr;
> > +		unsigned long size, offset;
> > +		char namebuf[KSYM_NAME_LEN];
> > +
> 
> You need to re-initialize "ret" in each iteration,
> or "ret" will be 0 except for the 1st iteration.
> 
> >  		jp = jps[i];
> >  		addr = arch_deref_entry_point(jp->entry);
> >  
> > -		/* Todo: Verify probepoint is a function entry point */
> > -		jp->kp.pre_handler = setjmp_pre_handler;
> > -		jp->kp.break_handler = longjmp_break_handler;
> > -		ret = register_kprobe(&jp->kp);
> > +		/* Verify probepoint is a function entry point */
> > +		if (kallsyms_lookup(addr, &size, &offset, NULL, namebuf) &&
> > +		    offset = 0) {
> > +			jp->kp.pre_handler = setjmp_pre_handler;
> > +			jp->kp.break_handler = longjmp_break_handler;
> > +			ret = register_kprobe(&jp->kp);
> 
> 		because, this line assigns 0 to ret if no error.
> 
> > +		}
> 
> I think here is a good point to do that. (in else block)
> 
> >  
> >  		if (ret < 0) {
> >  			if (i > 0)
> 
> Thank you,

Thanks for reviewing.
I'll resend the fix soon after.


-- 
Regards,
Namhyung Kim


--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] kprobes: verify jprobe entry point

[Namhyung Kim]

verify jprobe's entry point is a function entry point
using kallsyms' offset value.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 8f96701..0032868 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1340,13 +1340,20 @@ int __kprobes register_jprobes(struct jprobe **jps, int num)
 		return -EINVAL;
 	for (i = 0; i < num; i++) {
 		unsigned long addr;
+		unsigned long size, offset;
+		char namebuf[KSYM_NAME_LEN];
+
 		jp = jps[i];
 		addr = arch_deref_entry_point(jp->entry);
 
-		/* Todo: Verify probepoint is a function entry point */
-		jp->kp.pre_handler = setjmp_pre_handler;
-		jp->kp.break_handler = longjmp_break_handler;
-		ret = register_kprobe(&jp->kp);
+		/* Verify probepoint is a function entry point */
+		if (kallsyms_lookup(addr, &size, &offset, NULL, namebuf) &&
+		    offset = 0) {
+			jp->kp.pre_handler = setjmp_pre_handler;
+			jp->kp.break_handler = longjmp_break_handler;
+			ret = register_kprobe(&jp->kp);
+		} else
+			ret = -EINVAL;
 
 		if (ret < 0) {
 			if (i > 0)
-- 
1.7.0.4

Re: [PATCH 2/2] kprobes: verify jprobe entry point

[Masami Hiramatsu]

Namhyung Kim wrote:
> verify jprobe's entry point is a function entry point
> using kallsyms' offset value.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 8f96701..0032868 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1340,13 +1340,20 @@ int __kprobes register_jprobes(struct jprobe **jps, int num)
>  		return -EINVAL;
>  	for (i = 0; i < num; i++) {
>  		unsigned long addr;
> +		unsigned long size, offset;
> +		char namebuf[KSYM_NAME_LEN];
> +
>  		jp = jps[i];
>  		addr = arch_deref_entry_point(jp->entry);
>  
> -		/* Todo: Verify probepoint is a function entry point */
> -		jp->kp.pre_handler = setjmp_pre_handler;
> -		jp->kp.break_handler = longjmp_break_handler;
> -		ret = register_kprobe(&jp->kp);
> +		/* Verify probepoint is a function entry point */
> +		if (kallsyms_lookup(addr, &size, &offset, NULL, namebuf) &&

Hm, it seems that current kernel has kallsyms_lookup_size_offset()
which doesn't requires namebuf, and you can ommit size if you don't
need that. It will simplify this patch:)

Other parts are OK for me.

Thank you,

> +		    offset = 0) {
> +			jp->kp.pre_handler = setjmp_pre_handler;
> +			jp->kp.break_handler = longjmp_break_handler;
> +			ret = register_kprobe(&jp->kp);
> +		} else
> +			ret = -EINVAL;
>  
>  		if (ret < 0) {
>  			if (i > 0)

-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

[PATCH 2/2] kprobes: verify jprobe entry point

[Namhyung Kim]

verify jprobe's entry point is a function entry point
using kallsyms' offset value.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>

---

Here comes 3rd revision of the patch. Thanks to Masami Hiramatus for suggesting
kallsyms_lookup_size_offset. :-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 8f96701..1b0dbe0 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1339,14 +1339,18 @@ int __kprobes register_jprobes(struct jprobe **jps, int num)
 	if (num <= 0)
 		return -EINVAL;
 	for (i = 0; i < num; i++) {
-		unsigned long addr;
+		unsigned long addr, offset;
 		jp = jps[i];
 		addr = arch_deref_entry_point(jp->entry);

-		/* Todo: Verify probepoint is a function entry point */
-		jp->kp.pre_handler = setjmp_pre_handler;
-		jp->kp.break_handler = longjmp_break_handler;
-		ret = register_kprobe(&jp->kp);
+		/* Verify probepoint is a function entry point */
+		if (kallsyms_lookup_size_offset(addr, NULL, &offset) &&
+		    offset = 0) {
+			jp->kp.pre_handler = setjmp_pre_handler;
+			jp->kp.break_handler = longjmp_break_handler;
+			ret = register_kprobe(&jp->kp);
+		} else
+			ret = -EINVAL;

 		if (ret < 0) {
 			if (i > 0)
--
1.7.0.4

Re: [PATCH 2/2] kprobes: verify jprobe entry point

[Masami Hiramatsu]

Namhyung Kim wrote:
> verify jprobe's entry point is a function entry point
> using kallsyms' offset value.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>

Looks good for me :) Thanks!

Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>


> ---
> 
> Here comes 3rd revision of the patch. Thanks to Masami Hiramatus for suggesting
> kallsyms_lookup_size_offset. :-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 8f96701..1b0dbe0 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1339,14 +1339,18 @@ int __kprobes register_jprobes(struct jprobe **jps, int num)
>  	if (num <= 0)
>  		return -EINVAL;
>  	for (i = 0; i < num; i++) {
> -		unsigned long addr;
> +		unsigned long addr, offset;
>  		jp = jps[i];
>  		addr = arch_deref_entry_point(jp->entry);
> 
> -		/* Todo: Verify probepoint is a function entry point */
> -		jp->kp.pre_handler = setjmp_pre_handler;
> -		jp->kp.break_handler = longjmp_break_handler;
> -		ret = register_kprobe(&jp->kp);
> +		/* Verify probepoint is a function entry point */
> +		if (kallsyms_lookup_size_offset(addr, NULL, &offset) &&
> +		    offset = 0) {
> +			jp->kp.pre_handler = setjmp_pre_handler;
> +			jp->kp.break_handler = longjmp_break_handler;
> +			ret = register_kprobe(&jp->kp);
> +		} else
> +			ret = -EINVAL;
> 
>  		if (ret < 0) {
>  			if (i > 0)
> --
> 1.7.0.4
> 

-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 2/2] kprobes: verify jprobe entry point

[walter harms]

Namhyung Kim schrieb:
> verify jprobe's entry point is a function entry point
> using kallsyms' offset value.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> 
> ---
> 
> Here comes 3rd revision of the patch. Thanks to Masami Hiramatus for suggesting
> kallsyms_lookup_size_offset. :-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 8f96701..1b0dbe0 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1339,14 +1339,18 @@ int __kprobes register_jprobes(struct jprobe **jps, int num)
>  	if (num <= 0)
>  		return -EINVAL;
>  	for (i = 0; i < num; i++) {
> -		unsigned long addr;
> +		unsigned long addr, offset;
>  		jp = jps[i];
>  		addr = arch_deref_entry_point(jp->entry);
> 
> -		/* Todo: Verify probepoint is a function entry point */
> -		jp->kp.pre_handler = setjmp_pre_handler;
> -		jp->kp.break_handler = longjmp_break_handler;
> -		ret = register_kprobe(&jp->kp);
> +		/* Verify probepoint is a function entry point */
> +		if (kallsyms_lookup_size_offset(addr, NULL, &offset) &&
> +		    offset = 0) {
> +			jp->kp.pre_handler = setjmp_pre_handler;
> +			jp->kp.break_handler = longjmp_break_handler;
> +			ret = register_kprobe(&jp->kp);
> +		} else
> +			ret = -EINVAL;
> 
>  		if (ret < 0) {
>  			if (i > 0)


I am a bit converned about this code here:
	if (kallsyms_lookup_size_offset(addr, NULL, &offset) &&  offset = 0)

offset is set inside kallsyms_lookup_size_offset(), right ?
but can you guarantee that the calling sequence is the same on all processors ?
(it is  not in case of functions).

perhaps it is more secure (and readable) to do:

ret=kallsyms_lookup_size_offset(addr, NULL, &offset);
if ( ret &&  offset = 0 )

just my  2 cents,
re,
 wh

Re: [PATCH 2/2] kprobes: verify jprobe entry point

[Julia Lawall]

On Thu, 5 Aug 2010, walter harms wrote:

> 
> 
> Namhyung Kim schrieb:
> > verify jprobe's entry point is a function entry point
> > using kallsyms' offset value.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> > 
> > ---
> > 
> > Here comes 3rd revision of the patch. Thanks to Masami Hiramatus for suggesting
> > kallsyms_lookup_size_offset. :-)
> > 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 8f96701..1b0dbe0 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -1339,14 +1339,18 @@ int __kprobes register_jprobes(struct jprobe **jps, int num)
> >  	if (num <= 0)
> >  		return -EINVAL;
> >  	for (i = 0; i < num; i++) {
> > -		unsigned long addr;
> > +		unsigned long addr, offset;
> >  		jp = jps[i];
> >  		addr = arch_deref_entry_point(jp->entry);
> > 
> > -		/* Todo: Verify probepoint is a function entry point */
> > -		jp->kp.pre_handler = setjmp_pre_handler;
> > -		jp->kp.break_handler = longjmp_break_handler;
> > -		ret = register_kprobe(&jp->kp);
> > +		/* Verify probepoint is a function entry point */
> > +		if (kallsyms_lookup_size_offset(addr, NULL, &offset) &&
> > +		    offset = 0) {
> > +			jp->kp.pre_handler = setjmp_pre_handler;
> > +			jp->kp.break_handler = longjmp_break_handler;
> > +			ret = register_kprobe(&jp->kp);
> > +		} else
> > +			ret = -EINVAL;
> > 
> >  		if (ret < 0) {
> >  			if (i > 0)
> 
> 
> I am a bit converned about this code here:
> 	if (kallsyms_lookup_size_offset(addr, NULL, &offset) &&  offset = 0)
> 
> offset is set inside kallsyms_lookup_size_offset(), right ?
> but can you guarantee that the calling sequence is the same on all processors ?
> (it is  not in case of functions).

&& should always be evaluated lazily from left to right.  Otherwise
x != NULL && x->y = z wouldn't be safe.

julia

> perhaps it is more secure (and readable) to do:
> 
> ret=kallsyms_lookup_size_offset(addr, NULL, &offset);
> if ( ret &&  offset = 0 )
> 
> just my  2 cents,
> re,
>  wh
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/2] kprobes: verify jprobe entry point

[Dan Carpenter]

On Thu, Aug 05, 2010 at 11:41:41AM +0200, walter harms wrote:
> 
> I am a bit converned about this code here:
> 	if (kallsyms_lookup_size_offset(addr, NULL, &offset) &&  offset = 0)
> 
> offset is set inside kallsyms_lookup_size_offset(), right ?
> but can you guarantee that the calling sequence is the same on all processors ?
> (it is  not in case of functions).
> 

Namhyung Kim's version is fine.

regards,
dan carpenter

Re: [PATCH 2/2] kprobes: verify jprobe entry point

[Håkon Løvdal]

On 5 August 2010 11:47, Julia Lawall <julia@diku.dk> wrote:
> On Thu, 5 Aug 2010, walter harms wrote:
>> I am a bit converned about this code here:
>>       if (kallsyms_lookup_size_offset(addr, NULL, &offset) &&  offset = 0)
>>
>> offset is set inside kallsyms_lookup_size_offset(), right ?
>> but can you guarantee that the calling sequence is the same on all processors ?
>> (it is  not in case of functions).
>
> && should always be evaluated lazily from left to right.  Otherwise
> x != NULL && x->y = z wouldn't be safe.
>

The && operator triggers a sequence point.

I do not have access to the standard (ISO/IEC 9899:1990) any longer,
so the following is from a draft (but in case the text changed, there
are no functional differences for this):


2.1.2.3 Program execution
...
At certain specified points in the execution sequence called
sequence points, all side effects of previous evaluations shall
be complete and no side effects of subsequent evaluations
shall have taken place.
...
3.3.13 Logical AND operator
...
Unlike the bitwise binary & operator, the && operator guarantees
left-to-right evaluation; there is a sequence point after the
evaluation of the first operand.


BR Håkon Løvdal