linux-kernel - Re: [PATCH] uprobes: mmap_region() corrupts mm->mm_rb if uprobe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120731124805.GA485@redhat.com>
Date:	Tue, 31 Jul 2012 14:48:05 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Cc:	Ingo Molnar <mingo@...e.hu>, Anton Arapov <anton@...hat.com>,
	"Frank Ch. Eigler" <fche@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	William Cohen <wcohen@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] uprobes: mmap_region() corrupts mm->mm_rb if
	uprobe_mmap() fails

On 07/31, Srikar Dronamraju wrote:
>
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -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);
>
> I am not comfortable with this fix.

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

(or any other unsupported insn)

Yes sure, I agree that in the long term this change should be
reconsidered.

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

Exactly. And we already discusssed this, we have a lot of other
reasons to do this.

> 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.

Yes. And I already have the patch. I didn't send it because, unlike
this fix, it depends on other changes (already in -tip).

Until we move analysis to register, until we teach the callers of
uprobe_mmap() to bailout (and please note that vma_adjust() ignores
the result too), uprobe_mmap() should not give up if install fails,
it should continue.

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

Sure, I understand and agree.

But what we can do right now?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/