linux-kernel - Re: [PATCH 5/5] uprobes: Change uprobe_copy_process() to dup xol

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: <20131016125327.GI19729@linux.vnet.ibm.com>
Date:	Wed, 16 Oct 2013 18:23:27 +0530
From:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Ingo Molnar <mingo@...e.hu>, Anton Arapov <aarapov@...hat.com>,
	David Smith <dsmith@...hat.com>,
	"Frank Ch. Eigler" <fche@...hat.com>,
	Martin Cermak <mcermak@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] uprobes: Change uprobe_copy_process() to dup xol_area

* Oleg Nesterov <oleg@...hat.com> [2013-10-13 21:18:44]:

> This finally fixes the serious bug in uretprobes: a forked child
> crashes if the parent called fork() with the pending ret probe.
> 
> Trivial test-case:
> 
> 	# perf probe -x /lib/libc.so.6 __fork%return
> 	# perf record -e probe_libc:__fork perl -le 'fork || print "OK"'
> 
> (the child doesn't print "OK", it is killed by SIGSEGV)
> 
> If the child returns from the probed function it actually returns
> to trampoline_vaddr, because it got the copy of parent's stack
> mangled by prepare_uretprobe() when the parent entered this func.
> 
> It crashes because a) this address is not mapped and b) until the
> previous change it doesn't have the proper->return_instances info.
> 
> This means that uprobe_copy_process() has to create xol_area which
> has the trampoline slot, and its vaddr should be equal to parent's
> xol_area->vaddr.
> 
> Unfortunately, uprobe_copy_process() can not simply do
> __create_xol_area(child, xol_area->vaddr). This could actually work
> but perf_event_mmap() doesn't expect the usage of foreign ->mm. So
> we offload this to task_work_run(), and pass the argument via not
> yet used utask->vaddr.
> 
> We know that this vaddr is fine for install_special_mapping(), the
> necessary hole was recently "created" by dup_mmap() which skips the
> parent's VM_DONTCOPY area, and nobody else could use the new mm.


I was actually thinking if we can remove the VM_DONTCOPY from
install_special_mapping, But there are obvious issues with that approach
+ lot more house keeping. So your approach is much much better.

> 
> Unfortunately, this also means that we can not handle the errors
> properly, we obviously can not abort the already completed fork().
> So we simply print the warning if GFP_KERNEL allocation (the only
> possible reason) fails.
> 
> Cc: stable@...r.kernel.org # 3.9+
> Reported-by: Martin Cermak <mcermak@...hat.com>
> Reported-by: David Smith <dsmith@...hat.com>
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>

Acked-by: Srikar Dronamraju <srikar@...ux.vnet.ibm.com>

-- 
Thanks and Regards
Srikar Dronamraju

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