uprobes: mmap_region() corrupts mm->mm_rb if uprobe_mmap() fails

https://bugzilla.redhat.com/show_bug.cgi?id=843640

If mmap_region()->uprobe_mmap() fails, unmap_and_free_vma path does unmap_region() but does not remove the soon-to-be-freed vma from rb tree (actually there are more problems).

https://bugzilla.redhat.com/show_bug.cgi?id=843640

If mmap_region()->uprobe_mmap() fails, unmap_and_free_vma path does unmap_region() but does not remove the soon-to-be-freed vma from rb tree (actually there are more problems).

Perhaps we could do do_munmap() + return in this case, but in fact it is simply wrong to abort if uprobe_mmap() fails. Until at least we move the !UPROBE_COPY_INSN code from install_breakpoint() to uprobe_register().

For example, uprobe_mmap()->install_breakpoint() can fail if the probed insn is not supported (remember, uprobe_register() succeeds if nobody mmaps inode/offset), mmap() should not fail in this case.

dup_mmap()->uprobe_mmap() is wrong too by the same reason, fork() can race with uprobe_register() and fail for no reason if it wins the race and does install_breakpoint() first.

Change mmap_region() and dup_mmap() to ignore the error code from uprobe_mmap().

Reported-by William Cohen <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

Cc <[email protected]> # v3.5

---
 kernel/fork.c |    4 ++--
 mm/mmap.c     |    5 ++---
 2 files changed, 4 insertions(+), 5 deletions(-)

kernel/fork.c +2 -2

index ab5211b..54bb88a 100644
@@ -454,8 +454,8 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
 		if (retval)
 			goto out;
 
-		if (file && uprobe_mmap(tmp))
-			goto out;
+		if (file)
+			uprobe_mmap(tmp);
 	}
 	/* a new mm has just been created */
 	arch_dup_mmap(oldmm, mm);

mm/mmap.c +2 -3

index 4fe2697..f25fd3f 100644
@@ -1355,9 +1355,8 @@ out:
 	} else if ((flags & MAP_POPULATE) && !(flags & MAP_NONBLOCK))
 		make_pages_present(addr, addr + len);
 
-	if (file && uprobe_mmap(vma))
-		/* matching probes but cannot insert */
-		goto unmap_and_free_vma;
+	if (file)
+		uprobe_mmap(vma);
 
 	return addr;
 

If mmap_region()->uprobe_mmap() fails, unmap_and_free_vma path does unmap_region() but does not remove the soon-to-be-freed vma from rb tree (actually there are more problems).

Perhaps we could do do_munmap() + return in this case, but in fact it is simply wrong to abort if uprobe_mmap() fails. Until at least we move the !UPROBE_COPY_INSN code from install_breakpoint() to uprobe_register().

For example, uprobe_mmap()->install_breakpoint() can fail if the probed insn is not supported (remember, uprobe_register() succeeds if nobody mmaps inode/offset), mmap() should not fail in this case.

dup_mmap()->uprobe_mmap() is wrong too by the same reason, fork() can race with uprobe_register() and fail for no reason if it wins the race and does install_breakpoint() first.

Change mmap_region() and dup_mmap() to ignore the error code from uprobe_mmap().

Reported-by William Cohen <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

Cc <[email protected]> # v3.5

---
 kernel/fork.c |    4 ++--
 mm/mmap.c     |    5 ++---
 2 files changed, 4 insertions(+), 5 deletions(-)

kernel/fork.c +0 -0

index ab5211b..54bb88a 100644

Quoted 7 lines

kernel/fork.c +0 -0

@@ -454,8 +454,8 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
 		if (retval)
 			goto out;
 

kernel/fork.c static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm) · line 458

Quoted 4 lines

kernel/fork.c +2 -2

-		if (file && uprobe_mmap(tmp))
-			goto out;
+		if (file)
+			uprobe_mmap(tmp);

I am not comfortable with this fix.

I think the long term solution is as you mentioned, move the instruction analysis to register.

Lets say there were 10 probes that were to be installed in that vma. we were able to install five probes and the 6th one happened to fail because of invalid instruction; we dont continue with the registering probes for the remaining 4 probes.

The intention behind failing mmap()/fork() if uprobe_mmap failed, was to make sure that we always report the correct number of events.

OK, so what you suggest for now?

Please note that it is very trivial to crash the kernel. Just do something like

echo "p /bin/true:OFFSET_OF_SYSCALL_INSN" > /sys/kernel/debug/tracing/uprobe_events /bin/true

uprobe_mmap()->install_breakpoint() can fail if the probed insn is not supported

However failing mmap_region()/do_fork() because of a probe on an unsupported instruction is wrong.

Hence change uprobe_mmap() to ignore unsupported instructions.

Quoted 2 lines

uprobe_mmap()->install_breakpoint() can fail if the probed insn is not supported

But there are other reasons why it can fail,

Quoted 2 lines

