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:	Fri, 19 Mar 2010 00:16:32 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Alan Cox <alan@...ux.intel.com>,
	Roland McGrath <roland@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] move tty_kref_put() outside of __cleanup_signal()

On 03/18, Alan Cox wrote:
>
> > -			tty_kref_put(p->signal->tty);
> >  			p->signal->tty = tty_kref_get(current->signal->tty);
>
> That bit needs commenting clearly or a WARN_ON() that p->signal->tty is
> NULL before the get otherwise when the assumption is broken the flaw will
> be subtle and hard to find.

Well. It must be NULL. This tty_kref_get() was added by
9c9f4ded90a59eee84e15f5fd38c03d60184e112, and the same commit also added
sig->tty = NULL into copy_signal(). With the recent changes in copy_signal()
it must be NULL due to zalloc().

But this doesn't matter. We should not do tty_kref_put() even if it is not
NULL. If we do "put", we need the balancing "get" which we do not have.
That is why this "get" is not only unneeded, but wrong imho.

Probably the little comment makes sense, but I don't really understand what
it should explain. To me, this is like list_add_tail(&init_task.tasks) few
lines below. We do not have the comment to explain the "missing" list_del().
This task is new, nobody can see/use it before we drop the locks. NULL or not,
its signal->tty is just uninitialized yet.

> > --- 34-rc1/kernel/exit.c~7_TTY_PUT	2010-03-17 20:05:38.000000000 +0100
> > +++ 34-rc1/kernel/exit.c	2010-03-18 22:46:41.000000000 +0100
> > @@ -150,6 +150,7 @@ static void __exit_signal(struct task_st
> >  		 * see account_group_exec_runtime().
> >  		 */
> >  		task_rq_unlock_wait(tsk);
> > +		tty_kref_put(sig->tty);
>
> and a sig->tty = NULL assignment to trap races might not go amiss here
> perhaps ?

Indeed ;)

The subsequent patches will do this, we need more changes anyway. Currently
this doesn't matter because we are going to kfree() this memory unconditionally.
But when we pin ->signal to task_struct, we should clear ->signal->tty before
we drop ->siglock, then tty_kref_put().

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/

Powered by blists - more mailing lists