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]
Date:	Wed, 24 Dec 2008 13:04:04 -0800
From:	Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	ebiederm@...ssion.com, roland@...hat.com, bastian@...di.eu.org,
	daniel@...ac.com, xemul@...nvz.org, containers@...ts.osdl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCH 7/7][v4] SI_TKILL: Masquerade si_pid when crossing
	pid ns boundary

Oleg Nesterov [oleg@...hat.com] wrote:
| On 12/24, Sukadev Bhattiprolu wrote:
| >
| > --- a/kernel/signal.c
| > +++ b/kernel/signal.c
| > @@ -2385,17 +2385,22 @@ static int do_tkill(pid_t tgid, pid_t pid, int sig)
| >  	struct siginfo info;
| >  	struct task_struct *p;
| >  	unsigned long flags;
| > +	struct pid_namespace *ns;
| >
| >  	error = -ESRCH;
| >  	info.si_signo = sig;
| >  	info.si_errno = 0;
| >  	info.si_code = SI_TKILL;
| > -	info.si_pid = task_tgid_vnr(current);
| >  	info.si_uid = current_uid();
| >
| >  	rcu_read_lock();
| >  	p = find_task_by_vpid(pid);
| >  	if (p && (tgid <= 0 || task_tgid_vnr(p) == tgid)) {
| > +		ns = task_active_pid_ns(p);
| > +		if (ns)
| > +			info.si_pid = task_tgid_nr_ns(current, ns);
| > +		else
| > +			info.si_pid = task_tgid_vnr(current);
| 
| if ns == 0, "p" won't see the signal anyway, so all we need is

Yes, p won't see the signal, but task_tgid_nr_ns() is not safe if ns == NULL.
We should either check ns or make task_tgid_nr_ns() and friends safe with
ns == NULL ?

| 
| 	-	info.si_pid = task_tgid_vnr(current);
| 	+	info.si_pid = task_tgid_nr_ns(current, task_active_ns(p));
| 
| like we do in __do_notify().

Yes, I had a question about ns == 0 in this patch and was wondering if I
should add a check in __do_notify() too.

| 
| 
| But. this of course doesn't work for sys_kill(). Can't we change the helpers
| which send SI_FROMUSER() signals so that they do not fill .si_pid at all?

SI_FROMUSER() basically comes down to SI_USER and SI_TKILL (SI_QUEUE,
SI_SIGIO, SI_DETHREAD are unused ?)  SI_USER has to be masqueraded in
send_signal(). That leaves us with SI_TKILL. 

I was trying to have all si_pid settings done at origin and so the change
here for SI_TKILL. But yes, SI_USER (sys_kill() case) can't be done at
origin hence the special case for it in send_signal().

| Then send_signal() can do:
| 
| 	default:
| 		copy_siginfo(&q->info, info);
| 		info.si_pid = 0;
| 		if (!from_ancestot_ns)
| 			info.si_pid = task_tgid_nr_ns(current, ...);
| 
| ?


My preference was to address SI_TKILL also at origin, but am not
particular.  Yes, that will work too.


| 
| Yes, we use "current". But we already used it in siginfo_from_ancestor_ns().
| 
| 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/
--
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/

Powered by blists - more mailing lists