However failing mmap_region()/do_fork() because of a probe on an unsupported instruction is wrong.

Srikar, I strongly, absolutely disagree. Please correct me, but..

Do you realize how much uprobes hooks in mmap_region/dup_mmap are broken?

(cough, can't resist, vma_adjust()->uprobe_mmap() is not right too, but
this is another story).

OK, lets start with dup_mmap:

		// retval == 0

		if (file && uprobe_mmap(tmp))
			goto out;

	out:
		up_write(&mm->mmap_sem);
		flush_tlb_mm(oldmm);
		up_write(&oldmm->mmap_sem);
		return retval;

Given that retval == 0, what do you think dup_mmap() returns if uprobe_mmap() fails? And note that we didn't copy all vmas. OK, at least this can't crash (afaics), and easy to fix.

But mmap_region() is worse, much worse. It simply can _not_ fail after uprobe_mmap (of course, I am not saying this is unfixable) without the crash. And note that the crash is "delayed". And btw, like dup_mmap(), mmap_region() doesn't return the error too.

Srikar, I strongly believe this horror must not exist. Either we should teach mmap_region() and dup_mmap() (and vma_adjust!) to fail correctly, or we should ignore the error code.

It is that simple, isn't it?

Whatever you do with uprobe_mmap(), even if you change it to always return 0, the code in mmap_region() is absolutely, absolutely broken.

Do you agree?

And once again. I agree, in the long term we should reconsider this change. But we need a simple fix for now/stable.

Quoted 1 line

Hence change uprobe_mmap() to ignore unsupported instructions.

OK. Now suppose that mmap_region()-> uprobe_mmap() fails because the caller is SIGKILL'ed (so __get_user_pages fails). Given that mmap_region() can't handle the error correctly, the kernel can crash.

How about having your fix + my patch to fix the uprobe_mmap()?

OK, lets start with dup_mmap:

		// retval == 0

		if (file && uprobe_mmap(tmp))
			goto out;

	out:
		up_write(&mm->mmap_sem);
		flush_tlb_mm(oldmm);
		up_write(&oldmm->mmap_sem);
		return retval;

Given that retval == 0, what do you think dup_mmap() returns if uprobe_mmap() fails? And note that we didn't copy all vmas. OK, at least this can't crash (afaics), and easy to fix.

But mmap_region() is worse, much worse. It simply can _not_ fail after uprobe_mmap (of course, I am not saying this is unfixable) without the crash. And note that the crash is "delayed". And btw, like dup_mmap(), mmap_region() doesn't return the error too.

Srikar, I strongly believe this horror must not exist. Either we should teach mmap_region() and dup_mmap() (and vma_adjust!) to fail correctly, or we should ignore the error code.

It is that simple, isn't it?

Quoted 11 lines

But mmap_region() is worse, much worse. It simply can _not_ fail after uprobe_mmap (of course, I am not saying this is unfixable) without the crash. And note that the crash is "delayed". And btw, like dup_mmap(), mmap_region() doesn't return the error too.

Srikar, I strongly believe this horror must not exist. Either we should teach mmap_region() and dup_mmap() (and vma_adjust!) to fail correctly, or we should ignore the error code.

It is that simple, isn't it?

I think you would have thought of this approach already but just wanted to check if below is fine with you.

Forgot to mention...

kernel/events/uprobes.c int uprobe_mmap(struct vm_area_struct *vma) · line 1057

Quoted 13 lines · Srikar Dronamraju

While at it, add a missing put_uprobe() in the path where uprobe_mmap() races with uprobe_unregister().
...

kernel/events/uprobes.c +3 -1

