Re: [patch] kprobes: dont steal interrupts from vm86

Re: [patch] kprobes: dont steal interrupts from vm86

[Prasanna S Panchamukhi]

> Hello.
> 

Hi,

> With kprobes enabled, vm86 doesn't feel
> good. The problem is that kprobes steal
> the interrupts (mainly int3 I think) from
> it for no good reason.

If the int3 is not registered through kprobes,
kprobes handler does not handle it and it falls through the
normal int3 handler AFAIK.
Could you please provide a test case to show that kprobes 
steals the interrupts.

Thanks
Prasanna

-- 
Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<prasanna@in.ibm.com>

Re: [patch] kprobes: dont steal interrupts from vm86

[Stas Sergeev]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

Hi.

Prasanna S Panchamukhi wrote:
>> With kprobes enabled, vm86 doesn't feel
>> good. The problem is that kprobes steal
>> the interrupts (mainly int3 I think) from
>> it for no good reason.
> If the int3 is not registered through kprobes,
> kprobes handler does not handle it and it falls through the
> normal int3 handler AFAIK.
I was considering this, but I convinced
myself that checking the VM flag is good
in any case, because, as I presume, you
never need the interrupts from v86. Or do
you?
If there is a bug in kprobes, it would be
good to fix either, but I just think it
will not make my patch completely useless.

> Could you please provide a test case to show that kprobes 
> steals the interrupts.
Sure, attached. But it is not perfect: on
the patched kernel it passes the test, but
on the unpatched one (2.6.9), it just Oopses
the kernel without printing any reasonable
diagnostic. Because of the Oops, I can't
demonstrate the interrupt theft right away,
but I hope the test-case for the Oops in
kprobe_exceptions_notify() may also be
interesting for you.