@@ -1051,8 +1051,10 @@ int uprobe_mmap(struct vm_area_struct *vma)
 			if (ret == -EEXIST) {
 				ret = 0;
 
-				if (!is_swbp_at_addr(vma->vm_mm, vaddr))
+				if (!is_swbp_at_addr(vma->vm_mm, vaddr)) {
+					put_uprobe(uprobe);
 					continue;
+				}

Yes, this part looks correct.

In fact, I think this is not really correct anyway (wrt counter) but we are going to kill it.

Quoted 19 lines · Oleg Nesterov

Forgot to mention...

kernel/events/uprobes.c int uprobe_mmap(struct vm_area_struct *vma) · line 1057

Quoted 13 lines · Srikar Dronamraju

While at it, add a missing put_uprobe() in the path where uprobe_mmap() races with uprobe_unregister().
...

kernel/events/uprobes.c +3 -1

@@ -1051,8 +1051,10 @@ int uprobe_mmap(struct vm_area_struct *vma)
 			if (ret == -EEXIST) {
 				ret = 0;
 
-				if (!is_swbp_at_addr(vma->vm_mm, vaddr))
+				if (!is_swbp_at_addr(vma->vm_mm, vaddr)) {
+					put_uprobe(uprobe);
 					continue;
+				}

Yes, this part looks correct.

Quoted 4 lines

In fact, I think this is not really correct anyway (wrt counter) but we are going to kill it.

Are you expecting the counter to be decreased/increased here?

This is case where the uprobe_mmap() and uprobe_unregister() raced, and by the time install_breakpoint() was called by uprobe_mmap(), there were no consumers.

i.e there are no uprobe->consumers and the underlying instruction is still not a breakpoint instruction.

Quoted 22 lines · Srikar Dronamraju

Quoted 19 lines · Oleg Nesterov

Forgot to mention...

kernel/events/uprobes.c int uprobe_mmap(struct vm_area_struct *vma) · line 1057

Quoted 13 lines · Srikar Dronamraju

While at it, add a missing put_uprobe() in the path where uprobe_mmap() races with uprobe_unregister().
...

kernel/events/uprobes.c +3 -1

@@ -1051,8 +1051,10 @@ int uprobe_mmap(struct vm_area_struct *vma)
 			if (ret == -EEXIST) {
 				ret = 0;
 
-				if (!is_swbp_at_addr(vma->vm_mm, vaddr))
+				if (!is_swbp_at_addr(vma->vm_mm, vaddr)) {
+					put_uprobe(uprobe);
 					continue;
+				}

Yes, this part looks correct.

Quoted 6 lines

Quoted 4 lines

In fact, I think this is not really correct anyway (wrt counter) but we are going to kill it.

Are you expecting the counter to be decreased/increased here?

uprobes_state.count is very wrong, afaics. I'll try to send the fixes "soon", after we solve the pending problems (this one + stepping).

Quoted 3 lines

This is case where the uprobe_mmap() and uprobe_unregister() raced, and by the time install_breakpoint() was called by uprobe_mmap(), there were no consumers.

Yes, exactly, and this case doesn't look 100% right too,

Quoted 2 lines

i.e there are no uprobe->consumers and the underlying instruction is still not a breakpoint instruction.

Yes, but what if it _IS_ "int3" ?

Yet another reason to move arch_uprobe_analyze_insn/etc to _register.

Quoted 8 lines · Oleg Nesterov

Quoted 4 lines · Srikar Dronamraju

This is case where the uprobe_mmap() and uprobe_unregister() raced, and by the time install_breakpoint() was called by uprobe_mmap(), there were no consumers.

Yes, exactly, and this case doesn't look 100% right too,

Quoted 4 lines

Quoted 2 lines

i.e there are no uprobe->consumers and the underlying instruction is still not a breakpoint instruction.

Yes, but what if it _IS_ "int3" ?

for int3, install_breakpoint returns -ENOTSUPP as install_breakpoint does an explicit check if the instruction is breakpoint instruction and x86 analyse_insn() also returns -ENOTSUPP.

Quoted 2 lines

Yet another reason to move arch_uprobe_analyze_insn/etc to _register.

I am for moving the stuff to _register that avoids us from looking at these cases.

https://bugzilla.redhat.com/show_bug.cgi?id=843640

If mmap_region()->uprobe_mmap() fails, unmap_and_free_vma path does unmap_region() but does not remove the soon-to-be-freed vma from rb tree (actually there are more problems).

Perhaps we could do do_munmap() + return in this case, but in fact it is simply wrong to abort if uprobe_mmap() fails. Until at least we move the !UPROBE_COPY_INSN code from install_breakpoint() to uprobe_register().

For example, uprobe_mmap()->install_breakpoint() can fail if the probed insn is not supported (remember, uprobe_register() succeeds if nobody mmaps inode/offset), mmap() should not fail in this case.

dup_mmap()->uprobe_mmap() is wrong too by the same reason, fork() can race with uprobe_register() and fail for no reason if it wins the race and does install_breakpoint() first.

Change mmap_region() and dup_mmap() to ignore the error code from uprobe_mmap().

Reported-by William Cohen <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

Cc <[email protected]> # v3.5

---
 kernel/fork.c |    4 ++--
 mm/mmap.c     |    5 ++---
 2 files changed, 4 insertions(+), 5 deletions(-)

kernel/fork.c +2 -2

index ab5211b..54bb88a 100644
@@ -454,8 +454,8 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
 		if (retval)
 			goto out;
 
-		if (file && uprobe_mmap(tmp))
-			goto out;
+		if (file)
+			uprobe_mmap(tmp);
 	}
 	/* a new mm has just been created */
 	arch_dup_mmap(oldmm, mm);

mm/mmap.c +2 -3

index 4fe2697..f25fd3f 100644
@@ -1355,9 +1355,8 @@ out:
 	} else if ((flags & MAP_POPULATE) && !(flags & MAP_NONBLOCK))
 		make_pages_present(addr, addr + len);
 
-	if (file && uprobe_mmap(vma))
-		/* matching probes but cannot insert */
-		goto unmap_and_free_vma;
+	if (file)
+		uprobe_mmap(vma);
 
 	return addr;
 

Signature · 1 line

1.5.5.1