[-- Attachment #2: trap.c --]
[-- Type: text/x-csrc, Size: 2048 bytes --]

#include <stdio.h>
#include <string.h>
#include <inttypes.h>
#include <unistd.h>
#include <sys/mman.h>

#include <linux/unistd.h>
#include <asm/vm86.h>

_syscall2(int, vm86, int, func, struct vm86plus_struct *, v86)

static inline void set_bit(uint8_t *a, unsigned int bit)
{
    a[bit / 8] |= (1 << (bit % 8));
}

static inline uint8_t *seg_to_linear(unsigned int seg, unsigned int reg)
{
    return (uint8_t *)((seg << 4) + (reg & 0xffff));
}

int main()
{
    uint8_t *vm86_mem;
    int ret, seg, arg, insn;
    struct vm86plus_struct ctx;
    struct vm86_regs *r;

    vm86_mem = mmap((void *)0x00000000, 0x110000, 
                    PROT_WRITE | PROT_READ | PROT_EXEC, 
                    MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
    if (vm86_mem == MAP_FAILED) {
        perror("mmap");
        return 1;
    }

    memset(&ctx, 0, sizeof(ctx));
    /* init basic registers */
    r = &ctx.regs;
    r->eip = 0x100;
    r->esp = 0xfffe;
    seg = 256;
    r->cs = seg;
    r->ss = seg;
    r->ds = seg;
    r->es = seg;
    r->fs = seg;
    r->gs = seg;
    r->eflags = VIF_MASK;

    /* put return code */
    set_bit((uint8_t *)&ctx.int_revectored, 3);
    *seg_to_linear(r->cs, r->eip) = 0xcc;	/* int3 */
    *seg_to_linear(r->cs, r->eip + 1) = 0xf4;	/* hlt */

do_vm86:
    ret = vm86(VM86_ENTER, &ctx);
    arg = VM86_ARG(ret);
    insn = *seg_to_linear(r->cs, r->eip);
    switch(VM86_TYPE(ret)) {
        case VM86_INTx:
	    printf("vm86: INT 0x%x\n", VM86_ARG(ret));
            break;
        case VM86_STI:
        case VM86_SIGNAL:
            /* a signal came, we just ignore that */
	    goto do_vm86;
            break;
        case VM86_TRAP:
	    if (arg == 3)
		printf("vm86: Trap 3 - All OK\n");
	    else
		printf("Unknown trap %#x\n", arg);
	    break;
        case VM86_UNKNOWN:
	    if (insn == 0xf4)
		printf("vm86: HLT, test failed\n");
	    else
		printf("vm86: unknown result, insn=%#x\n", insn);
        default:
            fprintf(stderr, "unhandled vm86 return code (0x%x)\n", ret);
    }
    return 0;
}

Re: [patch] kprobes: dont steal interrupts from vm86

[Prasanna S Panchamukhi]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

Hello,

> 
> Prasanna S Panchamukhi wrote:
> >>With kprobes enabled, vm86 doesn't feel
> >>good. The problem is that kprobes steal
> >>the interrupts (mainly int3 I think) from
> >>it for no good reason.
> >If the int3 is not registered through kprobes,
> >kprobes handler does not handle it and it falls through the
> >normal int3 handler AFAIK.
> I was considering this, but I convinced
> myself that checking the VM flag is good
> in any case, because, as I presume, you
> never need the interrupts from v86. Or do
> you?
> If there is a bug in kprobes, it would be
> good to fix either, but I just think it
> will not make my patch completely useless.
> 
Yes, there is a small bug in kprobes. Kprobes int3 handler
was returning wrong value. Please check out if the patch
attached with this mail fixes your problem.

Please let me know if you have any issues.

Thanks
Prasanna

-- 
Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<prasanna@in.ibm.com>

[-- Attachment #2: kprobes-vm86-interrupt-miss.patch --]
[-- Type: text/plain, Size: 905 bytes --]


This patch fixes the problem reported by Stas Sergeev, that kprobes steals
the virtual-8086 exceptions. This fix modifies kprobe_handler() to return 0 when in
virtual-8086 mode.


---

 linux-2.6.10-rc2-prasanna/arch/i386/kernel/kprobes.c |    4 ++++
 1 files changed, 4 insertions(+)

diff -puN arch/i386/kernel/kprobes.c~kprobes-vm86-interrupt-miss arch/i386/kernel/kprobes.c
--- linux-2.6.10-rc2/arch/i386/kernel/kprobes.c~kprobes-vm86-interrupt-miss	2004-11-17 18:30:11.000000000 +0530
+++ linux-2.6.10-rc2-prasanna/arch/i386/kernel/kprobes.c	2004-11-17 18:38:20.000000000 +0530
@@ -117,6 +117,10 @@ static inline int kprobe_handler(struct 
 	p = get_kprobe(addr);
 	if (!p) {
 		unlock_kprobes();
+		if (regs->eflags & VM_MASK)
+		/*we are in virtual-8086 mode, return 0*/
+			goto no_kprobe;
+
 		if (*addr != BREAKPOINT_INSTRUCTION) {
 			/*
 			 * The breakpoint instruction was removed right

_

Re: [patch] kprobes: dont steal interrupts from vm86

[Stas Sergeev]

Hi.

Prasanna S Panchamukhi wrote:
> Yes, there is a small bug in kprobes. Kprobes int3 handler
> was returning wrong value. Please check out if the patch
> attached with this mail fixes your problem.
The patch is tested and works - thanks.

Re: [patch] kprobes: dont steal interrupts from vm86

[Stas Sergeev]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

Hello.

Prasanna S Panchamukhi wrote:
> Yes, there is a small bug in kprobes. Kprobes int3 handler
> was returning wrong value. Please check out if the patch
> attached with this mail fixes your problem.
> Please let me know if you have any issues.
Yes. After several days of debugging,
I am pointing to this problem again.
Unfortunately your patch appeared not
to work. It only masks the problem.
I was surprised that you check VM_MASK
after you already used "addr" a couple
of times - this "addr" is completely
bogus and should not be used. Now this
turned out more important. The problem
is that the "addr" calculated only from
the value of EIP, is bogus not only when
VM flag is set. It is also bogus if the
program uses segmentation and the
CS_base!=0. I have many of the like
programs here and they all are broken
because kprobes still steal the int3 from
them. They do not use V86, but they use
segments instead of the flat layout, so
the address cannot be calculated by the
EIP value.
I would suggest something like the attached
patch. I know nothing about kprobes (sorry)
so I don't know what CS you need. If you
need not only __KERNEL_CS, you probably
want the (regs->xcs & 4) check to see if
the CS is not from LDT at least. Does this
make sense?
Anyway, would be nice to get this fixed.
This can cause Oopses because you deref
the completely bogus pointer later in the
code.
Writing a test-case for this problem is
not a several-minutes work, but if you
really need one, I may try to hack it out.

Thanks.


[-- Attachment #2: kprb.diff --]
[-- Type: text/x-patch, Size: 684 bytes --]

--- linux/arch/i386/kernel/kprobes.c.old	2004-11-18 16:22:46.000000000 +0300
+++ linux/arch/i386/kernel/kprobes.c	2004-12-02 22:01:05.000000000 +0300
@@ -92,6 +92,11 @@
 	int ret = 0;
 	u8 *addr = (u8 *) (regs->eip - 1);
 
+	/* If we are in v86 mode or CS is not ours, get out */
+	if ((regs->eflags & VM_MASK) || regs->xcs != __KERNEL_CS) {
+		return 0;
+	}
+
 	/* We're in an interrupt, but this is clear and BUG()-safe. */
 	preempt_disable();
 
@@ -117,10 +122,6 @@
 	p = get_kprobe(addr);
 	if (!p) {
 		unlock_kprobes();
-		if (regs->eflags & VM_MASK) {
-			/* We are in virtual-8086 mode. Return 0 */
-			goto no_kprobe;
-		}
 
 		if (*addr != BREAKPOINT_INSTRUCTION) {
 			/*

Re: [patch] kprobes: dont steal interrupts from vm86

[Stas Sergeev]

[-- Attachment #1: Type: text/plain, Size: 311 bytes --]

Hi Prasanna.

I've found yet another bug in this
very same piece of code. Now I can
reproduce the interrupt theft without
using either vm86() or modify_ldt().
Test-case is attached. It gets
ocasionally fixed by the patch I've
sent in my previous mail, but it is
really another bug that requires a
separate fix.

[-- Attachment #2: trap1.c --]
[-- Type: text/x-csrc, Size: 233 bytes --]

#include <stdlib.h>
#include <signal.h>

void my_trap(int sig)
{
  printf("Test passed, all OK\n");
  exit(0);
}

int main()
{
  signal(SIGTRAP, my_trap);
  asm volatile (".byte 0xcd,3");
  printf("Stolen interrupt, very bad!\n");
}

Re: [patch] kprobes: dont steal interrupts from vm86

[Prasanna S Panchamukhi]

Hi,
On Thu, Dec 02, 2004 at 10:28:32PM +0300, Stas Sergeev wrote:
> Hello.
> 
> Prasanna S Panchamukhi wrote:
> >Yes, there is a small bug in kprobes. Kprobes int3 handler
> >was returning wrong value. Please check out if the patch
> >attached with this mail fixes your problem.
> >Please let me know if you have any issues.
> Yes. After several days of debugging,
> I am pointing to this problem again.
> Unfortunately your patch appeared not
> to work. It only masks the problem.
> I was surprised that you check VM_MASK
> after you already used "addr" a couple
> of times - this "addr" is completely
> bogus and should not be used. Now this
> turned out more important. The problem
> is that the "addr" calculated only from
> the value of EIP, is bogus not only when
> VM flag is set. It is also bogus if the
> program uses segmentation and the
> CS_base!=0. I have many of the like
> programs here and they all are broken
> because kprobes still steal the int3 from
> them. They do not use V86, but they use
> segments instead of the flat layout, so
> the address cannot be calculated by the
> EIP value.

Well, a test program is always better. I 
would appreciate if you can sent me the
test program.

> I would suggest something like the attached
> patch. I know nothing about kprobes (sorry)
> so I don't know what CS you need. If you
> need not only __KERNEL_CS, you probably
> want the (regs->xcs & 4) check to see if
> the CS is not from LDT at least. Does this
> make sense?
> Anyway, would be nice to get this fixed.
> This can cause Oopses because you deref
> the completely bogus pointer later in the
> code.
> Writing a test-case for this problem is
> not a several-minutes work, but if you
> really need one, I may try to hack it out.
> 
> Thanks.
> 



Thanks
Prasanna

-- 

Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<prasanna@in.ibm.com>

Re: [patch] kprobes: dont steal interrupts from vm86

[Prasanna S Panchamukhi]

Hi Stas,

> I've found yet another bug in this
> very same piece of code. Now I can
> reproduce the interrupt theft without
> using either vm86() or modify_ldt().

The patch below should fix this problem. Please
let me know if you any issues.

Regards
Prasanna



Stas repoted that kprobes steals int3 exceptions when not in 
virtual-8086 mode. This patch fixes the problem by returning 0,
if the int3 exceptions does not belong to kprobes.

Signed-off-by: Prasanna S Panchamukhi <prasanna@in.ibm.com>


---

 linux-2.6.10-rc3-prasanna/arch/i386/kernel/kprobes.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff -puN arch/i386/kernel/kprobes.c~kprobes-steals-int3 arch/i386/kernel/kprobes.c
--- linux-2.6.10-rc3/arch/i386/kernel/kprobes.c~kprobes-steals-int3	2004-12-07 11:20:33.000000000 +0530
+++ linux-2.6.10-rc3-prasanna/arch/i386/kernel/kprobes.c	2004-12-07 11:20:34.000000000 +0530
@@ -127,10 +127,10 @@ static inline int kprobe_handler(struct 
 			 * The breakpoint instruction was removed right
 			 * after we hit it.  Another cpu has removed
 			 * either a probepoint or a debugger breakpoint
-			 * at this address.  In either case, no further
-			 * handling of this interrupt is appropriate.
+			 * at this address. In either case, kprobes
+			 * need not handle it.
 			 */
-			ret = 1;
+			ret = 0;
 		}
 		/* Not one of ours: let kernel handle it */
 		goto no_kprobe;

_
-- 

Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<prasanna@in.ibm.com>

Re: [patch] kprobes: dont steal interrupts from vm86

[Stas Sergeev]

Hi Prasanna.

You wrote:
> The patch below should fix this problem. Please
Thanks.

> let me know if you any issues.
Well, it kind of fixes the problem.
Actually it happened that it fixes
even the problem for the programs
that are using the segmentation.
Now am I happy? I am afraid, not.
To me your patch looks very wrong
(sorry), even though I am really not
the one to do such a claims.
Let's see how it "fixes" the problem
with segmentation, that I outlined
in my previous posting:

> -			ret = 1;
> +			ret = 0;
It is easy to notice that ret==0 at
that point *always*, so effectively
this code is now a no-op. Apparently
also gcc mentioned that, and removed
that code together with the surrounding
"if" clause. And it was exactly the
"if" condition where the bogus pointer
gets dereferenced. Now, since it gets
optimized away, the Oops is not any
more. But if I stick a simple printk()
in that block, the Oops is back.
So you "fixed" that based on the gcc
optimization.
Now the comments (that you just altered)
suggest that the break-point can be
removed by another CPU. I don't think
delivering the fault to the user-space
in this case is wise (but that's what
I'd care least, as I am not using the
kprobes myself yet). Maybe instead
it would be better to return 1 when
p!=NULL?
Also, you still use the completely
invalid addrress and pass it to several
functions like get_kprobe() (addr is
invalid in either v86 case or when the
segmentation is used). Since the
deref is now optimized away by gcc, I
can't write an Oops test-case for this,
but why you do not perform the sanity
checks to see whether or not the address
is valid? (the checks like I suggested
in previous posting)

Oh well. That said, your patch works
for me. I'd appreciate another patch
very much, that will address the problems
I see, but can't demonstrate by the
test-case any more. But of course at
least for me it is already better than
nothing, and of course it is not me to
decide whether the patch is to apply
or not. We'll see. Thanks anyway.

Re: [patch] kprobes: dont steal interrupts from vm86

[Prasanna S Panchamukhi]

Hi,
> Now the comments (that you just altered)
> suggest that the break-point can be
> removed by another CPU. I don't think
> delivering the fault to the user-space
> in this case is wise (but that's what
> I'd care least, as I am not using the
> kprobes myself yet). Maybe instead
> it would be better to return 1 when

The patch below takes both the cases into 
consideration. 

> Also, you still use the completely
> invalid addrress and pass it to several
> functions like get_kprobe() (addr is
> invalid in either v86 case or when the
> segmentation is used). Since the
> deref is now optimized away by gcc, I
> can't write an Oops test-case for this,
> but why you do not perform the sanity
> checks to see whether or not the address
> is valid? (the checks like I suggested
> in previous posting)
> 
I am not able to think of a case, where 
address is invalid when it enters int3 handler.
I would appreciate if you can provide such a
test case.

Your comments are welcome.

Thanks
Prasanna


Stas reported that kprobes steals int3 exceptions when not in 
virtual-8086 mode. If processor  executes int 3 INT n type instruction, it
will end up executing int3 handler. This patch fixes the problem by returning 0,
if the int3 exceptions does not belong to kprobes and allowing the kernel to 
handle it.

Signed-off-by: Prasanna S Panchamukhi <prasanna@in.ibm.com>


---

 linux-2.6.10-rc3-prasanna/arch/i386/kernel/kprobes.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff -puN arch/i386/kernel/kprobes.c~kprobes-steals-int3 arch/i386/kernel/kprobes.c
--- linux-2.6.10-rc3/arch/i386/kernel/kprobes.c~kprobes-steals-int3	2004-12-09 17:09:08.000000000 +0530
+++ linux-2.6.10-rc3-prasanna/arch/i386/kernel/kprobes.c	2004-12-09 17:09:36.000000000 +0530
@@ -117,8 +117,12 @@ static inline int kprobe_handler(struct 
 	p = get_kprobe(addr);
 	if (!p) {
 		unlock_kprobes();
-		if (regs->eflags & VM_MASK) {
-			/* We are in virtual-8086 mode. Return 0 */
+		if ((regs->eflags & VM_MASK) ||
+				((*addr == 0x3) && (*(addr - 1) == 0xcd))) {
+			/* Either we are in virtual-8086 mode, or we executed
+			 * int 3 INT n type instruction. Let kernel handle
+			 * it, return 0.
+			 */
 			goto no_kprobe;
 		}
 

_
-- 

Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<prasanna@in.ibm.com>

Re: [patch] kprobes: dont steal interrupts from vm86

[Stas Sergeev]

[-- Attachment #1: Type: text/plain, Size: 1207 bytes --]

Hi.

Prasanna S Panchamukhi wrote:
> The patch below takes both the cases into 
> consideration. 
OK, perhaps this one is better, at least
it no longer plays on gcc optimization,
so that I can reproduce the Oopses again,
for good and for test-case.
But I think you really should consider
checking regs->xcs instead of explicitly
checking the corner cases like 0xcd,3.

> I am not able to think of a case, where 
> address is invalid when it enters int3 handler.
> I would appreciate if you can provide such a
> test case.
I already did - it was my very first test-case
which produced the Oops on the if(*addr!=...)
dereference, but you worked around by checking
the VM flag (well, it must be checked, but
not after you already used "addr" a couple of
times, IMHO).
OK, but if you need another test-case,
here it is. Much simpler than the vm86 one.
It can work in 2 modes: started without args,
it will print the diagnostic (passed or
failed) and exit. If started with any arg,
it will Oops the kernel.
This happens both with your latest patch
and without it. This doesn't happen with
your previous patch (no Oops), but then fixing
problems by exploiting the gcc optimization
was not the best idea I think.


[-- Attachment #2: brk.c --]
[-- Type: text/x-csrc, Size: 1766 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <signal.h>
#include <linux/unistd.h>
#include <asm/ldt.h>
#include <asm/segment.h>
#include <asm/page.h>
#include <sys/mman.h>

_syscall3(int, modify_ldt, int, func, void *, ptr, unsigned long, bytecount)

static int set_ldt_entry(int entry, unsigned long base, unsigned int limit,
	      int seg_32bit_flag, int contents, int read_only_flag,
	      int limit_in_pages_flag, int seg_not_present, int useable)
{
  struct modify_ldt_ldt_s ldt_info;
  ldt_info.entry_number = entry;
  ldt_info.base_addr = base;
  ldt_info.limit = limit;
  ldt_info.seg_32bit = seg_32bit_flag;
  ldt_info.contents = contents;
  ldt_info.read_exec_only = read_only_flag;
  ldt_info.limit_in_pages = limit_in_pages_flag;
  ldt_info.seg_not_present = seg_not_present;
  ldt_info.useable = useable;

  return modify_ldt(1, &ldt_info, sizeof(ldt_info));
}

void my_trap(int sig)
{
  printf("Test passed, All OK!\n");
  exit(0);
}

int main(int argc, char *argv[])
{
  unsigned char *ptr;
  if (mmap(0, PAGE_SIZE, PROT_READ | PROT_WRITE,
      MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0) == MAP_FAILED) {
    perror("mmap");
    return 1;
  }
  if ((ptr = mmap(0, PAGE_SIZE, PROT_READ | PROT_WRITE | PROT_EXEC,
      MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)) == MAP_FAILED) {
    perror("mmap");
    return 1;
  }
  if (argc == 1)		/* no-Oops mode */
    *(unsigned char *)0 = 1;	/* Set the no-Oops flag :) */
  /* Create the LDT entry */
  #define MY_CS (__USER_CS | 4)
  set_ldt_entry(MY_CS >> 3, (unsigned long)ptr, PAGE_SIZE - 1, 1,
    MODIFY_LDT_CONTENTS_CODE, 1, 0, 0, 0);
  ptr[0] = 0xcc;
  ptr[1] = 0xcb;
  signal(SIGTRAP, my_trap);
  asm volatile ("lcall %0,$0\n"::"i"(MY_CS));
  printf("Stolen interrupt, very bad.\n");
  return 0;
}

Re: [patch] kprobes: dont steal interrupts from vm86

[Prasanna S Panchamukhi]

Hi Stas,

On Thu, Dec 09, 2004 at 10:28:25PM +0300, Stas Sergeev wrote:

> OK, but if you need another test-case,
> here it is. Much simpler than the vm86 one.
> It can work in 2 modes: started without args,
> it will print the diagnostic (passed or
> failed) and exit. If started with any arg,
> it will Oops the kernel.
> This happens both with your latest patch
> and without it. This doesn't happen with
> your previous patch (no Oops), but then fixing
> problems by exploiting the gcc optimization
> was not the best idea I think.
> 

The patch below fixes this problem.
Please let me know your comments.

Thanks
Prasanna



The address used by the kprobes handler was not correct if the application
was using LDT entries for code segment. This patch fixes the above problem by
calculating the address using base address of the current code segment.
Also this patch removes the inline prefix of kprobe_handler() .

Signed-off-by: Prasanna S Panchamukhi <prasanna@in.ibm.com>


---

 linux-2.6.10-prasanna/arch/i386/kernel/kprobes.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff -puN arch/i386/kernel/kprobes.c~kprobes-corrupt-eip arch/i386/kernel/kprobes.c
--- linux-2.6.10/arch/i386/kernel/kprobes.c~kprobes-corrupt-eip	2005-01-07 17:01:37.000000000 +0530
+++ linux-2.6.10-prasanna/arch/i386/kernel/kprobes.c	2005-01-07 17:01:49.000000000 +0530
@@ -86,15 +86,28 @@ static inline void prepare_singlestep(st
  * Interrupts are disabled on entry as trap3 is an interrupt gate and they
  * remain disabled thorough out this function.
  */
-static inline int kprobe_handler(struct pt_regs *regs)
+static int kprobe_handler(struct pt_regs *regs)
 {
 	struct kprobe *p;
 	int ret = 0;
-	u8 *addr = (u8 *) (regs->eip - 1);
+	kprobe_opcode_t *addr = NULL;
+	unsigned long *lp;
 
 	/* We're in an interrupt, but this is clear and BUG()-safe. */
 	preempt_disable();
-
+	/* Check if the application is using LDT entry for its code segment and
+	 * calculate the address by reading the base address from the LDT entry.
+	 */
+	if ((regs->xcs & 4) && (current->mm)) {
+		lp = (unsigned long *) ((unsigned long)((regs->xcs >> 3) * 8)
+					+ (char *) current->mm->context.ldt);
+		addr = (kprobe_opcode_t *) ((((*lp) >> 16 &  0x0000ffff)
+				| (*(lp +1) & 0xff000000)
+				| ((*(lp +1) << 16) & 0x00ff0000))
+				+ regs->eip - sizeof(kprobe_opcode_t));
+	} else {
+		addr = (kprobe_opcode_t *)(regs->eip - sizeof(kprobe_opcode_t));
+	}
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {
 		/* We *are* holding lock here, so this is safe.

_
-- 

Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<prasanna@in.ibm.com>

Re: [patch] kprobes: dont steal interrupts from vm86

[Andi Kleen]

Prasanna S Panchamukhi <prasanna@in.ibm.com> writes:


> +	/* Check if the application is using LDT entry for its code segment and
> +	 * calculate the address by reading the base address from the LDT entry.
> +	 */
> +	if ((regs->xcs & 4) && (current->mm)) {
> +		lp = (unsigned long *) ((unsigned long)((regs->xcs >> 3) * 8)
> +					+ (char *) current->mm->context.ldt);
> +		addr = (kprobe_opcode_t *) ((((*lp) >> 16 &  0x0000ffff)
> +				| (*(lp +1) & 0xff000000)
> +				| ((*(lp +1) << 16) & 0x00ff0000))
> +				+ regs->eip - sizeof(kprobe_opcode_t));
> +	} else {
> +		addr = (kprobe_opcode_t *)(regs->eip - sizeof(kprobe_opcode_t));
> +	}

With that patch we would have LDT reading code three times in the kernel
now (ptrace, prefetch workaround and now this). How about you factor
this out into a common helper function? This stuff is tricky enough
that there are likely bugs in there anyways and it would be best 
to only fix them at one place then.

-Andi

Re: [patch] kprobes: dont steal interrupts from vm86

[Stas Sergeev]

Hi.

Prasanna S Panchamukhi wrote:
> The patch below fixes this problem.
> Please let me know your comments.
The patch works, thanks. I have no
complains to it, it fixes the problem
it is intended to fix.

The following is just a reminder about
the other problems I mentioned earlier.

This problem is still not addressed:
http://lkml.org/lkml/2004/12/4/48

Also VM86 check should be done before the
"addr" is used I think, is this true?

And dereferencing the pointer to user-space,
like "*addr", is this safe? I thought
get_user() is used for that, was this
intentional?

By the way, maybe it is possible to avoid
the removal race without checking an opcode
at all? Probably by marking the probes as
"removed", and checking the "addr" against
the "removed" ones instead of checking the
opcode? This way you'll avoid any interference
with the debuggers that can also remove their
breakpoints, and also the aforementioned
problem will get fixed automatically.

Re: [patch] kprobes: dont steal interrupts from vm86

[Prasanna S Panchamukhi]

Hi Andi,

> > +		addr = (kprobe_opcode_t *) ((((*lp) >> 16 &  0x0000ffff)
> > +				| (*(lp +1) & 0xff000000)
> > +				| ((*(lp +1) << 16) & 0x00ff0000))

> With that patch we would have LDT reading code three times in the kernel
> now (ptrace, prefetch workaround and now this). How about you factor
> this out into a common helper function? This stuff is tricky enough
> that there are likely bugs in there anyways and it would be best 
> to only fix them at one place then.

The patch below moves this tricky code to a common place, please let
me know your comments. Ptrace uses a structure instead of unsigned long *.

Thanks
Prasanna


Calculating the base address of the segment is tricky and
is used in several places as well. This patch moves this tricky part
in a common place as suggested by Andi Kleen.

Signed-of-by: Prasanna S Panchamukhi <prasanna@in.ibm.com>


---

 linux-2.6.11-rc1-prasanna/arch/i386/kernel/kprobes.c |    7 +++----
 linux-2.6.11-rc1-prasanna/arch/i386/mm/fault.c       |    4 +---
 linux-2.6.11-rc1-prasanna/include/asm-i386/desc.h    |    9 +++++++++
 3 files changed, 13 insertions(+), 7 deletions(-)

diff -puN arch/i386/mm/fault.c~kprobes-desc-common-routine arch/i386/mm/fault.c
--- linux-2.6.11-rc1/arch/i386/mm/fault.c~kprobes-desc-common-routine	2005-01-13 11:29:49.000000000 +0530
+++ linux-2.6.11-rc1-prasanna/arch/i386/mm/fault.c	2005-01-13 11:36:08.000000000 +0530
@@ -112,9 +112,7 @@ static inline unsigned long get_segment_
 	}
 
 	/* Decode the code segment base from the descriptor */
-	base =   (desc[0] >> 16) |
-		((desc[1] & 0xff) << 16) |
-		 (desc[1] & 0xff000000);
+	base = get_desc_base((unsigned long *)desc);
 
 	if (seg & (1<<2)) { 
 		up(&current->mm->context.sem);
diff -puN arch/i386/kernel/kprobes.c~kprobes-desc-common-routine arch/i386/kernel/kprobes.c
--- linux-2.6.11-rc1/arch/i386/kernel/kprobes.c~kprobes-desc-common-routine	2005-01-13 11:30:01.000000000 +0530
+++ linux-2.6.11-rc1-prasanna/arch/i386/kernel/kprobes.c	2005-01-13 11:44:43.000000000 +0530
@@ -31,6 +31,7 @@
 #include <linux/spinlock.h>
 #include <linux/preempt.h>
 #include <asm/kdebug.h>
+#include <asm/desc.h>
 
 /* kprobe_status settings */
 #define KPROBE_HIT_ACTIVE	0x00000001
@@ -101,10 +102,8 @@ static int kprobe_handler(struct pt_regs
 	if ((regs->xcs & 4) && (current->mm)) {
 		lp = (unsigned long *) ((unsigned long)((regs->xcs >> 3) * 8)
 					+ (char *) current->mm->context.ldt);
-		addr = (kprobe_opcode_t *) ((((*lp) >> 16 &  0x0000ffff)
-				| (*(lp +1) & 0xff000000)
-				| ((*(lp +1) << 16) & 0x00ff0000))
-				+ regs->eip - sizeof(kprobe_opcode_t));
+		addr = (kprobe_opcode_t *) (get_desc_base(lp) + regs->eip -
+						sizeof(kprobe_opcode_t));
 	} else {
 		addr = (kprobe_opcode_t *)(regs->eip - sizeof(kprobe_opcode_t));
 	}
diff -puN include/asm-i386/desc.h~kprobes-desc-common-routine include/asm-i386/desc.h
--- linux-2.6.11-rc1/include/asm-i386/desc.h~kprobes-desc-common-routine	2005-01-13 11:30:11.000000000 +0530
+++ linux-2.6.11-rc1-prasanna/include/asm-i386/desc.h	2005-01-13 11:31:36.000000000 +0530
@@ -126,6 +126,15 @@ static inline void load_LDT(mm_context_t
 	put_cpu();
 }
 
+static inline unsigned long get_desc_base(unsigned long *desc)
+{
+	unsigned long base;
+	base = ((desc[0] >> 16)  & 0x0000ffff) |
+		((desc[1] << 16) & 0x00ff0000) |
+		(desc[1] & 0xff000000);
+	return base;
+}
+
 #endif /* !__ASSEMBLY__ */
 
 #endif

_
-- 

Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<prasanna@in.ibm.com>

[patch] kprobes: dont steal interrupts from vm86

[Stas Sergeev]

[-- Attachment #1: Type: text/plain, Size: 319 bytes --]

Hello.

With kprobes enabled, vm86 doesn't feel
good. The problem is that kprobes steal
the interrupts (mainly int3 I think) from
it for no good reason.
The attached patch fixes the problem by
checking the VM flag where it makes sense.

Andrew, can this please be applied?

Signed-off-by: Stas Sergeev <stsp@aknet.ru>


[-- Attachment #2: kprb_vm86.diff --]
[-- Type: text/x-patch, Size: 2468 bytes --]

--- linux-2.6.10-rc1/arch/i386/kernel/traps.c	2004-11-09 12:59:20.000000000 +0300
+++ linux-2.6.10-rc1-kprb/linux/arch/i386/kernel/traps.c	2004-11-09 13:35:18.625427704 +0300
@@ -408,7 +408,8 @@
 #define DO_ERROR(trapnr, signr, str, name) \
 asmlinkage void do_##name(struct pt_regs * regs, long error_code) \
 { \
-	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) \
+	if (!(regs->eflags & VM_MASK) && \
+		notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) \
 						== NOTIFY_STOP) \
 		return; \
 	do_trap(trapnr, signr, str, 0, regs, error_code, NULL); \
@@ -422,7 +423,8 @@
 	info.si_errno = 0; \
 	info.si_code = sicode; \
 	info.si_addr = (void __user *)siaddr; \
-	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) \
+	if (!(regs->eflags & VM_MASK) && \
+		notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) \
 						== NOTIFY_STOP) \
 		return; \
 	do_trap(trapnr, signr, str, 0, regs, error_code, &info); \
@@ -431,7 +433,8 @@
 #define DO_VM86_ERROR(trapnr, signr, str, name) \
 asmlinkage void do_##name(struct pt_regs * regs, long error_code) \
 { \
-	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) \
+	if (!(regs->eflags & VM_MASK) && \
+		notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) \
 						== NOTIFY_STOP) \
 		return; \
 	do_trap(trapnr, signr, str, 1, regs, error_code, NULL); \
@@ -445,7 +448,8 @@
 	info.si_errno = 0; \
 	info.si_code = sicode; \
 	info.si_addr = (void __user *)siaddr; \
-	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) \
+	if (!(regs->eflags & VM_MASK) && \
+		notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) \
 						== NOTIFY_STOP) \
 		return; \
 	do_trap(trapnr, signr, str, 1, regs, error_code, &info); \
@@ -652,7 +656,8 @@
 #ifdef CONFIG_KPROBES
 asmlinkage int do_int3(struct pt_regs *regs, long error_code)
 {
-	if (notify_die(DIE_INT3, "int3", regs, error_code, 3, SIGTRAP)
+	if (!(regs->eflags & VM_MASK) &&
+		notify_die(DIE_INT3, "int3", regs, error_code, 3, SIGTRAP)
 			== NOTIFY_STOP)
 		return 1;
 	/* This is an interrupt gate, because kprobes wants interrupts
@@ -693,7 +698,8 @@
 
 	__asm__ __volatile__("movl %%db6,%0" : "=r" (condition));
 
-	if (notify_die(DIE_DEBUG, "debug", regs, condition, error_code,
+	if (!(regs->eflags & VM_MASK) && 
+		notify_die(DIE_DEBUG, "debug", regs, condition, error_code,
 					SIGTRAP) == NOTIFY_STOP)
 		return;
 	/* It's safe to allow irq's after DR6 has been saved */